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

refactor: unify snakemake versions between envs #16

Merged
merged 9 commits into from
Oct 14, 2021
Merged

Conversation

AngryMaciek
Copy link
Member

Update older snakemake version for the root-dedicated conda env.

Fixes #10

@AngryMaciek AngryMaciek added bug Something isn't working good first issue Good for newcomers labels Sep 30, 2021
@AngryMaciek AngryMaciek self-assigned this Sep 30, 2021
@AngryMaciek AngryMaciek added this to In progress in v0.3.0 via automation Sep 30, 2021
@AngryMaciek AngryMaciek removed the request for review from fgypas September 30, 2021 17:50
@AngryMaciek
Copy link
Member Author

As mentioned in #11, the GH Action for miniconda had some random problems with the installation. I have fixed that in this PR by removing the miniconda version fix in the CI workflow. It will always download the most recent one, which is good.

I updated the snakemake version in the conda env but then it required mamba too for package management within the zarp workflow (for automatic env generation). So I have added mamba as a dependency and let it run.
Now it seems that the output files while running with mamba envs do not match the expected output, but only for one tool: cutadapt:

results/multiqc_summary/multiqc_data/multiqc_cutadapt.txt: FAILED
results/multiqc_summary/multiqc_data/multiqc_cutadapt_1.txt: FAILED

This would suggest we need to use different expected output for conda and singularity executions<?>
What do you think @fgypas?

@AngryMaciek
Copy link
Member Author

What I described above (#16 (comment)) is a separate issue, linked to conda->mamba improvement. I have now fixed the configuration so that we always use conda frontend, as we have always been.
That way the initial problem of snakemake versions inconsistency is resolved!
I have opened an additional issue related to upgrade to mamba: #18

@fgypas
Copy link
Member

fgypas commented Oct 6, 2021

Sorry @AngryMaciek I did not see the notifications earlier. I will have a look at the open PRs soon.

uniqueg
uniqueg previously approved these changes Oct 7, 2021
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Based on the points mentioned in the discussion, these changes look reasonable to me. Given that the tests pass, I would tend to say let's merge this. But I'm not an expert on Conda, Singularity or Snakemake execution, so I think we should wait for @fgypas review as well

@AngryMaciek
Copy link
Member Author

@fgypas : could you plese review the change?

install/environment.root.yml Outdated Show resolved Hide resolved
install/environment.yml Outdated Show resolved Hide resolved
install/environment.yml Show resolved Hide resolved
profiles/local-conda/config.yaml Show resolved Hide resolved
Copy link
Member

@fgypas fgypas left a comment

Choose a reason for hiding this comment

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

Did you also test the cluster execution with the current version?

@AngryMaciek
Copy link
Member Author

Did you also test the cluster execution with the current version?

Yes, the outcome is the same as for local tests.

@AngryMaciek AngryMaciek merged commit ec29ecd into dev Oct 14, 2021
v0.3.0 automation moved this from In progress to Done Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

same snakemake version for normal and root users?
3 participants