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

Changes to nf-core pipelines download CLI #3178

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

MatthiasZepper
Copy link
Member

This PR suggests a change to the CLI of nf-core download respectively now nf-core pipelines download.

To free the -p flag for Seqera Platform, the -d flag was needed for --parallel-downloads and thus --download-configs had to change from -d to -c.

This is also more consistent, since most short flags use the first letter of the last word as the short flag, e.g. -l for --container-library or -u for --container-cache-utilisation.

Furthermore, -t / --tower has now been entirely removed as it was deprecated before. Therefore -t could have been used for --tag in the future, but there is hardly a point of having a short flag for a three letter argument, so I refrained from introducing it.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated Does that still apply? I looked into the new docs folder, but it seems the old manual Markdown was replaced by some autogenerated API documentation: docs/api/_src/api/pipelines/download.md

@MatthiasZepper MatthiasZepper added this to the 3.0 milestone Sep 23, 2024
@mirpedrol
Copy link
Member

Hi @MatthiasZepper, thanks for this PR. Docs are now on the website repo here.
May I ask why --download-configuration has changed to yes/no instead of being a flag?

@MatthiasZepper
Copy link
Member Author

MatthiasZepper commented Sep 25, 2024

Thanks for taking a look already.

May I ask why --download-configuration has changed to yes/no instead of being a flag?

Because I have occasionally received complaints similar to #3043 and the prompt for downloading the configs was one of them. Technically, if stderr.is_interactive: should prevent prompts in non-interactive shells, but apparently there are users where this fails.

I never felt it was a pressing issue, but kept it in the back of my head for the next major version jump.

@MatthiasZepper MatthiasZepper marked this pull request as ready for review September 27, 2024 13:45
@mashehu
Copy link
Contributor

mashehu commented Sep 30, 2024

I would keep it as a flag. Pointing towards the one-liner approach suggested by @ewels should be enough.

@MatthiasZepper
Copy link
Member Author

MatthiasZepper commented Sep 30, 2024

I would keep it as a flag. Pointing towards the one-liner approach suggested by @ewels should be enough.

But the download configuration prompt and the $NXF_SINGULARITY_CACHEDIR are two different things ?!?

@mashehu
Copy link
Contributor

mashehu commented Oct 1, 2024

okay, then I misunderstood why you linked the issue... just for clarity's sake: one can add the also the download-configuration with $NFCORE_DOWNLOAD-CONFIGURATION

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we are only missing updating the docs for these changes. Is this PR ready to be merged @MatthiasZepper ?

@MatthiasZepper
Copy link
Member Author

just for clarity's sake: one can add the also the download-configuration with $NFCORE_DOWNLOAD-CONFIGURATION

I did not know that. I also do not find it in the code. So how does that work?

            if not self.platform and self.include_configs is None:
                self.prompt_config_inclusion()

LGTM, we are only missing updating the docs for these changes. Is this PR ready to be merged @MatthiasZepper ?

From my side, absolutely, since there is seemingly no need to address the CACHE DIR issue, since the OP did never reply. But I do not think that @mashehu is happy with the other changes yet?

@mashehu
Copy link
Contributor

mashehu commented Oct 1, 2024

the env. variable thing comes with click: https://click.palletsprojects.com/en/8.1.x/options/#values-from-environment-variables

@MatthiasZepper MatthiasZepper merged commit 2a35da1 into nf-core:dev Oct 1, 2024
83 checks passed
@MatthiasZepper MatthiasZepper deleted the download_cli_refactor branch October 1, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants