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

feat: added support for micromamba frontend #2013

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

charmoniumQ
Copy link

Description

Updating #1889

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@charmoniumQ charmoniumQ changed the title Micromamba 2 feat: added support for micromamba frontend Dec 14, 2022
@charmoniumQ charmoniumQ reopened this Dec 15, 2022
@charmoniumQ charmoniumQ changed the title feat: added support for micromamba frontend feat: added support for micromamba frontend Dec 15, 2022
Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot! Just a few things below.
And may I ask you to format with black?

@@ -465,8 +465,12 @@ def create(self, dryrun=False):
)
env_archive = self.archive_file
try:
# Create empty env
if self.frontend == "micromamba":
(Path(env_path) / "conda-meta").mkdir(parents=True, exist_ok=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment here why this is needed? And if you consider this a bug in micromamba, add a todo for removal once the bug is fixed.

@sonarcloud
Copy link

sonarcloud bot commented Dec 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@charmoniumQ charmoniumQ marked this pull request as draft December 19, 2022 19:18
@pvandyken pvandyken linked an issue Dec 22, 2022 that may be closed by this pull request
@sonarcloud
Copy link

sonarcloud bot commented Jan 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester
Copy link
Contributor

@charmoniumQ can you please format the code with black? I would do it myself, but you configured your fork such that I don't have permissions to push changes to it.

@johanneskoester johanneskoester marked this pull request as ready for review January 23, 2023 15:16
@charmoniumQ
Copy link
Author

This should be a draft PR, because it doesn't completely work yet. Specifically mamba info --json has slightly different fields from conda info --json. I will run black on the code before I submit it for review.

@johanneskoester
Copy link
Contributor

I see. Thanks!

@charmoniumQ charmoniumQ marked this pull request as draft January 24, 2023 18:12
@johanneskoester
Copy link
Contributor

@charmoniumQ any chance to finalize this soon? Or shall I try to find some time to help out?

@charmoniumQ
Copy link
Author

Unfortunately, I haven't had a lot of time recently. I may have more time in two weeks. I would appreciate if you helped out.

Specify how conda info is read with micromamba
Work around micromamba not being able to create
environment in existing directory
@Austin-s-h
Copy link

This should be a draft PR, because it doesn't completely work yet. Specifically mamba info --json has slightly different fields from conda info --json. I will run black on the code before I submit it for review.

Also running into this issue when I tried to use micromamba in my CI as the conda provider.

I tried a workaround by setting an alias of conda=mamba, but Snakemake was still complaining the command
conda info --json that the conda executable wasn't found and the pipeline was crashed.

@maxim-h
Copy link
Contributor

maxim-h commented Apr 21, 2023

@johanneskoester
I've been making some small additions to this PR in the last couple of weeks and got it to a somewhat working condition, at least based on a couple of very basic tests.
https://github.com/maxim-h/snakemake/tree/micromamba-2
@charmoniumQ seems a little too busy to review those at the moment and merge with his tree.
I'm not sure whether it's best to open a new PR (there are already 2 open regarding the same thing: #1889 and #2013), or if you could just merge them with this PR.
Let me know if it's useful at all and how to proceed.

@charmoniumQ
Copy link
Author

Somehow I missed the notification. Sorry to keep you waiting @maxim-h

Now this PR reflects the tip of Maxim-H's work.

@charmoniumQ
Copy link
Author

I can't get Nose to work in Python 3.10, so I changed the docs/project_info/contributing.rst to refer to pytest instead.

But, with pytest, the tests pass on my machine except for some unrelated service/credential issues:

  • tests/test_google_lifesciences.py::test_github_issue1396
  • tests/test_google_lifesciences.py::test_github_issue1460
  • tests/test_slurm.py::test_slurm_mpi
  • tests/test_slurm.py::test_slurm_group_job
  • tests/test_slurm.py::test_slurm_complex
  • tests/test_tibanna.py::test_tibanna

@charmoniumQ charmoniumQ marked this pull request as ready for review April 21, 2023 17:37
@sonarcloud
Copy link

sonarcloud bot commented Apr 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@maxim-h
Copy link
Contributor

maxim-h commented Apr 21, 2023

@charmoniumQ awesome! No worries, we all have a day job after all)
I also noticed a problem with nose, hence the #2211. But your changes look good as well.
@johanneskoester feel free to ignore the #2211 if you merge this, since df92405 also introduces other useful changes.

@sparktx-ariel-hippen
Copy link

Hi, what is the status of this feature? I only have micromamba, not conda, installed on my machine and when I include the --use-conda parameter on my workflow, I get the following error:

Traceback (most recent call last):
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/__init__.py", line 790, in snakemake
    success = workflow.execute(
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/workflow.py", line 1078, in execute
    dag.create_conda_envs(
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/dag.py", line 350, in create_conda_envs
    env.create(dryrun)
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/deployment/conda.py", line 393, in create
    pin_file = self.pin_file
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/common/__init__.py", line 217, in __get__
    value = self.method(instance)
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/deployment/conda.py", line 103, in pin_file
    f".{self.conda.platform}.pin.txt"
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/common/__init__.py", line 217, in __get__
    value = self.method(instance)
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/deployment/conda.py", line 96, in conda
    return Conda(
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/deployment/conda.py", line 653, in __init__
    shell.check_output(self._get_cmd("conda info --json"), text=True)
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/site-packages/snakemake/shell.py", line 61, in check_output
    return sp.check_output(cmd, shell=True, executable=executable, **kwargs)
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/subprocess.py", line 420, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/ubuntu/micromamba/envs/sm803/lib/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command 'conda info --json' returned non-zero exit status 127.

@charmoniumQ
Copy link
Author

This feature seems to work for me and passes tests packaged with the source code.

I think we're just waiting on @johanneskoester to merge or give feedback.

@maxim-h
Copy link
Contributor

maxim-h commented Aug 17, 2023

@sparktx-ariel-hippen not sure what the issue is in your case.

A couple things to check:

  1. This PR is not merged, so make sure that you install snakemake from https://github.com/charmoniumQ/snakemake/tree/micromamba-2 if you want to use it.
  2. I don't think snakemake can automatically determine whether you're using micromamba. In addition to --use-conda make sure to set --conda-frontend micromamba

@baraaorabi
Copy link

Any update on this? Really appreciate the work you're doing 🙏🏻

@maxim-h
Copy link
Contributor

maxim-h commented May 13, 2024

I've had moderate success dogfooding the changes made here.
This feature is definitely not yet complete, although under the right circumstances it does works.

Some of the issues I've observed are:

  • Not recognizing conda base path sometimes (have to provide --conda-base-path manually)
  • Not being able to use an existing conda environment, only a yaml file definition. Also doesn't happen in all cases, but I don't know what exactly triggers it.
  • An issue with the api change in of Pulp requires manually restricting it's version to <2.8. This was already fixed in main, but these changes are now drifting away more and more from it.

And most glaring issue in my opinion is that I didn't manage to use this branch without using the testing environment (from test-environment.yml). My guess is that testing environment includes among other things conda and mamba, so in some cases it's able to fall back on those. This, of course, kinda goes against the whole point of switching to micromamba. And none of the tests can really show that these changes work on their own (they don't, at least not fully) unless they are ran outside of the testing environment with only micromamba present.

Unfortunately my python skills aren't good enough to push this all the way home. There are some things in the Workflow and the shell class that initialize Conda without specifying the frontend, but I still can't tell if that's the issue.

Copy link

sonarcloud bot commented May 13, 2024

Quality Gate Passed Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.4% Duplication on New Code

See analysis details on SonarCloud

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.

Add support for micromamba
7 participants