feat: run subcommand #47
Conversation
This also adds the ability to check/lint URLs
| #[derive(Parser, Debug)] | ||
| #[command(author, version, about)] | ||
| pub struct RunArgs { | ||
| /// The path or URL to the WDL document containing the task to run. |
There was a problem hiding this comment.
Do we want to write these as task? Eventually, it will support workflows, so should the comments reflect that or just the current functionality?
There was a problem hiding this comment.
I think we should reflect the current functionality, i.e. specify that we only look at tasks for now. I wouldn't be opposed to making the language more agnostic as a future-proof, so long as it doesn't imply anything we don't currently support.
| /// Runs a task. | ||
| pub async fn run(args: RunArgs) -> Result<()> { | ||
| eprintln!( | ||
| "the `run` command is in alpha testing and does not currently support workflows or using \ | ||
| containers." | ||
| ); |
There was a problem hiding this comment.
With the warning being printed, I would suggest we use something like workload, executable, etc. instead of task to represent that this will eventually support larger executions.
There was a problem hiding this comment.
Ah, you mean in the doc comment? I was confused for a second there. Yea that makes sense to me, can update the comment.
There was a problem hiding this comment.
Actually, on second thought I think this should reflect the current capabilities. I don't think this comment should change. Same reasoning as explained higher up in another thread.
src/commands/run.rs
Outdated
| .context("failed to find document in analysis results")?; | ||
| let document = result.document(); | ||
|
|
||
| let mut engine = Engine::new(LocalTaskExecutionBackend::new()); |
There was a problem hiding this comment.
Do we have a concept for how the execution backend will be chosen in the future?
There was a problem hiding this comment.
I don't have much of a vision for that yet, but I imagine Clay and Peter have thoughts. I think it's safe to assume that without advanced configuration/options, we will default to the local backend.
src/commands/run.rs
Outdated
| "inputs file `{path}` is empty; use the `--name` option to specify the name of \ | ||
| the task or workflow to run", |
There was a problem hiding this comment.
SInce there can be only one workflow in a file, is it safe to make an assumption to run that if name isn't specified? That's what the other runners do correct?
There was a problem hiding this comment.
Right now, wdl-engine expects the name of the task or workflow to be specified in the JSON (in the form "<name of task or workflow>.<name of parameter>": <value>). I played around with trying to loosen that requirement within the sprocket bin, but I think it will require some tweaks upstream.
So if no input JSON is supplied, we currently rely on --name to select what task/workflow to run. I suppose it would be fine if both are omitted to run any found workflow by default. I can make that change.
There was a problem hiding this comment.
However, I don't think it's really a common use-case to omit the input JSON, so I'm not certain how important this is. The input JSON must specify the name of the task or workflow, so supplying --name is redundant or leads to contradictions, so isn't allowed at the same time. There aren't many tasks or workflows "in the wild" that you can run without specifying any parameters (which currently must be done via JSON).
There was a problem hiding this comment.
To clarify: the only time a user can or should be using the --name parameter is if they want to run a task (or workflow) without any arguments. This is a pretty niche use case.
However in a future update, we will probably want to allow CL specified parameters, and when we add that we will want to use this --name flag to enable that behavior.
I think it's fine to require specificity from users and supply the workflow name if they want to use this feature.
Co-authored-by: Andrew Thrasher <[email protected]>
No description provided.