-
Notifications
You must be signed in to change notification settings - Fork 759
Support conda lock files with any extension and remote URLs #6665
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
base: master
Are you sure you want to change the base?
Conversation
This change allows conda lock files to be used with any file extension (e.g., .lock, .txt, or no extension) as long as they contain the @explicit marker in the first 20 lines. Additionally, remote conda lock files can now be specified using http/https URLs. Changes: - Add isLockFile() method to detect lock files by @explicit marker - Add isRemoteFile() method to check for http/https URLs - Add stageRemoteFile() method to download remote files locally - Modify condaPrefixPath() to handle remote files and lock files with any extension - Modify createLocalCondaEnv0() to detect and handle lock files properly - Add tests for new functionality - Update documentation to reflect new behavior Fixes #5583 Signed-off-by: Claude <[email protected]>
✅ Deploy Preview for nextflow-docs-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Document that the conda lock file detection behavior changed in 26.01.0-edge to support any file extension and remote URLs. Signed-off-by: Claude <[email protected]>
Refactor isRemoteFile() to use the existing FileHelper.getUrlProtocol() method instead of hardcoded http/https checks. This adds support for all remote protocols including S3, Google Cloud Storage, Azure Blob Storage, and FTP. Update tests and documentation to reflect the expanded protocol support. Signed-off-by: Claude <[email protected]>
Remove the isYamlUriPath method which is now redundant since isRemoteFile uses FileHelper.getUrlProtocol() to detect all remote protocols. Update tests to reflect the new behavior where remote files are staged locally before being passed to conda/mamba/micromamba. Add tests for remote lock files with mamba and micromamba. Signed-off-by: Claude <[email protected]>
Fix Spock interaction expectations in tests for remote YAML/lock files: - Remove redundant isRemoteFile stub when getLocalFilePath is stubbed (isRemoteFile is called internally by getLocalFilePath) - YAML files ending in .yml enter the YAML branch via isYamlFilePath, not the remote file branch - Add clarifying comments about test behavior Signed-off-by: Claude <[email protected]>
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.
Docs look good. One minor suggestion. Someone else will need to check the code.
Co-authored-by: Chris Hakkaart <[email protected]> Signed-off-by: Phil Ewels <[email protected]>
Signed-off-by: Phil Ewels <[email protected]>
|
It looks like this PR tries to do too many things (parsing content, formatting env (?), supporting remote uris, etc), please scope clearly the goals. |
|
Tested successfully in GitHub codespaces, working as expected:
|
You basically described the goals 😅 - though I don't think it does any formatting? Goals:
|
|
I updated the PR description to try to make this clearer @pditommaso. I'm not really sure how I can reduce the scope of the code changes without missing the goals. Suggestions welcome! |
|
I think the scope here is appropriate. The two goals are inherently linked - remote conda lock files (like Wave's The implementation is clean: three small helper methods ( This unblocks a real user pain point from #5583 - conda lock files are increasingly common in nf-core and other pipelines, and the current |
pditommaso
left a comment
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.
A part the comment on performance, not quite convinced nextflow should download remote lock files on-the-file. Lock files should be considered an assert of the module bundle (and eventually downloaded with the bundle itself)
| final lines = content.readLines() | ||
| final limit = Math.min(20, lines.size()) | ||
| for( int i = 0; i < limit; i++ ) { | ||
| if( lines[i].trim() == '@EXPLICIT' ) | ||
| return true | ||
| } | ||
| return false |
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.
This should use a reader (ie. read line by line) to avoid loading all file in memory to parse the header
From #5583 (comment):
PR Goals:
Details
This change allows conda lock files to be used with any file extension (e.g., .lock, .txt, or no extension) as long as they contain the `@EXPLICIT` marker in the first 20 lines. Additionally, remote conda lock files can now be specified using http/https URLs.Changes:
@EXPLICITmarkerFixes #5583