Conversation
Member
|
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.
11 tasks
…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
requested changes
Jul 22, 2025
olgabot
left a comment
olgabot
left a comment
Collaborator
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
Changed taxondmp_zip input from val to path, letting Nextflow stage local/remote files itself instead of curl-ing inside the container (val inputs are never bind-mounted, which broke under --profile docker). Also fixes the pre-existing hardcoded tar filename bug for free. Container swapped to match UNTAR's minimal coreutils/tar image, since curl is no longer needed. environment.yml/meta.yml updated to match.
Author
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). |
Collaborator
|
@tracelail can you resolve the last file conflict so I can hopefully go through the PR once more on Monday? |
Author
Resolved! |
|
Collaborator
|
@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
requested changes
Sep 7, 2026
vagkaratzas
left a comment
vagkaratzas
left a comment
Collaborator
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" |
Collaborator
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_" |
Collaborator
There was a problem hiding this comment.
Suggested change
| tag "modules_" |
not a legit tag
Comment on lines
+25
to
+27
| assert process.trace.tasks().size() == 1 | ||
| assert process.out.refseq_fasta.size() == 1 | ||
| assert snapshot(process.out).match() |
Collaborator
There was a problem hiding this comment.
Suggested change
| 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.
Collaborator
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" |
Collaborator
There was a problem hiding this comment.
Remove these tag "CI" throughout
Comment on lines
+7
to
+11
| tag "modules" | ||
| tag "modules_" | ||
| tag "ncbirefseqdownload" | ||
| tag "diamond" | ||
| tag "diamond_local" |
Collaborator
There was a problem hiding this comment.
Suggested change
| 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: |
Collaborator
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)
Author
|
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.) |
Collaborator
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 ;) |
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
7 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.
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).