fix: check if miniconda is present before installation, closes #47 #48
Conversation
📝 WalkthroughWalkthroughAdded a pre-check step that detects an existing Miniconda installation and sets an output; the conda setup step now runs only when that output is Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2024-12-09T10:52:48.855ZApplied to files:
📚 Learning: 2024-12-09T16:41:37.740ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tedil
Repo: snakemake/snakemake-github-action PR: 39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
action.yml (1)
76-85: Consider a more robust check for a valid Miniconda installation.While checking for directory existence is an improvement over the previous
.condarcapproach, it doesn't verify that the installation is valid or functional. A partially failed installation could leave the directory in a corrupt state, causing subsequent steps to fail silently.For better robustness, consider checking for the conda executable itself:
if [ -x "/home/runner/miniconda3/bin/conda" ]; then echo "installed=true" >> $GITHUB_OUTPUT else echo "installed=false" >> $GITHUB_OUTPUT fiAlternatively, if the setup-miniconda action sets the
$CONDAenvironment variable on successful installation, you could check that. However, the directory-only check does address the core issue from #47 (preventing re-installation and the symlink error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-09T10:52:48.855Z
Learnt from: tedil
Repo: snakemake/snakemake-github-action PR: 39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
Applied to files:
action.yml
🔇 Additional comments (1)
action.yml (1)
86-95: No issues found —installation-diris a supported parameter in setup-miniconda@v3.The
installation-dirinput is officially supported and documented in the action's v3.x releases. The conditional logic and parameter usage in lines 86–95 are correct.
ezherman
left a comment
ezherman
left a comment
There was a problem hiding this comment.
Thanks @m-jahn! See my thoughts below on the changes & let me know what you think.
Also, qould it be sensible to add a second run step, and/or a preceding setup-miniconda step, to https://github.com/snakemake/snakemake-github-action/blob/master/.github/workflows/main.yml to ensure we test the feature keeps working going forward?
action.yml
Outdated
| id: check-miniconda | ||
| shell: bash -el {0} | ||
| run: | | ||
| if [ -d "/home/runner/miniconda3" ]; then |
There was a problem hiding this comment.
A self‑hosted runner may use a different home path. What about $HOME/miniconda3?
There was a problem hiding this comment.
Or to check more generally for a conda installation, e.g. which conda
There was a problem hiding this comment.
good points. I like the checking for $HOME, if this env var is available on the github runner, it's fine with me.
Just checking with which conda is dangerous though, as the test would succeed for any conda available, not necessarily our one with snakemake.
There was a problem hiding this comment.
OK so the problem is that $HOME is not available in statements like installation-dir: /home/runner/miniconda3, only in the run: directive of a step. So it would be more complicated to do this, then what we have now.
There was a problem hiding this comment.
Why do we need to specify installation-dir? I believe it already defaults to $HOME/miniconda3
| fi | ||
| - name: Setup conda | ||
| if: steps.check-miniconda.outputs.installed == 'false' |
There was a problem hiding this comment.
If the setup step is skipped, conda won't be set up with .snakemake.environment.yaml as the default environment. Will snakemake then not be available as a consequence? If so, I imagine we need an additional step that installs snakemake into the base environment, or creates a snakemake conda environment, if conda is already available.
There was a problem hiding this comment.
the idea is that this check is specific for the miniconda setup step. when you run this step twice, as we do in the standard snakemake testing workflow, the env is already present and checked out from the first step, including snakemake. the test in this repo succeeded, meaning it works as expected. that's why I would also not test for any conda installation, as this could be the wrong one, only for the specific miniconda3 one.
There was a problem hiding this comment.
Gotcha yes, I see the containerize step would trigger a second setup miniconda step so there isn't a need for a second run step.
Regarding testing for miniconda3: if the workflow includes setup-miniconda upstream, then I believe the current implementation would not install snakemake. Or am I missing something? For example, if setup-miniconda is used to enable pytest:
Testing:
runs-on: ubuntu-latest
needs:
- Linting
- Formatting
steps:
- uses: actions/checkout@v4
- uses: conda-incubator/setup-miniconda@v3
with:
miniforge-version: latest
use-mamba: true
environment-file: .test/envs/test.yaml
- name: Test workflow python functions with pytest
shell: bash -el {0}
run: |
pytest workflow/tests/*.py
- name: Test workflow
uses: snakemake/snakemake-github-action@v2.0.0
with:
directory: .test
snakefile: workflow/Snakefile
args: "--use-conda --show-failed-logs --cores 2 --conda-cleanup-pkgs cache"
There was a problem hiding this comment.
Right. This is an edge case. A custom workflow with another setup-miniconda@v3 step would skip installing snakemake and fail. Is this a risk we can take (honest question)? In the end, the github action needs to work mainly for the test workflows, and it does so by wrapping several steps into a single one. In the example you posted, one would need to work around this by replacing the snakemake action with some selected steps.
There was a problem hiding this comment.
It's actually an important use-case, in my opinion. An extension is of this is to follow the snakemake-github-action with integration tests, which would not be possible if the action needs to be run in a separate job. Why limit the action to only operate in jobs that only use the snakemake github action?
What could go wrong by checking for a conda installation more generally, and updating/installing the snakemake environment if i conda exists?
There was a problem hiding this comment.
Hi Ezra, no rush I think this is probably going to be fixed upstream in miniconda, see David's other PR here. I've converted this PR to draft and we might close it without merging.
There was a problem hiding this comment.
as of 30 minutes ago our workflows are still failing at the test report step -- so no fix in sight...
There was a problem hiding this comment.
Sorry, I was out sick, so I only find time to follow up on this now.
It also still fails for the CI runs where I first noticed this. However, with one difference to the errors that I previously saw. Namely, these kinds of warnings (on the initial installation of miniconda3, which works) are gone:
Warning: warning libmamba Did not find a repodata record for https://conda.anaconda.org/conda-forge/linux-64/_libgcc_mutex-0.1-conda_forge.tar.bz2
So I think the constructor update is actually doing, what the original issue here was about:
conda/conda-libmamba-solver#798
But the re-installation error persists. So I think this might simply be a separate issue. Although I think we should not need to fix this here, but this is probably rather something where the miniforge installer should more gracefully handle existing installations -- as I guess it did before?
I'm looking into this now, hope to have some better idea where to fix this later. And if we get stuck here, I would opt for pinning to the previous working version for now (#49), so everyone can get on with their actual work, not waiting for CI.
action.yml
Outdated
| miniforge-version: latest | ||
| environment-file: .snakemake.environment.yaml | ||
| activate-environment: snakemake | ||
| installation-dir: /home/runner/miniconda3 |
There was a problem hiding this comment.
Here too, I believe $HOME/miniconda3 would be safer in case a self-hosted runner is used. Although is this parameter needed, which points to the default path, if we've already checked for a conda installation?
|
OK, I think I have really identified where this crept in and have suggested a fix here: I hope this does what I think it will and that it is a proper fix. Hopefully, someone will soon chime in over there. However, I think that it might take a moment until this propagates, as it needs to be merged and then released there, and then |
|
good job! that looks like it. I'm all for merging the temporary pin to previous miniconda, as this can easily be amended again. The alternative is to keep waiting for the upstream fix for an undefined amount of time. |
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.