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

Add mem for all tools with cores and rescale some memory based on usegalaxy.org usage #76

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

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Nov 4, 2024

Some of this may be controversial so I'd certainly like to hear from other usegalaxy.* admins before this is merged. =)

fastqc is now scalable cores and memory based off input_size - probably I should fix this for compressed data a la my bwa-mem2 rule but perhaps TPV should have an estimated_input_size?

Data I used to make adjustments can be found in this gist. There is also a discussion about how to do a better job of this long term in #75.

Any tool I didn't have data for that has cores but no mem I left a comment on. fastp I don't know what to do with, since its usage is way outside of what I allocate for it, so I might try a more restricted query for that.

Copy link
Collaborator

@mira-miracoli mira-miracoli left a comment

Choose a reason for hiding this comment

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

Thanks a lot! this can save so much resources 🌳 🌍

@@ -2360,18 +2343,20 @@ tools:
mem: 64
toolshed.g2.bx.psu.edu/repos/iuc/seqtk/seqtk_seq/.*:
cores: 4
mem: 12
toolshed.g2.bx.psu.edu/repos/iuc/seurat/seurat/.*:
mem: 10
toolshed.g2.bx.psu.edu/repos/iuc/shasta/shasta/.*:
cores: 10
mem: 12
toolshed.g2.bx.psu.edu/repos/iuc/shovill/shovill/.*:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

My spades rule is way more conservative than yours, let me collect the stats real quick though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok spades stats are now in the gist. Input to memory ratio is 29x for the 95th percentile and 40x for the 99th percentile.

@@ -2309,7 +2291,8 @@ tools:
cores: 4
mem: 7.6
toolshed.g2.bx.psu.edu/repos/iuc/scanpy_remove_confounders/scanpy_remove_confounders/.*:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are not setting any cores / mem here so it should get the default
cores: 4
mem: 15.2

Copy link
Member Author

Choose a reason for hiding this comment

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

Your config doesn't override cores or mem unless I am missing it, so it would still have gotten the 9 cores (and then mem = 9 * 3.8 from your default rule) from the shared DB? I think your env vars restrict it to 4 cores but it's allocated 9?

tools.yml Outdated
toolshed.g2.bx.psu.edu/repos/iuc/pear/iuc_pear/.*:
cores: 7
mem: 58
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would add ~ 30 G mem for EU

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I might've not been thinking clearly when I wrote this. The 95th percentile of jobs use 3x input size and 99th use 14x input size, so I'll rewrite this as a scalable rule.

@@ -1412,6 +1380,7 @@ tools:
mem: 10
toolshed.g2.bx.psu.edu/repos/iuc/hyphy_prime/hyphy_prime/.*:
cores: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow that saves memory!

@@ -1372,6 +1339,7 @@ tools:
mem: 122.8
toolshed.g2.bx.psu.edu/repos/iuc/gubbins/gubbins/.*:
cores: 16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice mem reduction!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. gubbins uses very little memory

@@ -492,82 +505,15 @@ tools:
mem: 40
toolshed.g2.bx.psu.edu/repos/devteam/ncbi_blast_plus/ncbi_tblastx_wrapper/.*:
cores: 16
mem: 64
toolshed.g2.bx.psu.edu/repos/devteam/picard/.*:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't find any picard tool in your gist, deleting them would mean a mem increase for the 12G picard tools to ~15.2G and I am not sure if we need the JAVA_OPTIONS or if they are set in the wrapper now

Copy link
Member Author

Choose a reason for hiding this comment

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

This should make all picard tools 12G by matching just the owner/repo combo of toolshed.g2.bx.psu.edu/repos/devteam/picard/.*. This entry already existed with mem: 12 whereas all the specific tool entries I removed also set mem: 12, so they were just redundant.

The tools definitely need -Xmx set, although some local TPV configs may already do this (I set it in defaults for all my tools, regardless of whether they are java, because it's harmless if they're not).

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tools definitely need -Xmx set, although some local TPV configs may already do this (I set it in defaults for all my tools, regardless of whether they are java, because it's harmless if they're not).

This is good to know, thank you :)

Comment on lines 649 to 656
toolshed.g2.bx.psu.edu/repos/galaxyp/maxquant/maxquant/.*:
cores: 16
mem: 64
toolshed.g2.bx.psu.edu/repos/galaxyp/maxquant/maxquant_mqpar/.*:
cores: 16
mem: 64
toolshed.g2.bx.psu.edu/repos/galaxyp/maxquant_mqpar/maxquant_mqpar/.*:
cores: 16
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't change much for EU :)

@@ -1133,19 +1088,24 @@ tools:
mem: 12
env:
_JAVA_OPTIONS: -Xmx{int(mem)}G -Xms1G
toolshed.g2.bx.psu.edu/repos/iuc/extract_genomic_dna/Extract genomic DNA 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice to know that you can use spaces like this in tool names :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it works although it's not something we should be doing. This is a veeeeeeeeeery old tool.

cores: 3
toolshed.g2.bx.psu.edu/repos/iuc/bbtools_callvariants/bbtools_callvariants/.*:
mem: 15
env:
_JAVA_OPTIONS: -Xmx{int(mem)}G -Xms1G
toolshed.g2.bx.psu.edu/repos/iuc/bcftools_mpileup/bcftools_mpileup/.*:
cores: 7
mem: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

That saves quite a lot 👍🏼

@@ -1208,10 +1171,10 @@ tools:
mem: 100
toolshed.g2.bx.psu.edu/repos/iuc/chira_map/chira_map/.*:
cores: 10
mem: 80
mem: 24
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice find! 🚀

Copy link
Collaborator

@mira-miracoli mira-miracoli left a comment

Choose a reason for hiding this comment

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

Please have a look on line 1241 before merging, I think there is a value missing

@@ -1428,12 +1395,14 @@ tools:
mem: 24
toolshed.g2.bx.psu.edu/repos/iuc/iqtree/iqtree/.*:
cores: 10
mem: 7
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is low setting for mem. Max memory for Galaxy Australia's iqtree jobs is regularly in the 8-19Gb range (19 is the what we give it in TPV)

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.

3 participants