-
Notifications
You must be signed in to change notification settings - Fork 208
Refactor pipeline downloads command to use nextflow inspect
for container detection
#3634
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
Open
ErikDanielsson
wants to merge
122
commits into
nf-core:dev
Choose a base branch
from
ErikDanielsson:download-refactor-again
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+6,540
−2,621
Open
Changes from 106 commits
Commits
Show all changes
122 commits
Select commit
Hold shift + click to select a range
ae0d4dd
Moved all singularity-related download functions into a new file
muffato bbd297f
Added a helper method to use a temporary file as intermediate output
muffato a9b2b00
First round of fixing the tests
muffato 2797752
WIP: Start refactoring
JulianFlesch dc333b3
downloads.singularity functions should rely on proper parameters, no …
muffato fbc09ac
Duplicated with the line above
muffato fee7182
Added tests for utils.intermediate_file and fixed it !
muffato c507d9e
Simpler implementation
muffato aba5233
Added helper functions to manage the main task so that callers don't …
muffato 0403a37
No need to build a temp path here
muffato 159c34e
More tests
muffato 693da64
Fixed the symlink_registries tests
muffato c5379fb
Rewrote to support older versions of Python
muffato 9bf7bb3
Adds nextflow command call to extract containers
JulianFlesch 5dc30e2
Move import
JulianFlesch 563a330
Debug outpu
JulianFlesch 8ee8eed
Added a dest for the main_task-management methods added to DownloadPr…
muffato 71ab538
Added some tests for the DownloadProgress object
muffato a141820
Turned two methods into functions, simplified testing
muffato 4133c90
Test comments
muffato 2b8fbd6
Theoretically it could be None, so check it really is a string
muffato 72b6d60
Added docker class to match singularity class
nikhil 7813f30
More type hints
muffato cb71d42
More tests for get_container_filename
muffato 2d8cb4d
No need to keep a wraper as a method
muffato 4bfb45e
Better comments
muffato d6b8f6c
New class to support the downloads
muffato 4abd736
Introduced a context manager that creates a new sub task. Useful to g…
muffato 31f5627
Forgot that pytest.raises exists !
muffato 06d3760
Moved the download class to utils
muffato 3857fae
The output path cannot be a directory or a symbolic link
muffato 31fe914
Basic test for FileDownloader.download_file
muffato 02cc2ca
Empty files are not allowed
muffato 397ff11
Removes inspect param that keeps failing
JulianFlesch 0107a6c
Description
JulianFlesch 141d4d8
Adds debugging section
JulianFlesch b648bc1
Test log.debug too
muffato 467ba3f
Test the kill_with_fire flag
muffato ea4bc89
Refactored the FileDownloader
muffato 8038046
Merge branch 'dev' into dev-download
ErikDanielsson e4327f1
Merge branch 'singularity_downloads' into dev-download
ErikDanielsson 208daca
Merge branch 'feature/docker_downloads' into dev-download
ErikDanielsson a446ee9
Create a barely functioning version
ErikDanielsson b9b8388
Update and test download.py
ErikDanielsson 40be0b6
Merge branch 'dev' into download-refactor-again
ErikDanielsson cb8a93b
Merge branch 'dev' into download-refactor-again
ErikDanielsson 7ca0875
Move WorkflowRepo to separate file
ErikDanielsson 1684127
Start adding in docker support, add hiding of progress bars
ErikDanielsson 0739df0
Start adding container base class
ErikDanielsson 9913e56
Move legacy container finding into utils (for now) and update tests
ErikDanielsson c65d09f
Merge branch 'dev' into download-refactor-again
ErikDanielsson 0e0a5b8
Add mock pipeline, and move some stuff around
ErikDanielsson 8160181
Try to refactor singularity and docker container handling in this branch
ErikDanielsson 9b695f2
Remove refactoring code and add some nice messages
ErikDanielsson 7670666
Merge branch 'dev' into download-refactor-again
ErikDanielsson b64a77f
Nearing working refactor
ErikDanielsson 35df64b
Merge branch 'download-refactor-again' into refactor-docker-singularity
ErikDanielsson a18f611
Further refactoring
ErikDanielsson 6b44e74
Make DockerError and SingularityError classes (too much customization…
ErikDanielsson 41c7332
Integrate new classes and update tests
ErikDanielsson c5cf8e0
Merge branch 'dev' into download-refactor-again
ErikDanielsson 275adab
Fix failing tests
ErikDanielsson 56a76b5
Add error message for weird containers and testing of it
ErikDanielsson d58c282
Remove singularity and docker files without subclassing
ErikDanielsson 1964d50
Make concurrency nicer for docker
ErikDanielsson 6de9f41
Merge branch 'dev' into download-refactor-again
ErikDanielsson d1ce487
Organize mock modules
ErikDanielsson 05adb4d
Remove unused code
ErikDanielsson ced8df1
Apply suggestions from code review
ErikDanielsson 7a2da94
Remove error message for variable container name and corresponding tests
ErikDanielsson 7dba881
Revert removel of .sif extension for now
ErikDanielsson a49ee12
os.path -> pathlib.Path and typing
ErikDanielsson a413d93
Fix failing tests
ErikDanielsson e2ddf3d
^^
ErikDanielsson af44891
More typing, docker concurrency, and properties
ErikDanielsson f4c5e66
Update tests and progress bar
ErikDanielsson cb6cfc9
Merge branch 'dev' into download-refactor-again
ErikDanielsson 7d7b391
Do not select singularity silently
ErikDanielsson 1e414cd
Merge branch 'dev' into download-refactor-again
ErikDanielsson 4b5f617
Fix typing issue
ErikDanielsson 828f1eb
Change name of function and remove absolute container URI from non-si…
ErikDanielsson 414b81d
Move nf stuff from download/utils to utils
ErikDanielsson f4c7450
Check logic for self.amend_cachedir
ErikDanielsson 5bc767b
Try fixing tests
ErikDanielsson c48886e
Test with python 3.9
ErikDanielsson c898730
Remove legacy code
ErikDanielsson da0a94f
Move prompts to singularity
ErikDanielsson caeacce
Move singularity prompts into SingularityFetcher
ErikDanielsson 0afb74a
Merge branch 'dev' into download-refactor-again
ErikDanielsson 39af8e3
Merge branch 'move-singularity-code' into download-refactor-again
ErikDanielsson 36cc046
Move docker load message to DockerFetcher
ErikDanielsson bde85f9
Add test and test_full profiles
ErikDanielsson bb84582
Add nf inspect created dirs to gitignore
ErikDanielsson 0141a16
Update changelog
ErikDanielsson cbd54fa
Remove old test data
ErikDanielsson b444f1a
Remove unused file
ErikDanielsson 696230c
Add more docker tests
ErikDanielsson d6c4895
Apply suggestions from code review
ErikDanielsson d9b0107
Further changes from code review
ErikDanielsson eb46d9c
Update nf_core/pipelines/download/container_fetcher.py
ErikDanielsson 29506be
Update CLI info for docker containers
ErikDanielsson b3c0677
Apply suggestions from code review
ErikDanielsson 3686f23
ruff
ErikDanielsson f13b067
Typing, nicer UX (progress bars, better support for multiple revision…
ErikDanielsson af744e7
Update CHANGELOG.md
ErikDanielsson 934fd7c
Merge branch 'dev' into download-refactor-again
ErikDanielsson 7615309
Implement suggestions from @JulianFlesch
ErikDanielsson 06262d6
Use intermediate file in singularity pull
ErikDanielsson a19b2c6
Fix error due to misuse of `Path.with_suffix`
ErikDanielsson 1f9669d
More typing and some docs
ErikDanielsson 6cac4b3
Fix failing tests
ErikDanielsson 8e4033f
Merge branch 'dev' into download-refactor-again
ErikDanielsson ec703ef
Update message for nf version fail and propagate hide_progress correc…
ErikDanielsson be145e1
Make error messages nicer
ErikDanielsson 37d183d
Apply suggestions from code review
ErikDanielsson 1b5b2ea
Merge branch 'dev' into download-refactor-again
ErikDanielsson fe1da84
Revert --parallel to --parallel-downloads
ErikDanielsson 1876ccc
Merge branch 'dev' into download-refactor-again
ErikDanielsson 66513ac
Update CLI test with --parallel-downloads
ErikDanielsson ad2130c
Add mock for prompt
ErikDanielsson 006b19e
Mock prompt everywhere
ErikDanielsson 951bbe7
Make parent dir
ErikDanielsson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think, @mirpedrol and @mashehu are quite strict on renaming existing CLI arguments, since breaking changes would, according to SemVer, require a new major version release (4.0.0). But maybe Click would map a
--parallel-downloads
automatically to--parallel
, so it is backwards compatible?Apart from that, CLI parameters in download are mostly standardized in a way that the first letter of the last term is their short flag:
-d
/--parallel-downloads
,-s
/--container-system
,-i
/--container-cache-index
and others.What was the reason you wish to rename? As far as I remember, it used to be
-p
and--parallel
in 2.x.x and was specifically changed, because Seqera went from Tower to Platform, and we needed to free the short letter-p
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.
I think we can be sloppy enough to see a long name change of a parameter not as a breaking change, especially if it is just because of semantics 🙂
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.
I agree with this. I wouldn't change unless there is a good reason to.
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.
Fair. But particularly because there is a short flag, I also do not necessarily see the need to abbreviate the long form. But that is not a hill to die on for me, so have it as you like.
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.
Do as you see fit, I will also not fight anyone on this :)
FYI: The short flag
-t
as in tasks / threads, is also still available.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.
joining late to the party, but I would vote for keeping the long
--parallel-downloads
, otherwise I don't see where the short-d
comes from.But also not a strong request, so happy to accept the change if this is the preferred option.
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.
In the interest of not changing the API I think I agree with you and others @mirpedrol -- I'll switch back the change.
I think Julians suggestion with
--tasks/-t
is even better though, so if you agree you could perhaps put it on the pile of things you change before a 4.0.0 release (if there ever is one)?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.
I think we decided to not use
-t
to not confuse people with the previous option-t
fortower
, do I remember this well?We should consider if it has been long enough for people to not think about "tower" anymore
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.
At NGI, we continue calling it Tower, since Platform is simply a more cumbersome generic name.
But I doubt that many will still remember the old parameters - judged by the Slack discussions, very few people ever used that functionality anyway. So fine with me.
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.
I see -- slightly annoying then that platform/tower perfectly cover the two normal parallelism short flags... I've reverted the change now 👍