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

Interactive prompts for 'download' and 'launch' #1027

Merged
merged 50 commits into from
May 10, 2021

Conversation

ErikDanielsson
Copy link
Contributor

@ErikDanielsson ErikDanielsson commented Apr 16, 2021

Added more interactive prompts to download and launch commands in accordance to #1009:

Download:

  • Version selection list if -rflag is not set.
  • Confirmation when --container (previously --singularity) and --singularity-cache flags are not set.
  • Selection list of compression types when --compression flag is not set.
  • Option to append export NXF_SINGULARITY_CACHEDIR=path to .bashrc if not already present.

Launch:

  • Selection list to specify version of pipeline.

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

@ewels ewels marked this pull request as ready for review April 29, 2021 23:16
nf_core/launch.py Outdated Show resolved Hide resolved
nf_core/__main__.py Outdated Show resolved Hide resolved
@ewels

This comment has been minimized.

@ewels ewels added WIP Work in progress and removed ready-for-review labels May 1, 2021
@ewels ewels added ready-for-review and removed WIP Work in progress labels May 6, 2021
@ewels
Copy link
Member

ewels commented May 6, 2021

ok @KevinMenden / others - I think that this is ready for final review + merge now. Let me know if you spot anything else 😁

@KevinMenden
Copy link
Contributor

Great will go over it today :-)

Copy link
Contributor

@KevinMenden KevinMenden left a comment

Choose a reason for hiding this comment

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

Code looks all good to me! Only a few small comments, mostly related to the singularity stuff. Didn't properly work for me while testing 🤔

Comment on lines 214 to +216
@click.option(
"-c",
"--singularity-cache",
is_flag=True,
default=False,
help="Don't copy images to the output directory, don't set 'singularity.cacheDir' in workflow",
"-c", "--container", type=click.Choice(["none", "singularity"]), help="Download software container images"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we are planning to extend this choice list at some point, this could also just be flag instead of a prompt I think.
But also fine leaving it as a prompt.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to leave it as a prompt. One could make a semi-reasonable case for offering download to pull e.g. docker images to the local instance.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I went back on forth on this. At the moment it is just a --singularity flag. However, with the interactive prompts I would have needed to make it an on/off flag (--singularity --no-singularity). I don't love littering the cli with these, and figured that --container [str] was kind of more concise and had the added benefit of future-proofing.

@phue - as the only CharlieCloud user I know, does that have a similar concept of pulling image files? Would it ever be useful to add CharlieCloud image downloads to the nf-core download command for example?

Copy link
Member

Choose a reason for hiding this comment

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

And yes I also wondered about docker too @pontus 👍🏻

nf_core/download.py Show resolved Hide resolved
nf_core/download.py Show resolved Hide resolved
@KevinMenden
Copy link
Contributor

As discussed on slack, I'll try to explain how the path autocompletion breaks for me.

When asked for a path, I start by typing the ~ symbol as a shortcut to my home directory. And from there I can then actually access directories that are in my home, and complete the path to the desired directory. So that the path in the prompt looks something like this:

~/path/to/cache_dir

Although when I then run it, it breaks and shows me that it used a path something like:

/home/kevin/path/to/cache_dir/~/path/to/cache_dir

Probably some weirdness with the os.abspath in combination with questionary.path.

nf_core/download.py Outdated Show resolved Hide resolved
@ewels
Copy link
Member

ewels commented May 7, 2021

Thanks for the brilliant reviewing both - this is picking up some really good edge case stuff! 😅 Keep it coming!

@ewels ewels linked an issue May 9, 2021 that may be closed by this pull request
5 tasks
@KevinMenden
Copy link
Contributor

Other than the weird home directory stuff, I couldn't find an issue 👍 good from my side!

@ewels
Copy link
Member

ewels commented May 10, 2021

@KevinMenden - I think that the weird home directory stuff should be fixed now, after the last commit. If you're still having strange behaviour with homedirs can you let me know?

@pontus
Copy link
Contributor

pontus commented May 10, 2021

I can't test right now, but it still seems there should be a return in the abort case (empty answer) as it's inside a while.

But I may be wrong and don't think that's super important, so merging is fine with me.

@ewels
Copy link
Member

ewels commented May 10, 2021

I think it should be fine as it's while cachedir_path is None: and it sets cachedir_path to False.. (If I'm looking at the code that you're talking about anyway)..

@pontus
Copy link
Contributor

pontus commented May 10, 2021

Ah, yes, should be fine.

@KevinMenden
Copy link
Contributor

I double-checked again and now it works fine with ~ 👌

@ewels ewels merged commit 7735845 into nf-core:dev May 10, 2021
@ewels
Copy link
Member

ewels commented May 10, 2021

Thanks all!

@ewels
Copy link
Member

ewels commented May 11, 2021

Screencast demo:

nf-core-download.mov

@ErikDanielsson ErikDanielsson deleted the prompts branch July 25, 2022 09:02
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.

White on white can be hard to read Interactive prompts for nf-core download
4 participants