Skip to content

New module: GRIMER#11663

Open
Jonahnki wants to merge 12 commits into
nf-core:masterfrom
Jonahnki:grimer
Open

New module: GRIMER#11663
Jonahnki wants to merge 12 commits into
nf-core:masterfrom
Jonahnki:grimer

Conversation

@Jonahnki
Copy link
Copy Markdown

Closes #11566

Description

Adds a new nf-core module for GRIMER (v1.1.0), a tool that generates interactive HTML dashboards for contamination detection in metagenomics and viromics datasets.

Developed during the May 2026 nf-core x VirJenDB Virus Bioinformatics Hackathon.

Checklist

  • environment.yml matches bioconda package
  • main.nf passes lint (47/47 tests passed)
  • meta.yml complete and schema-valid
  • nf-test dry run passes (2/2 tests discovered)
  • All pre-commit hooks pass

@Jonahnki Jonahnki requested review from a team as code owners May 17, 2026 15:33
@Jonahnki
Copy link
Copy Markdown
Author

The stub test now uses an existing nf-core/test-datasets file to pass CI. I hope to add in a proper GRIMER-specific count table TSV to nf-core/test-datasets in a follow-up post-PR after review

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.

Can you create this file on-the-fly within the nf.test please.

Comment thread modules/nf-core/grimer/meta.yml Outdated
Comment thread modules/nf-core/grimer/meta.yml
Comment thread .nf-core.yml
Comment thread modules/nf-core/grimer/tests/main.nf.test
Comment thread modules/nf-core/grimer/tests/main.nf.test Outdated
@Jonahnki
Copy link
Copy Markdown
Author

Thanks for the latest comments @SPPearce! I've gone through everything and pushed some fixe

*The metadata input in main.nf was tripping the linter since anything prefixed with meta gets treated as a Groovy map and renamed it to sample_metadata and that cleared it up
*Synced meta.yml to match while I was at it; --fix had silently collapsed the sample_metadata block to {} so I restored the full field descriptions manually. All 51 lint checks are green now
*For the test part, I swaped out the bare assert calls for snapshot(process.out).match() and added a -stub block. Test data is now generated on-the-fly in setup {} so the static tests/testdata/test_count_table.tsv wasn't needed anymore and I've removed it
*.nf-core.yml is also back to upstream state

@Jonahnki
Copy link
Copy Markdown
Author

Managed to fix the two GRIMER-specific CI failures in the latest push:

*Version eval: grimer --version outputs a multi-line ASCII art banner rather than a clean version string, which caused the snapshot mismatch on CI. Changed the eval to grimer --version 2>&1 | tail -1 which reliably extracts just the bare version number from the last line of the output
*meta.yml sync: ran nf-core modules lint --fix to update the version eval string in both the outputs and topics blocks to match the updated main.nf. All 51 lint checks now passing and the stub snapshot has been regenerated to match

The remaining CI failures (SENTIEON, HMMER, WFMASH, SALSA2, KRAKENUNIQ etc.) are infrastructure-level flakes — runner disk full (no space left on device) and conda HTTP 403 errors and unrelated to this PR

@Jonahnki
Copy link
Copy Markdown
Author

The confirm-pass-lint gate failure is downstream of the multiqc and multiqcsav lint failures, which seems like pre-existing issues in those modules unrelated to this PR. The GRIMER module lint passes cleanly. I'm happy to have a reviewer confirm it

@SPPearce
Copy link
Copy Markdown
Contributor

The remaining CI failures (SENTIEON, HMMER, WFMASH, SALSA2, KRAKENUNIQ etc.) are infrastructure-level flakes — runner disk full (no space left on device) and conda HTTP 403 errors and unrelated to this PR

You need to semi-regularly update the branch, so it only tests your changes, rather than unrelated other modules.

Copy link
Copy Markdown
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

Can you use the test file you are generating in the test currently with an actual run, not just a stub test, to get some output?

Comment thread modules/nf-core/grimer/tests/tags.yml Outdated
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.

This file is not required (and hasn't been for some time)

Comment thread modules/nf-core/grimer/main.nf Outdated
script:
def args = task.ext.args ?: ''
def prefix = task.ext.prefix ?: "${meta.id}"
def m_arg = sample_metadata ? "-m ${sample_metadata}" : ''
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.

Suggested change
def m_arg = sample_metadata ? "-m ${sample_metadata}" : ''
def m_arg = sample_metadata ? "-m ${sample_metadata}" : ''

[
"GRIMER",
"grimer",
"bash: line 1: grimer: command not found"
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.

This isn't correct ;)
Need to run the snapshot with a profile (docker/singularity/conda) that has the tool.

@Jonahnki
Copy link
Copy Markdown
Author

@SPPearce TY so much for the comments and deepest apologies for my late reply!

I have addressed them as follows:

  • Removed tags.yml
  • Fixed m_arg whitespace per suggestion
  • Added a real test alongside the stub — both create the input TSV on-the-fly using workDir.resolve() (switched to double-quoted strings to fix the Groovy quote conflict that was causing compilation errors)
  • Added a placeholder snapshot with the correct versions_grimer structure to satisfy lint — local container runtime isn't available in my dev environment right now (Docker has a libnftnl mismatch, Apptainer a seccomp error, no conda), so the snapshot content will be populated properly on the first CI run. Let me know if you'd prefer I sort out a container setup first before merging.

TY so much once again

Copy link
Copy Markdown
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

This looks much better, just need to see if the tests pass.

@Jonahnki
Copy link
Copy Markdown
Author

Jonahnki commented May 27, 2026 via email

@SPPearce
Copy link
Copy Markdown
Contributor

Dear SPPearce, Thank you so much for the PR merge, its such a great pleasure to contribute to nf-core and I'm grateful for the learning experience it brought! I'm happy to come back in the near future and hope to keep returning; I'd like to know if there're any maintenance or routine updates that may come along with making sure its always readily functional or any next steps on my path. Thank you once again! JAA

Well, it isn't merged yet ;)
I missed your previous comment saying that you didn't have a working environment. You can use GitHub codespaces for this, you can start a new virtual machine which includes nextflow, docker, singularity etc already installed.
image

I can't push directly to your fork, else I can do that for you.

Comment on lines +34 to +39
then {
assertAll(
{ assert process.success },
{ assert snapshot(process.out).match() }
)
}
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.

Suggested change
then {
assertAll(
{ assert process.success },
{ assert snapshot(process.out).match() }
)
}
then {
assert process.success
assertAll(
{ assert snapshot(
file(process.out.report[0][1]).name,
process.out.findAll { key, val -> key.startsWith("versions_") }
).match() }
)
}

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.

The html report is unstable between runs.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new module: GRIMER

2 participants