-
Notifications
You must be signed in to change notification settings - Fork 644
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
Add support for using mamba with conda #2030
Add support for using mamba with conda #2030
Conversation
f99e9fd
to
091c5ad
Compare
Is mamba cli compatible with conda cli? In that case would not make sense just to use it in place of nextflow/modules/nextflow/src/main/groovy/nextflow/conda/CondaCache.groovy Lines 246 to 254 in 914d7cb
|
Yes Paolo, I've put forward a couple of approaches to adding Please let me know which approach makes more sense to be used for the integration :) |
It seems that Also note that However,
|
Yes, the direction we have decided to take is to implement it as a Without the process foo {
mamba 'bwa samtools multiqc', micromamba: true
'''
your_command --here
'''
}
Thoughts? |
I think that is a great solution 👍 |
I'm not convived by this approach that's introducing a new mamba specific feature and al related options and vars eg Since mamba is a drop-in replacement for Conda as a user I would expect to be able to use the usual Conda option and just use Mamba binary in place of conda behind the scene. I think we need to introce a |
Yep, I agree. If possible, recycling the current Conda parameters and env variables would be much cleaner with the hope that maybe when this stabilises a little we can maybe just set It also means we don't need to add more syntactic clutter to module files here specifically for |
Yes, agree all apart from having |
Given how slow Conda is I think you would have to have way too much time on your hands not to opt in to using mamba by default 😅 |
Cool, I'll refactor accordingly 👍 |
a4adb89
to
43a9a70
Compare
Okay so I've refactored the PR code, to rely upon the One place I need some guidance is how should I accomodate this option within the nextflow/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy Line 345 in 73899f3
Thoughts? |
modules/nextflow/src/main/groovy/nextflow/conda/CondaOptions.groovy
Outdated
Show resolved
Hide resolved
f021153
to
6bc4b4b
Compare
Update: Able to create the environments using conda {
useMamba = true
} Please let me know your thoughts on the current solution. |
modules/nextflow/src/main/groovy/nextflow/conda/CondaCache.groovy
Outdated
Show resolved
Hide resolved
@@ -209,20 +215,20 @@ class CondaCache { | |||
* @return the conda environment prefix {@link Path} | |||
*/ | |||
@PackageScope | |||
Path createLocalCondaEnv(String condaEnv) { | |||
Path createLocalCondaEnv(String condaEnv, String binaryName = "conda") { |
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.
Can we keep binaryName
as a class attribute instead of passing as argument? or both conda
and mamba
can be used in the same class 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.
Can we keep binaryName as a class attribute instead of passing as argument?
Hmm, do you think that it's better to do use the following at class level
private useMamba = false
private binaryName = useMamba ? "mamba" : "conda"
and then simply use the binaryName
within the methods instead of argument?
or both conda and mamba can be used in the same class instance?
I am not sure how to interpret that, but from what I understand, it's enough to rely on mamba (i.e. a single binary) CLI since it'll automatically locate conda
if needed, without accomodating this in the Groovy code.
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.
Hmm, do you think that it's better to do use the following at class level
Let's have a binaryName
default to conda
and switch to mamba
when the flag is enabled
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.
Okay, I have accomodated the change request :)
@PackageScope String binaryName() { | ||
useMamba ? "mamba" : "conda" | ||
} | ||
|
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.
Renaming this to getBinaryName
it becomes a synthetic attribute that you can access as binaryName
instead of binaryName()
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.
Interesting - thanks for the tip 😊
@@ -60,6 +60,8 @@ class CondaCache { | |||
|
|||
private String createOptions | |||
|
|||
private Boolean useMamba = 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.
Minor, null is useless in this case better private boolean useMamba
(false is default)
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.
Hmm, good to know!
I've refactored accordingly.
@@ -90,6 +96,10 @@ class CondaCache { | |||
|
|||
if( config.cacheDir ) | |||
configCacheDir0 = (config.cacheDir as Path).toAbsolutePath() | |||
|
|||
if( config.useMamba ) | |||
useMamba = config.useMamba as Boolean |
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.
boolean
instead of Boolean
to match the type declaration
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.
Copy that 👍
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.
Ok, we are almost there. Have tried it? also it should be added a note in the docs for conda, explaining this functionality, the benefits, how to use it and that's an experimental feature.
Please also sign the commit to make the DCO both green. See the contribution readme for details.
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
3ced9ed
to
4dd48a3
Compare
Okay, so here's the current status
Summary: The
Details: On my humble local machine, the results without
The result with
|
modules/nextflow/src/main/groovy/nextflow/conda/CondaCache.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
Signed-off-by: Abhinav Sharma <abhi18av@gmail.com>
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.
Awesome! nice contribution @abhi18av!
Initiating the work for
mamba
support in conda.Planned features are
Amamba
directive withmicromamba
option to users to usemicromamba
which doesn't rely on a base environment - see hereWe have decided to implement this as a config setting for
conda
scope.