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

Added options to customise cpu and memory used per multicore for bismark align #126

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

ewels
Copy link
Member

@ewels ewels commented Nov 26, 2019

Closes #124

Note that this PR will hopefully build with the new docker image created in PR #123. This should be considered when checking the CI tests..

@ewels ewels requested review from alneberg, phue and a team November 26, 2019 10:25
@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

Sigh, the docker build failed: https://cloud.docker.com/u/nfcore/repository/docker/nfcore/methylseq/builds/dce2a365-d736-4d56-a467-399b8e15ac36

UnsatisfiableError: The following specifications were found to be in conflict:
- bioconda::hisat2=2.1.0 -> python[version='>=2.7,<2.8.0a0']
- python=3.7
Use "conda search <package> --info" to see the dependencies for each package.

@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

https://bioconda.github.io/contributor/guidelines.html states that:

hisat2 runs 2to3 to make it Python 3 compatible, and copies over individual scripts to the bin dir

Sure enough, the build recipe does this: https://github.com/bioconda/bioconda-recipes/blob/90620569f00fa786b85d272507445a229748272d/recipes/hisat2/build.sh#L5-L20

However, the meta file then skips python 3k builds: https://github.com/bioconda/bioconda-recipes/blob/90620569f00fa786b85d272507445a229748272d/recipes/hisat2/meta.yaml#L13 🤦‍♂

@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

x-ref: bioconda/bioconda-recipes#18911

@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

Reverted MultiQC back to version 1.7 and removed the Python 3 dependency. Hopefully everything will start working again then 🤞

Still need to come back to the Python 3 thing at a later date, as future versions of MultiQC will drop py2k support. But we need to release soon to fix the iGenomes bug, so that can wait.

@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

Again, needs to be merged and a new PR opened to test that the reverted MultiQC build works..

Copy link
Member

@alneberg alneberg left a comment

Choose a reason for hiding this comment

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

Only some slight comment on the docs. Personally I find the bismark parameters impossible to understand and I wouldn't possibly try to mixture with them, but I guess someone out there might be able to.

docs/usage.md Outdated
The pipeline makes use of the `--multicore` option for Bismark align. When using this option,
Bismark uses a large number of CPUs for every `--multicore` specified. The pipeline
calculates the number of `--multicore` based on the resources available to the task.
It divides the available CPUs by 3, or by 5 if `--single_cell`, `--zymo` or `--non_directional`
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean 3 if either of the two first and 5 if --non_directional is specified? It's a bit ambiguous the way it's currently written

Copy link
Member Author

Choose a reason for hiding this comment

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

3 unless any of those three are specified..

Copy link
Member Author

Choose a reason for hiding this comment

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

But yes I agree, this was not clear 👍 Hopefully improved now..

Copy link
Member

Choose a reason for hiding this comment

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

Yes, much better.

@ewels
Copy link
Member Author

ewels commented Nov 26, 2019

Tests look fine apart from MultiQC breaking..

@ewels ewels merged commit 2a93a1f into nf-core:dev Nov 26, 2019
@ewels ewels deleted the config-cpu_per_multicore branch November 26, 2019 15:55
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.

2 participants