-
Notifications
You must be signed in to change notification settings - Fork 186
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
Java (multiple tools) -Xmx parameter #204
Conversation
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.
@tdayris , this is really cool. Thanks for taking the time to update all these recipes.
Your strategy to convert from Mb to Gb seems good, the -Xmx
parameter seems to work with powers of 1024
.
One general thing I would suggest, is using the resources
field in the snakemake rule
s, so that snakemake can directly use the arguments for restricting global memory usage (by setting --resources mem_mb=4096
should you only have 8G of RAM on your machine, for example) and requesting the amount of memory from a cluster system (as part of a qsub
command that uses resources.mem_mb
, for example). So instead of checking snakemake.params.get("extra", "")
, you would check for snakemake.resources.get("mem_mb", "")
.
Also, to showcase that users can set this parameter, I would always add it to the respective test/Snakefile
, because that is what's rendered as the example in the docs and users will usually copy-paste this. You could simply set this to something like:
# optional specification of memory usage of the JVM that snakemake will respect with global
# resource restrictions (https://snakemake.readthedocs.io/en/latest/snakefiles/rules.html#resources)
# and which can be used to request RAM during cluster job submission as `{resources.mem_mb}`:
# https://snakemake.readthedocs.io/en/latest/executing/cluster.html#job-properties
resources:
mem_mb=1024
Or if unsure about a good default value, you could comment the two resource specification lines out.
Another question---although I am not necessarily set on this---would be if we want to generally use mem_gb
instead of mem_mb
wherever the bioconda recipes allow it. For any of the tools that I have used, I usually deal in Gb
s of memory, rather than Mb
s. But if you disagree, feel free to keep it as is, with mem_mb
. Especially, as the snakemake docs example for --resources
deals in mem_mb
.
And finally, with a quick scan across the wrappers, I identified some extra ones that would benefit from your update -- I checked that their bioconda scripts would respect a setting of -Xmx
:
- jannovar
- snpeff
- trimmomatic
- varscan
However, this might still be an incomplete list. But the more we get done at once, the better. Also, should you need help with all the stuff I request, feel free to ping me in!
Hi, Many thanks for your review !
Edit: I forgot to cite #127 in this PR |
Thanks a lot! What a quick answer! I'll commit the changes before the end of the week. |
Hi, changes have been made, but I can't make through the tests since there is a very small bug in the snakemake-wrapper-utils |
Your PR is merged and a new release of the |
Hi, once again, thank you very much for your help with this work. Changes since last time:
Feel free to point out any mistake I have made |
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.
Very cool! Many thanks for the systematic update of Java memory management options and all the additions of docs links!
The only thing that could still be amended, would be adding resources: mem_mb = 1024
entries to the gatk3
wrappers:
- realignertargetcreator
- printreads
- baserecalibrator
- indelrealigner
But by now, these are kind of legacy wrappers (with gatk4 long released as the default). So it's maybe not necessary to update them. So just let me know if you want to add this (if so, note that I did one little commit adding a comment in the jannovar Snakefile, so pull you branch before changing it locally) or if I should just merge as is.
Thanks you, the resources information were added in GATK3. You are right. |
@tdayris Did you push this already? Because I can't see it here in the PR, yet. |
Wrong branch, my fault ! here are the modifications |
Cheers! And thanks again for this major effort! 🎉 |
Hi again,
As promised, all the memory additions in the previously discussed wrappers (#189 )
int()
function. If you know a better way to do it, I'll make the changes.generate_docs.py
still considered as 'new' despite being already accepted.Many thanks in advance for your reviews
Edit: merge conflict solved