-
Notifications
You must be signed in to change notification settings - Fork 483
Add memory flag to metaSPAdes for better RAM handeling #7363
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -49,6 +50,7 @@ metaspades.py | |||
| <option value="--iontorrent">Iontorrent: required when assembling IonTorrent data (--iontorrent)</option> | |||
| </expand> | |||
| <expand macro="optional_output"/> | |||
| <param argument="--memory" type="integer" value="350" min="64" max="350" label="Set RAM limitation" help="When using a big input it is better to set a RAM limit to reduce the chance that metaSPAdes will get an error based on memory usage"/> | |||
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.
This is not a good idea to expose memory parameters to users. Instead this needs to be set up as a rule in TPV configs by admins. You can refer to some examples here https://github.com/usegalaxy-eu/infrastructure-playbook/blob/ee168ba226433d32f033663eaffb4e0a979fe694/files/galaxy/tpv/tools.yml
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.
This it a tool Limit such that certain hasmaps or so will be reduce to handle the the tool better.
@paulzierep can give you a better overview about it.
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.
I'm with @SaimMomin12. Also it is already set here
tools-iuc/tools/spades/macros.xml
Line 117 in 9fc4d01
| -m \$((\${GALAXY_MEMORY_MB:-8192}/1024)) |
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.
Ah I did not check weather the wrapper already got this flag. I this case we can drop this PR, question would be why the tool then still fails due to memory, maybe we should reduce it a bit to make sure its below the Galaxy provided memory.
Another question is whether this option changes the assembly logic, e.g. automatically use less kmers. If so we should reconsider the logic here, since that would mean, that TPV adjusts memory -> other -m paramter -> different assembly -> not reproducible. Will check with the devs.
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.
FYI: ablab/spades#1535
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.
allow user/workflow input of memory usage prediction (that could be picked up by TPV)
Thats another question entirely, but I would say like in this case, we cannot directly allow it, but maybe with some encoding or as file metadata ... still no idea tbh :)
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.
We should do that, maybe go for -10 to be sure and check if that works for some of our failed jobs.
-10GB seems quite a lot. Could we ask upstream for a reasonable value.
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.
I think at a point it was the default option to use only 90% but i can ask them if they have a good idea for this. But we can also have different ratio for it like only remove 1 GB from it which will act as a buffer
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.
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.
Provide me please with a patch, a single line that I need to add to the current wrapper on EU, and we can test it in real-life.
FOR CONTRIBUTOR: