feat(comebin/runcomebin): add GPU support, fix outputs and add a real test - #12796
Open
dialvarezs wants to merge 7 commits into
Open
feat(comebin/runcomebin): add GPU support, fix outputs and add a real test#12796dialvarezs wants to merge 7 commits into
dialvarezs wants to merge 7 commits into
Conversation
… test
Select the container and conda environment from `task.accelerator`, and pass
the resolved device to `run_comebin.sh` with `-d`. The module never passed
`-d`, so it used the tool's `cuda` default against a CPU-only pytorch build
and raised before training. Also emits the CUDA runtime version to the
versions topic.
Two further fixes that only show up on a real run:
- The declared `embeddings.tsv` and `covembeddings.tsv` outputs do not
exist. COMEBin 1.1.0 writes `embeddings.npy` and `covembeddings.npy`,
plus `embedding_ids.txt` giving the row order, which is now emitted too.
- `find ${prefix}/comebin_res_bins/*.fa -exec gzip {} \;` fails with
`find: command not found` under Singularity, because that image is based
on Amazon Linux 2023 while the Docker one is Ubuntu. The glob already
does the work, so this is now a plain `gzip`. Note the `find` gave no
protection against an empty glob either; both forms fail the same way.
Replaces the stub-only test with a real one over a two-genome community,
plus a GPU test tagged `gpu`. COMEBin has no default random seed and its
clustering also varies with the thread count, so the test config pins both.
Snapshots were confirmed stable across two runs on each path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI showed the bin contents differ between machines on both the CPU and the
GPU path. COMEBin's own log says why:
KMeans, NumPy, scikit-learn, BLAS, OpenMP, and other native runtime
pools are not controlled by --num_threads.
So pinning `-s 1` and `cpus` fixes COMEBin's own threading but not the
native pools underneath, which follow the host's core count. On the GPU
path the runner's T4 (sm_75) also gives different numerics than the sm_120
card the snapshot was recorded on.
Only the well-separated genome landed identically everywhere; the marginal
contigs moved between bins. So the snapshot now keeps the stable outputs
and the bin count assertion, mirroring what semibin/multieasybin already
does for the same reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The binning dataset is merged, so the tests use params.modules_testdata_base_path instead of raw URLs on a fork. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dialvarezs
marked this pull request as ready for review
August 26, 2026 07:11
The CPU test passed a single BAM, so the list staging into `bam/` was never exercised. It now passes all three, which is also the mode the dataset was built for. The GPU test keeps one sample, so both input shapes are covered. Runtime is unchanged: COMEBin's cost is dominated by training and CheckM, not by the number of coverage files. Also trims the code comments added earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
prototaxites
left a comment
prototaxites
left a comment
Contributor
There was a problem hiding this comment.
One small comment, but looks really good - thanks for doing this!
dialvarezs
commented
Aug 26, 2026
Co-authored-by: Diego Alvarez S. <dialvarezs@gmail.com>
prototaxites
left a comment
prototaxites
left a comment
Contributor
There was a problem hiding this comment.
OK, it looks good to me but I will leave approval to someone else with some knowledge of GPUs and their nf-core implementation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GPU support
Container and conda environment are selected from
task.accelerator, and the resolved device is passed torun_comebin.shwith-d. The CUDA runtime version goes to the versions topic.The GPU environment pins
pytorch-gpu=2.13.0withcuda-version=12.9, the lowest CUDA that pytorch 2.13 ships on linux-64.Fixes
The declared
embeddings.tsvandcovembeddings.tsvoutputs do not exist. COMEBin 1.1.0 writesembeddings.npyandcovembeddings.npy, plusembedding_ids.txtgiving the row order, which is now emitted too.Passing
-dis also needed on the CPU path since 1.1.0.run_comebin.shdefaults todevice=cuda, and 1.1.0 turned that into a hard error rather than the silent CPU fallback 1.0.4 did, so a real run on the current container raises before training:find ${prefix}/comebin_res_bins/*.fa -exec gzip {} \;fails under Singularity (nofindin the image).The glob already does the work, so this is now a plain
gzip.Tests
The stub-only test is replaced by a real one over a two-genome community, plus a GPU test tagged
gpu.Bin contents are not reproducible across machines. Pinning
-s 1andcpusfixes COMEBin's own threading, but not the BLAS and OpenMP pools underneath, which follow the host's core count. So the snapshot keeps the stable outputs and the bin count is checked by assertion, assemibin/multieasybinalready does for the same reason.PR checklist
topic: versionslabelnf-core modules test comebin/runcomebin --profile singularitynf-core modules test comebin/runcomebin --profile dockernf-core modules test comebin/runcomebin --profile conda