Skip to content

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.

Sign up for GitHub

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

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

Add memory flag to metaSPAdes for better RAM handeling #7363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef89572
Add memory flag for better RAM handeling to metaSPAdes
SantaMcCloud Oct 15, 2025
9fc4d01
bump version
SantaMcCloud Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/spades/macros.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<macros>
<token name="@TOOL_VERSION@">4.2.0</token>
<token name="@VERSION_SUFFIX@">0</token>
<token name="@VERSION_SUFFIX@">1</token>
<xml name="requirements">
<requirements>
<requirement type="package" version="@TOOL_VERSION@">spades</requirement>
Expand Down
3 changes: 3 additions & 0 deletions tools/spades/metaspades.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
## run
metaspades.py
-o 'output'
--memory ${memory}
@RESOURCES@
@INPUT_READS_MAIN@
#if $additional_reads.selector == 'true'
Expand All @@ -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"/>
Copy link
Contributor

@SaimMomin12 SaimMomin12 Oct 15, 2025 •
edited
Loading

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

Copy link
Contributor Author

@SantaMcCloud SantaMcCloud Oct 15, 2025

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.

Copy link
Contributor

@bernt-matthias bernt-matthias Oct 15, 2025

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))

Copy link
Contributor

@paulzierep paulzierep Oct 16, 2025

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.

Copy link
Contributor

@paulzierep paulzierep Oct 16, 2025

Choose a reason for hiding this comment

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

FYI: ablab/spades#1535

Copy link
Contributor

@paulzierep paulzierep Oct 18, 2025

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 :)

Copy link
Contributor

@bernt-matthias bernt-matthias Oct 20, 2025

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.

Copy link
Contributor Author

@SantaMcCloud SantaMcCloud Oct 20, 2025 •
edited
Loading

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

Copy link
Contributor Author

@SantaMcCloud SantaMcCloud Oct 20, 2025

Choose a reason for hiding this comment

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

ablab/spades#1536

Copy link
Member

@bgruening bgruening Oct 20, 2025

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.

</inputs>
<outputs>
<expand macro="out_ag"/>
Expand All @@ -69,6 +71,7 @@ metaspades.py
<param name="sPaired" value="paired_interlaced"/>
<param name="input1" value="ecoli_1K.fastq.gz"/>
</conditional>
<param name="memory" value="64"/>
<output name="out_ag">
<assert_contents>
<has_n_lines n="36"/>
Expand Down