-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
This comment has been minimized.
This comment has been minimized.
Sometimes we add the nf-core/ prefix to the pipeline name, so return that too.
ok @KevinMenden / others - I think that this is ready for final review + merge now. Let me know if you spot anything else 😁 |
Great will go over it today :-) |
There was a problem hiding this 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 🤔
@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" | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍🏻
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
Although when I then run it, it breaks and shows me that it used a path something like:
Probably some weirdness with the |
Thanks for the brilliant reviewing both - this is picking up some really good edge case stuff! 😅 Keep it coming! |
Other than the weird home directory stuff, I couldn't find an issue 👍 good from my side! |
@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? |
I can't test right now, but it still seems there should be a But I may be wrong and don't think that's super important, so merging is fine with me. |
I think it should be fine as it's |
Ah, yes, should be fine. |
I double-checked again and now it works fine with |
Thanks all! |
Screencast demo: nf-core-download.mov |
Added more interactive prompts to
download
andlaunch
commands in accordance to #1009:Download:
-r
flag is not set.--container
(previously--singularity
) and--singularity-cache
flags are not set.--compression
flag is not set.export NXF_SINGULARITY_CACHEDIR=path
to.bashrc
if not already present.Launch:
PR checklist
CHANGELOG.md
is updateddocs
is updated