pepkit / looper

A job submitter for Portable Encapsulated Projects
http://looper.databio.org
BSD 2-Clause "Simplified" License
20 stars 7 forks source link

Add remaining arguments to `run` command model #448

Closed simeoncarstens closed 5 months ago

simeoncarstens commented 5 months ago

While #441 is meant to just get the hello_looper example working, meaning just a looper run with the basic example config file given in the hello_looper repository, it lacks a couple arguments that make it fully compatible with general looper run uses, as pointed out by @donaldcampbelljr in https://github.com/pepkit/looper/pull/440#issuecomment-1910277389.

This PR aims to add all remaining arguments to the run command, and thus to provide full feature compatibility (for the run command) between the current argparse-based looper CLI and the pydantic-argparse-based one.

We encourage all looper users to check whether their looper run commands work as intended when using the looper-pydantic-argparse script as described in #441.

[!NOTE] There are still no tests for that in the test suite - it is not clear we are going to keep pydantic-argparse as a library to build a CLI from pydantic models, but these models are the foundation of the HTTP API being developed in #441.

simeoncarstens commented 5 months ago

@donaldcampbelljr feel free to look at this and try it out! It's in draft mode for now, because we still need to fix the tests / refactor in #441, on which this PR is based.

donaldcampbelljr commented 5 months ago

Just to confirm the current usage for the new functionality:

I am trying to run the new command via

looper-pydantic-argparse run --looper-config .looper_pipestat.yaml

Attempting to run this on the other hello_looper examples yields an error of unrecognized arguments:

(venv) drc@databio:~/GITHUB/hello_looper/hello_looper$ looper-pydantic-argparse run --looper-config .looper_pipestat.yaml
usage: looper [-h] [--settings SETTINGS] [--pep-config PEP_CONFIG] [--output-dir OUTPUT_DIR] [--config-file CONFIG_FILE] [--looper-config LOOPER_CONFIG]
              [--sample-pipeline-interfaces SAMPLE_PIPELINE_INTERFACES [SAMPLE_PIPELINE_INTERFACES ...]] [--project-pipeline-interfaces PROJECT_PIPELINE_INTERFACES [PROJECT_PIPELINE_INTERFACES ...]] [--amend AMEND [AMEND ...]]
              [--sel-flag SEL_FLAG] [--exc-flag EXC_FLAG] [--silent] [--verbosity VERBOSITY] [--logdev] [--pipestat PIPESTAT]
              {run} ...
looper: error: unrecognized arguments: --looper-config .looper_pipestat.yaml

However, using looper-pydantic-argparse run works fine, because there is a .looper.yaml file in the same folder.

simeoncarstens commented 5 months ago

Thanks, @donaldcampbelljr, for trying it out again! As for your error, in our pydantic-argparse-based CLI, some arguments that seem to be common to all commands are now top-level arguments, and --looper-config is one of them. I didn't try due to the late hour here in Europe, but if you move --looper-config before the run subcommand, chances are it might work.

donaldcampbelljr commented 5 months ago

Ah ok, excellent. That does work.

I was able to run the pipestat example in hello_looper!

I tried some other commands with mixed results:

I can pass this --sel-flag but its not working properly since it should only run samples that have "failed" flag: looper-pydantic-argparse --looper-config .looper_pipestat.yaml --sel-flag "failed" run (see this for context: https://github.com/pepkit/looper/issues/126) Essentially, it ran all the samples anyway.

I also tried using the dry run flag -d and it did not recognize that argument.

simeoncarstens commented 5 months ago

@donaldcampbelljr thanks again! Turns out we ignored the --sel-flag and similar options in some early hacking. This is fixed now. If I run the hello_looper pipestat example with --sel-flag "failed", it tells me that zero samples are valid for job generation, while without that option, two samples are submitted. Would that be the expected behavior? As for -d, unfortunately, the pydantic-argparse-based CLI / the pydantic-argparse command models don't support short-form arguments / flags. That's currently a limitation of pydantic-argparse :cry: But --dry-run as an argument to the run command works for me.

donaldcampbelljr commented 5 months ago

Excellent! Yes, --sel-flag "failed" should not run any of the samples since (hopefully) no failed flags exist for either of the samples.

Thank you for the clarification of the short-form arguments.

This looks good on my end.