Repository navigation
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ff56f5a to
5504259
Compare
…zeOutput snapshots Same adjustments as requested in the tmalign review (nf-core#13065): - Emit colabfold_batch's own files (pdb, scores json, a3m, png, log) instead of copying rank_001 models in a loop and computing a custom pLDDT table; per-residue pLDDT is already in the scores json and the PDB B-factor column. - Weights input is now a tuple with meta2. - Remove the header comment block and pipeline-specific workarounds (query-name trimming, unified-memory env, CUDA-image cache path); Amber relaxation is now opt-in through ext.args. - Tests snapshot sanitizeOutput(process.out). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
- Blocking: the container is still a legacy biocontainer; please port it to a Seqera Wave build with
nf-core modules containers create colabfold(E1). - Blocking: the only non-stub test runs on an empty FASTA, so the tool is never executed and the weights branch is untested; the test name also needs the
<organism> - <inputs> - <outputs>form (T10, T5). - Smaller points:
meta.ymlneedsargs_id, EDAM ontologies and "Optional" onweights; tests should assertprocess.successbeforeassertAll. - Module has the
process_gpulabel: please also check it against the GPU module docs (GPU test config and tag,ext.use_gpuhandling), since the CI GPU jobs were skipped.
Gates: lint, nf-test (conda, docker, singularity) and nextflow lint all pass.
Diff 8.18/10 · Overall 8.18/10
nf-core-reviewer 0.3.0 / vagkaratzas-review (rubric v2) · claude-sonnet-5-5
| container "${workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container | ||
| ? 'https://depot.galaxyproject.org/singularity/colabfold:1.5.5--pyh7cba7a3_2' | ||
| : 'quay.io/biocontainers/colabfold:1.5.5--pyh7cba7a3_2'}" |
There was a problem hiding this comment.
The container still points at depot.galaxyproject.org / quay.io/biocontainers. Please build a Seqera Wave container with nf-core modules containers create colabfold, which updates this directive, adds the containers: block to meta.yml and the .conda-lock/ files.
There was a problem hiding this comment.
Done in a260a90. The CPU container was built with nf-core modules containers create colabfold, which also added the containers: block and the .conda-lock/ file. The linux/arm64 builds failed twice on the Wave side (an S3 AccessDenied on the arm64 builder, then exit 137), so only linux/amd64 is listed for now; I can rerun the command once those builds go through.
There is also a GPU variant (see the general comment): environment.gpu.yml, built with Wave using --build-template conda/micromamba:v2 and selected with task.accelerator, following the dual-container pattern from the GPU modules guide.
| // A real prediction needs the ~4 GB AlphaFold2 weights, which no CI runner | ||
| // has. What can run for real is the empty-input path, where the module | ||
| // must succeed and emit no structure. | ||
| test("empty fasta") { |
There was a problem hiding this comment.
Test name should follow <organism> - <inputs> - <outputs/config>, e.g. proteomics - empty faa - no structures.
More importantly, this only exercises the empty-input shortcut: colabfold_batch is never run, and neither the --data ${weights} branch nor the no-weights branch is covered. Please add a test that runs the tool for real (a tiny sequence with weights staged via meta2, or a documented way to make it fit in CI), or at least cover the weights-given branch.
There was a problem hiding this comment.
Renamed to proteomics - empty faa - no structures (and proteomics - faa - stub).
For a real run, I added tests/main.gpu.nf.test (tag gpu, nextflow.gpu.config sets accelerator = 1). It runs a single-sequence prediction of proteomics/interproscan/l_arginase.faa (--msa-mode single_sequence --num-models 1 --num-recycle 1, so no MSA server is queried) and asserts that a PDB with ATOM records is produced. Locally it passes in ~7 min including the weights download; the prediction itself takes ~40 s on a GPU.
The real test is GPU-only because on CPU the same prediction takes 15 min on 4 cores, after a ~3.5 GB weights download, and would run three times (conda, docker, singularity).
On the weights: the test passes [], which exercises the download branch. The --data branch can't be covered in CI because the AlphaFold2 parameters can't go into nf-core/test-datasets (a single model is >350 MB, above GitHub's 100 MB file limit). It is the same colabfold_batch call with --data <dir> added; I checked it locally with a real prediction against a local weights directory.
Running this for real also caught two bugs that the empty-input and stub tests could not see: a fresh solve of colabfold 1.5.5 pulled dm-haiku 0.0.17, which crashes at the first prediction with jax 0.4.25 (now pinned to 0.0.12 + flax 0.8.2, as in the old biocontainer), and the CUDA 11.8 jaxlib build has no ptxas, so ColabFold skipped every query while still exiting 0 (hence CUDA 12.0).
| } | ||
|
|
||
| then { | ||
| assertAll( |
There was a problem hiding this comment.
Put a bare assert process.success first, then assertAll(...) holding the snapshot, as in the nf-core template.
There was a problem hiding this comment.
Done in a260a90, here and in the GPU tests.
| } | ||
|
|
||
| then { | ||
| assertAll( |
There was a problem hiding this comment.
Same here: bare assert process.success before assertAll(...).
| documentation: https://github.com/sokrypton/ColabFold | ||
| doi: 10.1038/s41592-022-01488-1 | ||
| licence: ["MIT"] | ||
| identifier: biotools:colabfold |
There was a problem hiding this comment.
Add the args_id for the tool, matching the $args variable in main.nf.
| identifier: biotools:colabfold | |
| identifier: biotools:colabfold | |
| args_id: "$args" |
(Y4)
| - weights: | ||
| type: directory | ||
| description: | | ||
| ColabFold data directory containing the AlphaFold2 parameters | ||
| (`params/`). Pass `[]` to let colabfold_batch download them into its | ||
| own cache on first use. | ||
|
|
There was a problem hiding this comment.
weights is optional ([] is accepted), so the description should start with "Optional". Descriptions should also be a single short line.
There was a problem hiding this comment.
Done in a260a90: the description now starts with "Optional" and is a single line, and the other input/output descriptions are shortened too.
| - fasta: | ||
| type: file | ||
| description: | | ||
| Protein sequences to fold, one prediction per record. An empty file | ||
| is accepted and produces no output. | ||
| pattern: "*.{fa,faa,fasta}" |
There was a problem hiding this comment.
Input and output file entries are missing EDAM ontologies (fasta, pdb, json, png, log, weights), one per format in the pattern, each followed by its format name comment (e.g. # FASTA). Use ontologies: [] only where no term exists (e.g. a3m).
There was a problem hiding this comment.
Added in a260a90: FASTA (format_1929), PDB (format_1476), JSON (format_3464), PNG (format_3603) and textual format (format_2330) for the log. ontologies: [] for the a3m output and for weights, which is a directory of AlphaFold2 .npz parameter files with no matching EDAM format.
…t, meta.yml fixes - Containers: Seqera Wave builds. CPU via `nf-core modules containers create` (amd64; the arm64 builds failed on the Wave side). GPU variant from environment.gpu.yml (CUDA 12.0, jaxlib 0.4.25 cuda build, built with --build-template conda/micromamba:v2), selected with task.accelerator following the dual-container pattern of the GPU modules guide. - Pin dm-haiku=0.0.12 and flax=0.8.2: a fresh solve of colabfold 1.5.5 pulls dm-haiku 0.0.17, which fails at the first prediction with jax 0.4.25. - CUDA 12.0 rather than 11.8: the jaxlib cuda118 build has no ptxas, so ColabFold skipped every prediction while still exiting 0. - Emit the CUDA version on the versions topic (no CUDA available on CPU). - Tests: real single-sequence prediction of l_arginase in main.gpu.nf.test (tag gpu, nextflow.gpu.config sets accelerator = 1); CPU tests renamed to <domain> - <inputs> - <outputs>; bare `assert process.success` before assertAll. - meta.yml: args_id, EDAM ontologies, one-line descriptions, weights marked Optional. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
|
@vagkaratzas all points are addressed in a260a90; replies are in each thread. On the GPU point:
Local checks: nf-test (singularity) CPU and GPU tests pass and their snapshots are stable on a second run; GPU run on an RTX 2060; One note for running it: colabfold sets |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
- Thanks, the GPU setup now follows the GPU modules guide: two containers chosen by
task.accelerator,cuda-versionpinned exactly at the lowest working version,jaxlib=*cuda*build glob, CUDA version emitted on the versions topic, and agpu-taggedmain.gpu.nf.testwith real and stub tests andaccelerator = 1/maxForks = 1. The GPU CI jobs pass. - Blocking: the module should expose every file
colabfold_batchreads or writes. The template inputs and several outputs (config.json,cite.bibtex, PAE JSON, pickles, representations, zip) are missing. - Blocking: versions emits are missing for the pinned
dm-haiku,flaxandjax, and the stub does not match a real run. - Blocking (GPU tests): the stable checks need to go inside the snapshot, the test arguments should come through
params.module_args, and the stub test name should end with- stub.
Two questions, not blocking:
meta.ymlcontainers:and.conda-lock/only cover the CPU build. Ifnf-core modules containers createcan handleenvironment.gpu.yml, please add the GPU entries too, so the GPU image is tracked like the CPU one.- Was the GPU image built with
--build-template conda/micromamba:v2, as the guide recommends for GPU environments?
Diff 7.74/10 · Overall 7.74/10
nf-core-reviewer 0.5.0dev / vagkaratzas-review (rubric v4) · claude-opus-5-5
| input: | ||
| tuple val(meta), path(fasta) | ||
| tuple val(meta2), path(weights) |
There was a problem hiding this comment.
colabfold_batch also reads template files: --custom-template-path (a folder of PDB/mmCIF files), and --pdb-hit-file with --local-pdb-path. Files can't go through ext.args, so please add them as optional inputs (e.g. tuple val(meta3), path(templates)) and pass each flag only when its input is given. --templates itself stays in ext.args.
There was a problem hiding this comment.
Done in a5aaff6. Two optional inputs: tuple val(meta3), path(templates) for --custom-template-path, and tuple val(meta4), path(pdb_hits), path(local_pdb) for --pdb-hit-file / --local-pdb-path. Each flag is only added when its input is given, and --templates stays in ext.args. I checked it with a real run using 1L2Y as a custom template for the Trp-cage sequence: colabfold reports found templates: ['1l2y_A'] and writes *_template_domain_names.json.
| tuple val(meta), path("${prefix}/*_rank_*.pdb") , emit: pdb , optional: true | ||
| tuple val(meta), path("${prefix}/*_scores_rank_*.json"), emit: json, optional: true | ||
| tuple val(meta), path("${prefix}/*.a3m") , emit: a3m , optional: true | ||
| tuple val(meta), path("${prefix}/*.png") , emit: png , optional: true | ||
| tuple val(meta), path("${prefix}/log.txt") , emit: log , optional: true |
There was a problem hiding this comment.
Several files that colabfold_batch writes are not emitted: config.json, cite.bibtex, *_predicted_aligned_error_v1.json, the *.pickle files (--msa-only, --save-all), the *_repr_*.npy files (--save-single-representations, --save-pair-representations), *.result.zip (--zip) and *_template_domain_names.json (--templates). Please emit them as optional outputs, and update meta.yml and the stub to match.
| tuple val(meta), path("${prefix}/*_rank_*.pdb") , emit: pdb , optional: true | |
| tuple val(meta), path("${prefix}/*_scores_rank_*.json"), emit: json, optional: true | |
| tuple val(meta), path("${prefix}/*.a3m") , emit: a3m , optional: true | |
| tuple val(meta), path("${prefix}/*.png") , emit: png , optional: true | |
| tuple val(meta), path("${prefix}/log.txt") , emit: log , optional: true | |
| tuple val(meta), path("${prefix}/*_rank_*.pdb") , emit: pdb , optional: true | |
| tuple val(meta), path("${prefix}/*_scores_rank_*.json") , emit: json , optional: true | |
| tuple val(meta), path("${prefix}/*_predicted_aligned_error_v1.json"), emit: pae , optional: true | |
| tuple val(meta), path("${prefix}/*.a3m") , emit: a3m , optional: true | |
| tuple val(meta), path("${prefix}/*.png") , emit: png , optional: true | |
| tuple val(meta), path("${prefix}/*.pickle") , emit: pickle , optional: true | |
| tuple val(meta), path("${prefix}/*_repr_*.npy") , emit: npy , optional: true | |
| tuple val(meta), path("${prefix}/*.result.zip") , emit: zip , optional: true | |
| tuple val(meta), path("${prefix}/*_template_domain_names.json") , emit: templates, optional: true | |
| tuple val(meta), path("${prefix}/config.json") , emit: config , optional: true | |
| tuple val(meta), path("${prefix}/cite.bibtex") , emit: bibtex , optional: true | |
| tuple val(meta), path("${prefix}/log.txt") , emit: log , optional: true |
There was a problem hiding this comment.
Done in a5aaff6, with your suggested outputs (pae, pickle, npy, zip, templates, config, bibtex), plus meta.yml entries and EDAM terms where they exist (ZIP format_3987, NumPy format_4003, pickle format_4002; [] for BibTeX). I checked the file names against colabfold 1.5.5's source: representations are *_single_repr_*.npy / *_pair_repr_*.npy and the pickles are *_all_*.pickle (--save-all) or <query>.pickle (--msa-only), so the globs match.
| tuple val(meta), path("${prefix}/*.png") , emit: png , optional: true | ||
| tuple val(meta), path("${prefix}/log.txt") , emit: log , optional: true | ||
| tuple val("${task.process}"), val('colabfold'), eval("python -c 'import importlib.metadata as m; print(m.version(\"colabfold\"))'"), topic: versions, emit: versions_colabfold | ||
| tuple val("${task.process}"), val('cuda'), eval('python -c "import glob, json, sys; f = glob.glob(sys.prefix + \'/conda-meta/cuda-version-*.json\'); print(json.load(open(f[0]))[\'version\'] if f else \'no CUDA available\')"'), topic: versions, emit: versions_cuda |
There was a problem hiding this comment.
dm-haiku and flax are pinned in both environments, and jax in the GPU one, but none of them has a versions emit. Since they are pinned to keep colabfold working, please report them too (then refresh meta.yml with nf-core modules lint colabfold --fix).
| tuple val("${task.process}"), val('cuda'), eval('python -c "import glob, json, sys; f = glob.glob(sys.prefix + \'/conda-meta/cuda-version-*.json\'); print(json.load(open(f[0]))[\'version\'] if f else \'no CUDA available\')"'), topic: versions, emit: versions_cuda | |
| tuple val("${task.process}"), val('cuda'), eval('python -c "import glob, json, sys; f = glob.glob(sys.prefix + \'/conda-meta/cuda-version-*.json\'); print(json.load(open(f[0]))[\'version\'] if f else \'no CUDA available\')"'), topic: versions, emit: versions_cuda | |
| tuple val("${task.process}"), val('dm-haiku'), eval("python -c 'import importlib.metadata as m; print(m.version(\"dm-haiku\"))'"), topic: versions, emit: versions_dm_haiku | |
| tuple val("${task.process}"), val('flax'), eval("python -c 'import importlib.metadata as m; print(m.version(\"flax\"))'"), topic: versions, emit: versions_flax | |
| tuple val("${task.process}"), val('jax'), eval("python -c 'import importlib.metadata as m; print(m.version(\"jax\"))'"), topic: versions, emit: versions_jax |
There was a problem hiding this comment.
Done in a5aaff6: versions_dm_haiku, versions_flax and versions_jax, all on the versions topic. The GPU snapshot now records dm-haiku 0.0.12, flax 0.8.2, jax 0.4.25 and cuda 12.0.
| """ | ||
| mkdir ${prefix} | ||
| touch ${prefix}/${prefix}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | ||
| touch ${prefix}/${prefix}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | ||
| touch ${prefix}/${prefix}.a3m | ||
| touch ${prefix}/${prefix}_plddt.png | ||
| touch ${prefix}/log.txt | ||
| """ |
There was a problem hiding this comment.
The stub doesn't match a real run. A real run also writes _coverage.png, _pae.png, the PAE JSON, config.json and cite.bibtex, and an empty FASTA gives no output at all. The stub always creates files and writes only one plot.
| """ | |
| mkdir ${prefix} | |
| touch ${prefix}/${prefix}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | |
| touch ${prefix}/${prefix}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | |
| touch ${prefix}/${prefix}.a3m | |
| touch ${prefix}/${prefix}_plddt.png | |
| touch ${prefix}/log.txt | |
| """ | |
| mkdir ${prefix} | |
| if [ -s ${fasta} ]; then | |
| touch ${prefix}/${prefix}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | |
| touch ${prefix}/${prefix}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | |
| touch ${prefix}/${prefix}_predicted_aligned_error_v1.json | |
| touch ${prefix}/${prefix}.a3m | |
| touch ${prefix}/${prefix}_coverage.png | |
| touch ${prefix}/${prefix}_pae.png | |
| touch ${prefix}/${prefix}_plddt.png | |
| touch ${prefix}/config.json | |
| touch ${prefix}/cite.bibtex | |
| touch ${prefix}/log.txt | |
| fi | |
| """ |
There was a problem hiding this comment.
Done in a5aaff6 with your suggestion: the stub now writes the three plots, the PAE JSON, config.json and cite.bibtex, and nothing when the FASTA is empty.
| assert process.success | ||
| assertAll( | ||
| { assert snapshot(sanitizeOutput(process.out, unstableKeys: ['pdb', 'json', 'png', 'log'])).match() }, | ||
| { assert path(process.out.pdb[0][1]).readLines().any { it.startsWith('ATOM') } }, | ||
| { assert path(process.out.log[0][1]).text.contains('Done') } | ||
| ) |
There was a problem hiding this comment.
Please put the stable checks for the unstable channels inside the snapshot, so a change shows up as a snapshot diff, and add one for json. This also replaces the implicit it with a named closure parameter. The atom count of the unrelaxed model depends only on the sequence, so it stays stable across runs.
| assert process.success | |
| assertAll( | |
| { assert snapshot(sanitizeOutput(process.out, unstableKeys: ['pdb', 'json', 'png', 'log'])).match() }, | |
| { assert path(process.out.pdb[0][1]).readLines().any { it.startsWith('ATOM') } }, | |
| { assert path(process.out.log[0][1]).text.contains('Done') } | |
| ) | |
| assert process.success | |
| assertAll( | |
| { assert snapshot( | |
| sanitizeOutput(process.out, unstableKeys: ['pdb', 'json', 'png', 'log']), | |
| path(process.out.pdb[0][1]).readLines().count { line -> line.startsWith('ATOM') }, | |
| path(process.out.json[0][1]).text.contains('plddt'), | |
| path(process.out.log[0][1]).text.contains('Done') | |
| ).match() } | |
| ) |
There was a problem hiding this comment.
Done in a5aaff6: the ATOM count, the plddt check on the scores JSON and the Done check on the log are now in the snapshot, with a named closure parameter. pae, config and bibtex are added to unstableKeys. Two local GPU runs gave the same snapshot.
| } | ||
| } | ||
|
|
||
| test("proteomics - faa - stub - gpu") { |
There was a problem hiding this comment.
Stub test names should end with - stub.
| test("proteomics - faa - stub - gpu") { | |
| test("proteomics - faa - gpu - stub") { |
There was a problem hiding this comment.
Renamed to proteomics - faa - gpu - stub in a5aaff6.
| accelerator = 1 | ||
| // Concurrent GPU tasks on one GPU can deadlock under Singularity | ||
| maxForks = 1 | ||
| ext.args = '--msa-mode single_sequence --num-models 1 --num-recycle 1' |
There was a problem hiding this comment.
Please don't hardcode test arguments in the config. Pass them through params.module_args, and add params { module_args = '--msa-mode single_sequence --num-models 1 --num-recycle 1' } to the when block of the GPU tests.
| ext.args = '--msa-mode single_sequence --num-models 1 --num-recycle 1' | |
| ext.args = { params.module_args ?: '' } |
There was a problem hiding this comment.
Done in a5aaff6: ext.args = { params.module_args ?: '' } in the config, and params { module_args = '--msa-mode single_sequence --num-models 1 --num-recycle 1' } in the when block of both GPU tests.
|
@JoseEspinosa have a look at this one to see where it is going, and let us know if it would be re-usable in proteinfold ;) |
…s, fix tests - Optional inputs for custom templates (--custom-template-path) and for a PDB hit file with a local PDB mirror (--pdb-hit-file, --local-pdb-path); each flag is passed only when its input is given, --templates stays in ext.args. - Emit every file colabfold_batch can write: PAE JSON, pickles, representation .npy files, result.zip, template domain names, config.json and cite.bibtex. - Report the pinned dm-haiku, flax and jax on the versions topic. - Stub writes the same files as a real run, and nothing for an empty FASTA. - GPU tests: stable checks inside the snapshot, test arguments through params.module_args, stub test renamed to end with "- stub". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@vagkaratzas thanks, all points are addressed in a5aaff6 and CI is green, including the GPU jobs. On your two questions:
|
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
-
Blocking: the stub names files after
prefix, while a real run names them after the FASTA headers. -
Blocking: the
weights,templatesandpdb_hitsinputs are not tested; only the download path runs in CI. -
Blocking (the weights download): with
weights = []every task downloads the 3.5 GB AlphaFold2 tarball into its own work directory (HOME=$PWD, so the cache is per task). A pipeline that folds N proteins as N tasks downloads it N times and uses N × 3.5 GB of disk. Please split the module the way caalm and fungtion do:colabfold/downloadmodelsdownloads the weights (e.g.download_alphafold_params()into the task directory, which also writesparams/download_finished.txt). A stub-only test is fine here, since the real download runs in the main module's test.colabfold/colabfold(moved fromcolabfold/) takesweightsas a required input. Its GPU test gets the weights fromsetup { run("COLABFOLD_DOWNLOADMODELS") { script "../../downloadmodels/main.nf" } }, so the--datapath that pipelines will use is the one tested. RenamewithName: 'COLABFOLD'innextflow.gpu.configto the new process name.
The 3.5 GB download on the GPU runners is fine: no smaller set of weights exists that test-datasets could host, the GPU jobs already finish in 3–5 minutes, and
checkm2/databasedownloadpulls a similar size from Zenodo. Just keep it out of the CPU tests.
Not blocking: the empty-FASTA guard in script exists mainly so the CPU file has a non-stub test, and that test only checks versions. With weights required, you can drop both the guard and that test and keep the CPU file stub-only, since the real run lives in main.gpu.nf.test.
Diff 9.19/10 · Overall 9.19/10
nf-core-reviewer 0.5.0 / vagkaratzas-review (rubric v4) · claude-opus-5-5
Comment added by Claude Opus 5.5
| stub: | ||
| prefix = task.ext.prefix ?: "${meta.id}" | ||
| """ | ||
| mkdir ${prefix} | ||
| if [ -s ${fasta} ]; then | ||
| touch ${prefix}/${prefix}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | ||
| touch ${prefix}/${prefix}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | ||
| touch ${prefix}/${prefix}_predicted_aligned_error_v1.json | ||
| touch ${prefix}/${prefix}.a3m | ||
| touch ${prefix}/${prefix}_coverage.png | ||
| touch ${prefix}/${prefix}_pae.png | ||
| touch ${prefix}/${prefix}_plddt.png | ||
| touch ${prefix}/config.json | ||
| touch ${prefix}/cite.bibtex | ||
| touch ${prefix}/log.txt | ||
| fi | ||
| """ |
There was a problem hiding this comment.
The stub names files after prefix, but colabfold_batch names them after each FASTA header (safe_filename keeps [A-Za-z0-9_.-], everything else becomes _). The GPU snapshot shows it: the real run writes GI_225038609_EFDID_719595_FULL.a3m, the stub writes test.a3m. That's on me, my earlier suggestion had the same mistake. Deriving the names from the headers fixes it and also gives one set of files per record.
| stub: | |
| prefix = task.ext.prefix ?: "${meta.id}" | |
| """ | |
| mkdir ${prefix} | |
| if [ -s ${fasta} ]; then | |
| touch ${prefix}/${prefix}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | |
| touch ${prefix}/${prefix}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | |
| touch ${prefix}/${prefix}_predicted_aligned_error_v1.json | |
| touch ${prefix}/${prefix}.a3m | |
| touch ${prefix}/${prefix}_coverage.png | |
| touch ${prefix}/${prefix}_pae.png | |
| touch ${prefix}/${prefix}_plddt.png | |
| touch ${prefix}/config.json | |
| touch ${prefix}/cite.bibtex | |
| touch ${prefix}/log.txt | |
| fi | |
| """ | |
| stub: | |
| prefix = task.ext.prefix ?: "${meta.id}" | |
| """ | |
| mkdir ${prefix} | |
| grep '^>' ${fasta} | sed 's/^>//; s/[^A-Za-z0-9_.-]/_/g' | while read -r name; do | |
| touch ${prefix}/\${name}_unrelaxed_rank_001_alphafold2_ptm_model_1_seed_000.pdb | |
| touch ${prefix}/\${name}_scores_rank_001_alphafold2_ptm_model_1_seed_000.json | |
| touch ${prefix}/\${name}_predicted_aligned_error_v1.json | |
| touch ${prefix}/\${name}.a3m | |
| touch ${prefix}/\${name}_coverage.png | |
| touch ${prefix}/\${name}_pae.png | |
| touch ${prefix}/\${name}_plddt.png | |
| done | |
| if [ -s ${fasta} ]; then | |
| touch ${prefix}/config.json | |
| touch ${prefix}/cite.bibtex | |
| touch ${prefix}/log.txt | |
| fi | |
| """ |
There was a problem hiding this comment.
Done, with your suggestion. For an A3M input the name comes from the file name, as colabfold does it. The stub snapshot now shows GI_225038609_EFDID_719595_FULL.a3m, the same name as the real run.
| e.g. [ id:'alphafold2_params' ] | ||
| - weights: | ||
| type: directory | ||
| description: Optional ColabFold data directory with the AlphaFold2 parameters in `params/`; pass `[]` to download them |
There was a problem hiding this comment.
colabfold_batch checks for an empty file, params/download_finished.txt, which colabfold's own downloader creates when it finishes. It checks even when --data is given. If the file is missing (e.g. weights extracted by hand, or reused from proteinfold), colabfold downloads the 3.5 GB again into the staged weights folder. That writes through the symlink into the upstream folder, or fails on read-only storage. Please say this in the description, so users who bring their own weights know to add the file.
| description: Optional ColabFold data directory with the AlphaFold2 parameters in `params/`; pass `[]` to download them | |
| description: ColabFold data directory with the AlphaFold2 parameters in `params/`. It must contain the empty `params/download_finished.txt` that colabfold's downloader creates (e.g. via `colabfold/downloadmodels`); without it colabfold downloads the weights again |
There was a problem hiding this comment.
Applied as suggested.
| [ id:'test' ], | ||
| file(params.modules_testdata_base_path + 'proteomics/interproscan/l_arginase.faa', checkIfExists: true) | ||
| ] | ||
| input[1] = [ [:], [] ] |
There was a problem hiding this comment.
No test passes weights, templates or pdb_hits, so the --data, --custom-template-path and --pdb-hit-file / --local-pdb-path branches never run in CI. Once the weights come from a setup block (see the review body), the --data path is tested. For templates, the cheapest test is a second GPU test with the 20-residue Trp-cage sequence and the 1L2Y template you already tried locally (both small enough for test-datasets), with --templates in module_args.
There was a problem hiding this comment.
Done: the weights come from the setup block, and two new GPU tests cover templates and pdb_hits / local_pdb on Trp-cage (details in the general comment). Test data: nf-core/test-datasets#2354.
| type: file | ||
| description: Optional BLAST-m8 PDB hit file for the input MSA, e.g. from colabfold_search (`--pdb-hit-file`; needs `--templates` in `ext.args`) | ||
| pattern: "*.m8" | ||
| ontologies: [] |
There was a problem hiding this comment.
*.m8 is a tab-separated BLAST table; TSV (format_3475) fits until EDAM gets a specific term.
| ontologies: [] | |
| ontologies: | |
| - edam: http://edamontology.org/format_3475 # TSV |
There was a problem hiding this comment.
Applied (TSV, format_3475).
|
@Bboy010 thanks, getting there! |
… test every input - New colabfold/downloadmodels (COLABFOLD_DOWNLOADMODELS): downloads a parameter set once with colabfold's own download_alphafold_params(), which also writes the params/download*_finished.txt marker. Stub test. - colabfold/colabfold (COLABFOLD_COLABFOLD, moved from colabfold/): weights are a required input (--data); the empty-FASTA guard and its test are gone, the CPU test file is stub-only. - templates are staged as files into templates/, so the pdb70 index and PDB->mmCIF conversions colabfold writes there stay in the task dir. - The input also accepts a single A3M (colabfold names that query after the file); with a FASTA, --templates always queries the MSA server. - Stub names the files after the FASTA headers (or the A3M file name). - GPU tests take the weights from a setup block running COLABFOLD_DOWNLOADMODELS, and cover custom templates (1L2Y) and pdb_hits + local_pdb (via UNTAR) on Trp-cage; test data in nf-core/test-datasets#2354. - meta.yml: weights description (download marker), TSV for *.m8. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @vagkaratzas! The split is done in e22eedc. I followed caalm/fungtion:
Two changes go beyond your suggestions, so please take a look:
|
|
Fixes failing tests please and I'll have another look! |
Thanks @vagkaratzas! The code is not what fails: every failing test reads trp_cage.a3m or local_pdb.tar.gz, which are added by nf-core/test-datasets#2354. In the same jobs the other tests pass, including the real GPU tests with the 3.5 GB weights download. The template tests pass locally with those files. #2354 is green (pre-commit) but has no reviewer yet. Could you, or someone with access, have a look at it? Once it is merged I'll re-run the failed jobs here. |
|
Merged test-datasets and merged main branch to rerun tests anew. |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
- Thanks, the split worked out well.
colabfold/downloadmodelsfeeds the GPU tests throughsetup, so they now test the--datapath that pipelines will use, plus the custom-template andpdb_hits/local_pdbpaths. The stub names match a real run. CI is green, GPU jobs included. - Blocking:
colabfold/downloadmodelshas no versions emits for the pinneddm-haikuandflax. - Small: the
local_pdbdirectory entry inmeta.ymlshould haveontologies: [].
Diff 9.52/10 · Overall 9.52/10
nf-core-reviewer 0.5.0 / vagkaratzas-review (rubric v4) · claude-opus-5-5
|
|
||
| output: | ||
| tuple val(meta), path("weights"), emit: weights | ||
| tuple val("${task.process}"), val('colabfold'), eval("python -c 'import importlib.metadata as m; print(m.version(\"colabfold\"))'"), topic: versions, emit: versions_colabfold |
There was a problem hiding this comment.
dm-haiku and flax are pinned in this environment.yml too, so they need their own versions emits here, the same as in colabfold/colabfold. Then run nf-core modules lint colabfold/downloadmodels --fix to update meta.yml.
| tuple val("${task.process}"), val('colabfold'), eval("python -c 'import importlib.metadata as m; print(m.version(\"colabfold\"))'"), topic: versions, emit: versions_colabfold | |
| tuple val("${task.process}"), val('colabfold'), eval("python -c 'import importlib.metadata as m; print(m.version(\"colabfold\"))'"), topic: versions, emit: versions_colabfold | |
| tuple val("${task.process}"), val('dm-haiku'), eval("python -c 'import importlib.metadata as m; print(m.version(\"dm-haiku\"))'"), topic: versions, emit: versions_dm_haiku | |
| tuple val("${task.process}"), val('flax'), eval("python -c 'import importlib.metadata as m; print(m.version(\"flax\"))'"), topic: versions, emit: versions_flax |
| ontologies: | ||
| - edam: http://edamontology.org/format_1477 # mmCIF |
There was a problem hiding this comment.
local_pdb is a directory, and directory entries get ontologies: [].
| ontologies: | |
| - edam: http://edamontology.org/format_1477 # mmCIF | |
| ontologies: [] |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Nice, preemptively approving this round. Just make sure to fix the last two issues before merging :D
…logy for local_pdb - colabfold/downloadmodels: versions_dm_haiku and versions_flax emits, as in colabfold/colabfold (both are pinned in environment.yml). - colabfold/colabfold meta.yml: local_pdb is a directory, so `ontologies: []`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @vagkaratzas! Both fixed in c34377f: colabfold/downloadmodels now emits the dm-haiku and flax versions, and local_pdb has ontologies: []. CI is green, GPU jobs included. |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Thank you @Bboy010! Good job!
New module
colabfold: protein structure prediction with ColabFold (AlphaFold2), MSA from an MMseqs2 server.No existing module covers this (checked
modules/nf-core, open/closed PRs and issues forcolabfold/alphafold;foldseek/foldcomp/foldmasonare search/compression tools, not predictors).versionstopic channel.proteomics/interproscan/l_arginase.faa. A real fold needs ~4 GB of weights, an MMseqs2 server and a GPU/hours of CPU, which CI runners lack.nf-core modules lint colabfold: 59 passed, 1 warning (process_gpulabel, expected for a GPU module), 0 failed.PR checklist
label.nf-core modules lint colabfoldandnf-test test modules/nf-core/colabfold --profile dockerpass locally.