Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI v2 #3600

Draft
wants to merge 61 commits into
base: master
Choose a base branch
from
Draft

CLI v2 #3600

wants to merge 61 commits into from

Conversation

bentsherman
Copy link
Member

@bentsherman bentsherman commented Feb 2, 2023

Closes #3595

To summarize:

  • nextflow.cli.*Impl classes implement the core functionality of each command
  • nextflow.cli.v1.*Cmd classes implement the CLI v1 and pass options to corresponding *Impl class
  • nextflow.cli.v2.*Cmd classes implement the CLI v2 and pass options to corresponding *Impl class
  • nextflow.cli.v1.Launcher and nextflow.cli.v2.Launcher provide entrypoints to the v1 and v2 CLI respectively

TODO:

  • Add basic v2 CLI
  • Update tests
  • Test support for list values (keep same as CLI v1)
  • Test support for params
  • Test support for basic type conversion (keep same as CLI v1)
  • Update build/install process to provide both CLIs

Signed-off-by: Ben Sherman <[email protected]>
@pditommaso
Copy link
Member

I'd propose keeping the current CLI in the current package. That would make the PR much simpler

Signed-off-by: Ben Sherman <[email protected]>
@bentsherman

This comment was marked as outdated.

@pditommaso

This comment was marked as outdated.

@bentsherman

This comment was marked as outdated.

@bentsherman
Copy link
Member Author

@pditommaso Today I realized that picocli can't really tell the difference between pipeline args and pipeline params in the run command. Consider the example:

nextflow run script.nf arg1 arg2 --param1 value1 --param2=value2

Whereas jcommander can use @DynamicParameter(names='--') to catch the params, picocli doesn't have the same trick. So picocli will interpret all of these as args, regardless of whether they occur before or after the double dash --. The double dash is only useful to distinguish params from CLI options.

So there are two solutions:

  1. Remove support for pipeline args. It's a breaking change but I'm not sure that anyone uses positional args on pipeline scripts in the first place?
  2. Assume the first positional arg that starts with -- is the first param and interpret everything after that as params.

I'm assuming you don't want to do (1), so let's explore (2) for a moment.

In the above example, picocli would parse the positional args as:

args = ['arg1', 'arg2', '--param1', 'value1', '--param2=value2']

Then Nextflow would separate the args from params as follows:

scriptArgs = ['arg1', 'arg2']
args = ['param1': 'value1', 'param2': 'value2']

Since the CLI has no idea what args or params are expected by the pipeline script at this point, I think this is the best we can do. Basically it means that you can't provide an arg value that looks like an option.

@nextflow-io nextflow-io deleted a comment from sonatype-lift bot Feb 15, 2023
@nextflow-io nextflow-io deleted a comment from sonatype-lift bot Feb 15, 2023
@pditommaso

This comment was marked as outdated.

@bentsherman

This comment was marked as outdated.

@marcodelapierre
Copy link
Member

marcodelapierre commented Dec 14, 2023

Great job Ben ! 🧨💥🚀

@bentsherman
Copy link
Member Author

Good catch. We should also think about the unit tests and integration tests. All of the integration tests use the current CLI, so it would be good to have a single e2e test for the CLI v2 that simply runs a bunch of different commands (see #3600 (comment))

@ewels
Copy link
Member

ewels commented Dec 19, 2023

The ANSI logger isn't initialized at this point so I just used the ANSI codes directly

This is a bit risky - note that the ANSI codes will be dumped to redirected text files / anything that can't support ANSI output.

The ANSI logger is smarter than that and strips the codes if the output doesn't support them.

@ewels
Copy link
Member

ewels commented Dec 19, 2023

Couple of thoughts from looking at the help output:

  • Any reason why -remote-debug only has one hyphen?
  • Some options like --bg look like short versions, could that be -bg/--background?
  • Can we print the Nextflow 1-line header above help text? (Maybe say, with a nice green background? - would require ANSI library init though)

@marcodelapierre
Copy link
Member

@pditommaso , we would be keen on your feedback on this PR, firstly from a feature-set perspective, and optionally from the tech standpoint. Thank you!

@bentsherman
Copy link
Member Author

This is a bit risky - note that the ANSI codes will be dumped to redirected text files / anything that can't support ANSI output.

Okay, I just removed the ANSI logging from this part

Any reason why -remote-debug only has one hyphen?

Good catch

Some options like --bg look like short versions, could that be -bg/--background?

Since bg is two characters I think it has to be double-dash, but I can also add --background for posterity

Can we print the Nextflow 1-line header above help text?

Looks non trivial as the help text is customized in an annotation. It's probably possible but I'm not going to bother with it. You're welcome to try 😄

@marcodelapierre
Copy link
Member

@bentsherman can you check the merge conflicts from master?

@bentsherman
Copy link
Member Author

For the new CLI I think we should consider removing support for args in the run command. You can specify args and access them in the pipeline script via args[0], args[1], etc. This seems like more of a DSL1 concept that doesn't really fit with our vision for formalizing the workflow inputs. And I've never seen anyone use it, not sure if anyone even knows it exists. Removing it would simplify the params parsing in the new CLI. Just some food for thought for the reviewers.

@marcodelapierre
Copy link
Member

Good point Ben.

I personally did NOT know of the feature. I see it is not documented in docs.nextflow.io, and that it is not used across nf-core (see e.g. this search https://github.com/search?q=org%3Anf-core+path%3A*.nf+args%5B0+&type=code).

I agree it clashes with our discourse on more formal input schemas.

I am in favour of dropping it.

@ewels
Copy link
Member

ewels commented Feb 12, 2024

I've heard of some folks using it, eg. @FelixKrueger / @s-andrews at Babraham.

We've actually just made use of it for the first time in @nf-core a few days ago, purely as a method to autodetect malformed profile statements (it's a common error to put a space after the comma, eg. -profile test, docker).

Agree that we should remove it, as the majority of the time it's just a way for stuff to fail silently. Having a hard failure would be better.

The main valid use is to be able to glob input files. But yeah, I think very few do that and there are probably better ways to handle it (eg. if / when we build tooling to automate the generation of sample sheets for local files).

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman
Copy link
Member Author

From our conversation about this PR last week -- if we remove run args from CLI v1, that would allow us to migrate CLI v1 from jcommander to picocli, with the benefits of better
parsing and help text. It is one way to salvage PR if we don't have the appetite to maintain a separate CLI just to use double dashes for long options. But it might just be a good thing to do either way.

My understanding was that if we do release CLI v2 then we would eventually deprecate the old CLI, so the extra scaffolding to maintain both would be temporary.

@bentsherman
Copy link
Member Author

Moving this to draft. I am now also wary about introducing this change amidst everything else that we are doing.

I would prefer to refactor the existing CLI to picocli to fix the various issues with option parsing, and maybe also the improvements to CLI params described in #4669 , to make sure that we address the main CLI issues that users deal with.

After that, if there is still lots of interest in having a proper GNU command line, maybe we can do it more cleanly at that point.

@bentsherman bentsherman marked this pull request as draft October 21, 2024 16:56
@bentsherman bentsherman removed this from the 24.10.0 milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants