Repository navigation
Conversation
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.3.1. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
…nto unfinished-diamond-blastp
…o unfinished-diamond-blastp
…aretaxa local module template.
…t and wrote an initial main.nf.test for the process
…t process is successful.
…utilized tests/nextflow.config to fix memory error.
…to diamondpreparetaxa module
…est1.fasta and test2.fasta to the diamond subworkflow directory for diamond/blastp nf-test input.
…iamond subworkflow Diamond_blastp output. Included Diamond subworkflow execution in the functional annotation subworkflow.
olgabot
left a comment
olgabot
left a comment
There was a problem hiding this comment.
Turns out I had started this review a while ago... But here are a bunch of suggestions that will hopefully deal with the container issues.
Co-authored-by: Olga Botvinnik <olga.botvinnik@gmail.com>
…w config processes for DIAMOND_MAKEDB and DIAMOND_BLASTP.
…o unfinished-diamond-blastp
…blastp' into unfinished-diamond-blastp
Non-nf-core stuff and merge conflicts: done.
On functional_annotation vs its own derive_taxonomy subworkflow: keeping it under functional_annotation for now, will revisit with @olgabot since she's the one who set up this structure originally.
Metro map: good catch, you're right. Checked and DIAMOND isn't in it (neither is KOfamScan). There's also a tool for this now, nf-metro, converts Nextflow's dag output into the metro map style directly, in case that's useful. You'd mentioned you're happy to handle the update yourself, so deferring to you on that unless you'd rather I take a pass at it (haven't done this myself). |
|
@tracelail can you resolve the last file conflict so I can hopefully go through the PR once more on Monday? |
Resolved! |
|
|
@tracelail you are still not a member of the nf-core github organization. Can you request that through the github-invitations slack channel? |
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Left another round of comments. These, along with the failing tests will hopefully guide you a bit better now.
| when { | ||
| process { | ||
| """ | ||
| input[0] = "file://${moduleTestDir}/mini_taxdump.tar.gz" |
There was a problem hiding this comment.
The proper way to do this is to push this file at: https://github.com/nf-core/test-datasets/tree/proteinannotator/testdata
Take a look into the rest of the local subworkflows for inspiration (e.g., functional_annotation nf-tests)
| process "DIAMONDPREPARETAXA" | ||
|
|
||
| tag "modules" | ||
| tag "modules_" |
There was a problem hiding this comment.
| tag "modules_" |
not a legit tag
| assert process.trace.tasks().size() == 1 | ||
| assert process.out.refseq_fasta.size() == 1 | ||
| assert snapshot(process.out).match() |
There was a problem hiding this comment.
| assert process.trace.tasks().size() == 1 | |
| assert process.out.refseq_fasta.size() == 1 | |
| assert snapshot(process.out).match() | |
| { assert snapshot(sanitizeOutput(process.out)).match() } |
try using sanitizeOutput instead, which is the new standard way in nf-core assertions. This also covers the tool versions, which do not show anywhere in the snapshot atm. You will have to reproduce the snapshots.
There was a problem hiding this comment.
Update the rest of the test assertions accordingly as well
|
|
||
| test("Should download ncbi refseq 'other' zipped protein fasta -- stub") { | ||
| tag "stub" | ||
| tag "CI" |
There was a problem hiding this comment.
Remove these tag "CI" throughout
| tag "modules" | ||
| tag "modules_" | ||
| tag "ncbirefseqdownload" | ||
| tag "diamond" | ||
| tag "diamond_local" |
There was a problem hiding this comment.
| tag "modules" | |
| tag "modules_" | |
| tag "ncbirefseqdownload" | |
| tag "diamond" | |
| tag "diamond_local" | |
| tag "modules" | |
| tag "ncbirefseqdownload" |
no diamond tool or diamond_local is called. update like this across
tags should include:
tag "modules" by default and then a tag for each module that is executed
| description: NCBI taxonomy names file containing taxon names | ||
| pattern: "names.dmp" | ||
|
|
||
| - versions_curl: |
There was a problem hiding this comment.
out of sync, should contain the new topic versions channel with the proper tool (tar)
- Remove genome/igenomes_base/igenomes_ignore from nextflow.config; stray template params not in schema (fixes schema_params lint failures) - Add enum constraint to refseq_release (14 valid NCBI RefSeq divisions) - Add format: file-path to taxondmp_zip/taxonmap - Remove redundant default: false from skip_diamond schema entry (matches convention of other skip_* params; default lives in nextflow.config) - Rewrite diamondpreparetaxa meta.yml to match actual main.nf: tar (not untar/diamond), path input type, topic-channel versions - Trim unused grep/lbzip2 deps from diamondpreparetaxa environment.yml - Remove stray <h1> from README.md - Update usage.md with refseq_release valid values - Clean up test tags across diamondpreparetaxa, ncbirefseqdownload, diamond, and functional_annotation tests (remove tag "modules_", stray tag "CI", mismatched tool tags)
|
Shard 6/7 has failed twice now with identical Looking into it, it seems structural rather than flaky: tests/default.nf.test (the full -profile test run) always lands on shard 6, since sharding splits by file and there are only 6 test files across 7 shards. That test downloads every domain + functional annotation database and container concurrently (Pfam, FunFam, metagRoot, NMPfams, InterProScan, KOfamScan, refseq, taxdump), so it's by far the heaviest job in the suite. I don't believe have visibility into the runner's disk config or whether this is a known issue. @vagkaratzas: do you know what I should do here, or who'd be the right person to flag it to? (Used Claude to help investigate the logs and draft this comment.) |
By now, I've used Claude that much, that I can feel its "watermark" in code, text, video and audio..no worries :p The problem happens in Tests should always run with the minimal viable data possible, so try to create that subset file and host it in the test-datasets proteinannotator branch along with the rest! Fast and accurate ;) |
|
Hi @tracelail ! Now with the test data up, this should be close to completion, right? Looking into a 1.2.0 release soon with the Would be nice to have this completed by then, otherwise we could push this to an upcoming 1.3.0 release. |
I hope so! I am looking to resubmit for review by Monday at the latest. |
…unfinished-diamond-blastp Merged to sync remote branch merge conflict with local branch
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Thanks for all the work on this, @tracelail — CI is green and the test fixtures from nf-core/test-datasets#2287 are wired in correctly for -profile test. There are still a few things to address before merging, mostly around what happens on real (non-test) runs, plus some leftovers from the dev merge. Details are in the inline comments; summary below.
Blocking
- DIAMOND must be opt-in:
skip_diamondshould default totrue. Without-profile test, the defaults (refseq_release = 'complete', fullprot.accession2taxid.gz) mean every run downloads ~97 GB of RefSeq protein FASTAs (1,907 files inrefseq/release/complete/, measured today) plus ~12 GB for the accession2taxid map, then rebuilds the DB from scratch. Please flip the default innextflow.config+nextflow_schema.json, and add a> [!WARNING]block in the DIAMOND section ofdocs/usage.mdstating these sizes (similar to the InterProScan note), suggesting smaller releases (e.g.viral,bacteria) where appropriate. - No way to reuse a built database. Every other DB in the pipeline has a "download or use existing local path" option. Please add a
diamond_dbparam (path to an existing.dmnd); when set, skipNCBIREFSEQDOWNLOAD/DIAMONDPREPARETAXA/DIAMOND_MAKEDB. Then users can feed back the publisheddownloaded_dbs/diamond/refseq.dmnd. - Raw downloads are published to
outdir.NCBIREFSEQDOWNLOADandDIAMONDPREPARETAXAfall through to the defaultpublishDir, so the whole RefSeq FASTA (another ~97 GB) lands inncbirefseqdownload/(seetests/default.nf.test.snap). Please disable publishing for both. - CHANGELOG: the entry was added under the already-released
v1.1.0section, and 7 old 2025 entries were brought back by the merge. Please drop those and add one line underv1.2.0dev→Addedin the current format, plus DIAMOND in theDependenciestable. docs/usage.mdInterProScan section got broken by the merge (stray sentence, DIAMOND section inserted in the middle of it, and an "Updating the pipeline" block with InterProScan 5.72 that contradicts the 5.59-91.0 requirement). Please restore it fromdevand place### DIAMONDafter it.subworkflows/local/functional_annotation/meta.yml: keywords moved intocomponents, and original authors moved undermaintainers. Please only appenddiamondto components and the new people to authors/maintainers.
Should fix
ncbirefseqdownload:zcat | gziprecompression (usecat, concatenated gzip is valid gzip),process_lowlabel for a ~97 GB job, apptainer missing from the container check, unpinnedca-certificates, browser user-agent spoofing towards NCBI, and the rclone sync branch (the one real users hit) is not covered by any test.- Local module
meta.ymlfiles are out of sync withmain.nf(versions.ymlvs topic, missingrefseq_fasta_urlinput,taxondmp_ziptyped as string while it is apathinput).diamondpreparetaxacontainer doesn't match itsenvironment.yml. - Params/schema:
taxondmp_zipis a.tar.gznot a zip; params lack adiamond_prefix unlikekofamscan_*/interproscan_*; wrongmimetypes; empty"default"on the group object; "taxonomic classification" wording is only true for outfmt 102.refseq_fasta_urlis undocumented in usage.md. - Tests: pipeline-level DIAMOND outputs are all empty files (md5
d41d8…) because the mini RefSeq has no homologs of the samplesheet proteins — a follow-up test-datasets PR adding a few homologous sequences (+ accession2taxid rows) would make this meaningful.functional_annotationtest snapshots a non-existentworkflow.out.versions. Minor path/tag inconsistencies inline.
Minor
- Unrelated changes (
.vscode/removal,nft-utilsbump, ro-crate regen) — fine if intentional, but ideally separate. - Please also cite the 2021 DIAMOND paper (Buchfink, Reuter, Drost, Nat Methods 2021, doi:10.1038/s41592-021-01101-x) in
CITATIONS.md. diamond/makedb(2.1.16) anddiamond/blastp(2.1.23) use different DIAMOND versions; worth aligning.- Still open from the previous round: should DIAMOND live under
functional_annotationor become its own taxonomy-oriented subworkflow? Since the taxonomy files are only useful forstaxids/sscinames/LCA output, a separate subworkflow may be cleaner.
| kofamscan_ko_list = null | ||
|
|
||
| // DIAMOND options | ||
| skip_diamond = false |
There was a problem hiding this comment.
Blocking: please default this to true. Without -profile test, the current defaults download ~97 GB of RefSeq complete protein FASTAs + ~12 GB prot.accession2taxid.gz on every run and rebuild the DB from scratch. DIAMOND should be opt-in (same change needed in nextflow_schema.json).
|
|
||
| // DIAMOND options | ||
| skip_diamond = false | ||
| refseq_release = 'complete' |
There was a problem hiding this comment.
Related: please add a diamond_db param (existing .dmnd) next to these; when set, skip download/makedb entirely so the published downloaded_dbs/diamond/refseq.dmnd can be reused, like the InterProScan/KOfam *_db options.
Also consider prefixing these params with diamond_ (diamond_refseq_release, diamond_taxonmap, diamond_taxdump) for consistency with kofamscan_* / interproscan_*. taxondmp_zip is a .tar.gz, not a zip.
| ] | ||
| } | ||
|
|
||
| withName: 'NFCORE_PROTEINANNOTATOR:PROTEINANNOTATOR:FUNCTIONAL_ANNOTATION:DIAMOND:DIAMOND_MAKEDB' { |
There was a problem hiding this comment.
NCBIREFSEQDOWNLOAD and DIAMONDPREPARETAXA have no override here, so they fall back to the default publishDir. That publishes the full RefSeq FASTA (~97 GB for complete) to outdir/ncbirefseqdownload/ and the taxdump files to outdir/diamondpreparetaxa/ (visible in tests/default.nf.test.snap). Please add withName blocks with publishDir = [ enabled: false ] for both.
|
|
||
| - [[PR #52](https://github.com/nf-core/proteinannotator/pull/52)] Add option to turn off InterProScan for testing | ||
| - [[PR #51](https://github.com/nf-core/proteinannotator/pull/51)] Update to nf-core/tools v3.3.1 | ||
| - [[PR #50](https://github.com/nf-core/proteinannotator/pull/50)] Add DIAMOND subworkflow to run [Diamond](https://github.com/bbuchfink/diamond) |
There was a problem hiding this comment.
This landed under the released v1.1.0 section, and lines 31-38 re-add old 2025 entries from the dev merge. Please remove these 8 lines and add a single entry under v1.2.0dev → Added, in the current style:
- [#50](https://github.com/nf-core/proteinannotator/pull/50) - Added DIAMOND blastp homology search against NCBI RefSeq (opt-in via `--skip_diamond false`). (by @tracelail)
and add diamond to the Dependencies table.
|
|
||
| ### InterProScan | ||
|
|
||
| Running [InterProScan](https://interproscan-docs.readthedocs.io/) requires a pre-prepared input database. You can provided this as one of two options: |
There was a problem hiding this comment.
Merge leftover: this sentence contradicts the paragraph below (the DB is auto-downloaded by default) and has a typo. Please restore the InterProScan section from dev.
| tag "diamond/makedb" | ||
| tag "diamond/blastp" | ||
|
|
||
| def testdata = 'https://raw.githubusercontent.com/nf-core/test-datasets/proteinannotator/testdata/diamond/' |
There was a problem hiding this comment.
Please use params.pipelines_testdata_base_path + 'testdata/diamond/' instead of hard-coding the URL. Minor: indentation of the test on line 72 is off.
| aws.client.anonymous = true | ||
|
|
||
| process { | ||
| withName: NCBIREFSEQDOWNLOAD { |
There was a problem hiding this comment.
Likely redundant: conf/test.config already caps resources via resourceLimits.
| ch_taxonnodes, | ||
| ch_taxonnames | ||
| ) | ||
| ch_diamond_db = DIAMOND_MAKEDB.out.db.first() |
There was a problem hiding this comment.
Harmless, but the upstream processes only take values, so db is already a value channel and .first() is a no-op.
|
|
||
| - [DIAMOND](https://github.com/bbuchfink/diamond) | ||
|
|
||
| > Buchfink B, Xie C, Huson DH, "Fast and sensitive protein alignment using DIAMOND", Nature Methods 12, 59-60 (2015). doi:10.1038/nmeth.3176 |
There was a problem hiding this comment.
Please also add the 2021 paper: Buchfink B, Reuter K, Drost HG, "Sensitive protein alignments at tree-of-life scale using DIAMOND", Nature Methods 18, 366-368 (2021). doi:10.1038/s41592-021-01101-x
| "K25926.hmm:md5,0f5e05441ef0fa21de35a39d70f8de6b", | ||
| "metagroot_test.hmm.gz:md5,d23de95bf39fb6e27ffb266ce61ac98e", | ||
| "nmpfamsdb_test.hmm.gz:md5,ad7a094618ccfdaeed1c03e93f6abf1e", | ||
| "T1024.refseq.txt:md5,d41d8cd98f00b204e9800998ecf8427e", |
There was a problem hiding this comment.
All DIAMOND outputs at pipeline level are empty files (md5 d41d8…): the mini RefSeq has no homologs of the samplesheet proteins. A follow-up test-datasets PR adding a few homologous sequences (+ accession2taxid rows) would make this test meaningful.
Note that once skip_diamond defaults to true, conf/test.config should set skip_diamond = false so the pipeline test keeps covering DIAMOND.
|
@tracelail let me know if the raised points make sense, and/or if you need help with any. |
Yes all of the comments make sense. Recent bandwidth is low so changes are slow. |
Protein functional annotation at scale demands efficient sequence alignment against large reference databases. BLAST, the traditional tool for this task, is computationally expensive for large datasets. [DIAMOND](https://github.com/bbuchfink/diamond) (Buchfink et al., [Nature Methods 2021](https://doi.org/10.1038/s41592-021-01101-x)) provides BLAST-compatible sensitivity with significantly higher throughput, up to 10,000× faster than BLAST on large protein databases by using a double-indexed alignment algorithm optimized for modern hardware. Adding DIAMOND blastp to
proteinannotatorenables users to run protein homology searches against the full NCBI RefSeq protein database as part of their functional annotation workflow, which would be impractical with BLAST at scale.This PR adds [DIAMOND blastp](https://github.com/bbuchfink/diamond) ([Buchfink et al., Nature Methods 2021](https://doi.org/10.1038/s41592-021-01101-x)) to the pipeline, using the existing nf-core
diamond/blastpand[diamond/makedb](https://nf-co.re/modules/diamond_makedb/)modules, plus two new local modules:New local modules:
ncbirefseqdownload: Downloads and concatenates NCBI RefSeq protein FASTAs for a specified release category (e.g.complete,other) into a single compressed reference FASTA for use bydiamond/makedbdiamondpreparetaxa: Extracts NCBI taxonomy files (nodes.dmp,names.dmp) required for taxonomic classification indiamond/makedb, from a local path or remote URL supplied directly to Nextflow's own file stagingNew subworkflows:
subworkflows/local/diamond: Orchestrates the full DIAMOND pipeline: RefSeq download → taxonomy preparation →DIAMOND_MAKEDB→DIAMOND_BLASTP. Supports all seven DIAMOND output formats (blast,xml,txt,daa,sam,tsv,paf) viaparams.diamond_outfmtandparams.diamond_blast_columnssubworkflows/local/functional_annotation— Integrates theDIAMONDsubworkflow alongside the existing InterProScan and KOfamScan logic. All three tools are independently gated by their own skip flag (params.skip_diamond,params.skip_interproscan,params.skip_kofamscan);FUNCTIONAL_ANNOTATIONpasses through all seven of DIAMOND's real output channels (diamond_blast,diamond_xml,diamond_txt,diamond_daa,diamond_sam,diamond_tsv,diamond_paf) rather than hardcoding a single outfmt-specific emit. Consuming these outputs downstream inproteinannotator.nf(e.g. a comparison/reporting step) is left to a follow-up PR.New parameters (added to
nextflow_schema.jsonandnextflow.config):-skip_diamond— Skip the DIAMOND BLASTP taxonomic classification step entirely (default:false)-refseq_release— NCBI RefSeq release category (default:complete)-taxondmp_zip— URL to NCBI taxonomy dump archive-taxonmap— URL to compressed protein accession-to-taxid map-diamond_outfmt— Output format code (default:6, tabular)-diamond_blast_columns— Optional column list for tabular outputTesting:
All new local modules have nf-test test suites with both live and stub tests:
ncbirefseqdownload— tested against the NCBIotherRefSeq release viarclone(NCBI [permanently discontinued rsync support](https://ncbiinsights.ncbi.nlm.nih.gov/2026/03/25/retire-rsync-support-ftp-downloads/) as of June 1, 2026)diamondpreparetaxa— tested against a minimalmini_taxdump.tar.gzfixture rather than NCBI's full real taxonomy dump, to keep CI fast and network-independent; stub creates placeholdernodes.dmpandnames.dmpThe
diamondsubworkflow is tested end-to-end using a miniaturemini_prot.accession2taxid.gztaxon map and a smalltest_refseq.fasta. Tests cover five scenarios:DIAMOND_MAKEDB's output now correctly broadcasts to every sample via.first(), rather than pairing positionally and silently dropping any sample beyond the first)The
functional_annotationsubworkflow is tested withskip_interproscan = trueandskip_diamond = falsefor DIAMOND-focused tests (live and stub), confirming the DIAMOND path runs correctly independently. Pre-existing InterProScan- and KOfamScan-focused tests now setskip_diamond = true, since they were previously running the full DIAMOND pipeline unconditionally despite being unrelated to what they test.All stub tests are tagged
CIfor fast pipeline CI runs. Live module tests require-profile dockerand are tagged accordingly. All local-module and subworkflow live tests, including the DIAMOND-focused paths indiamondandfunctional_annotation, have been run and verified under real-profile dockerexecution.PR checklist
nf-core pipelines lint).nf-test test */local --profile=~test,dockerfor all new local tests).nf-test test */local --profile=~test,docker,debug).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).