Skip to content

Use @ADDMEMORY@ / @ADDTHREADS@ macros appropriately across samtools c…#7902

Open
wm75 wants to merge 2 commits intogalaxyproject:mainfrom
wm75:fix-memory-for-samtools-sort
Open

Use @ADDMEMORY@ / @ADDTHREADS@ macros appropriately across samtools c…#7902
wm75 wants to merge 2 commits intogalaxyproject:mainfrom
wm75:fix-memory-for-samtools-sort

Conversation

@wm75
Copy link
Copy Markdown
Member

@wm75 wm75 commented Apr 17, 2026

@wm75
Copy link
Copy Markdown
Member Author

wm75 commented Apr 17, 2026

For the record:
there are more tools outside the samtools collection of tools that could benefit from using those TOKENS, too.

The tool that stands out as requiring fixing is arriba, which hard-codes memory usage of samtools sort thread, but determines threads from GALAXY_SLOTS.

| samtools fixmate -@ \$addthreads -u - -
| samtools sort -@ \$addthreads -m \${GALAXY_MEMORY_MB:-768}M -T "\${TMPDIR:-.}" -o '${output_bam}'
| samtools fixmate -@ \$addthreads -u - -
| samtools sort -@ \$addthreads -m \$addmemory"M" -T "\${TMPDIR:-.}" -o '${output_bam}'
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please note that here the change improves things, but the entire pipe using multiple threads at each step seems questionable.
@bernt-matthias maybe you have an idea how to fix this properly?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are ampliconclip, collate and fixmate compute intense tasks that need a lot of memory / CPUs?

Maybe just remove the calls of collate, fixmate and sort completelty. It's not documented that these tools run anyway. If we would want to fix all problems that are potentially introduced by ampliconclip the docs suggest that we also should call calmd`... Let the user do this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately, I still have never used the tool.
I agree with your points, but then lets do this separately. I will try to understand what's required next week and then open a new PR with those changes.

@wm75 wm75 added the skip-version-check Allow IUC members to skip the version linter in PR reviews (use only for partially updated suites). label Apr 17, 2026
@wm75
Copy link
Copy Markdown
Member Author

wm75 commented Apr 17, 2026

added the skip-version-check label just for samtools sort with the cosmetic indentation change. All other changed tools should have version bumps now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-version-check Allow IUC members to skip the version linter in PR reviews (use only for partially updated suites).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants