Skip to content

Migrate narfmap/hashtable to topic channels - #12445

Open
xHaMMaDy wants to merge 20 commits into
nf-core:masterfrom
xHaMMaDy:narfmap_hashtable_topics_migration
Open

Migrate narfmap/hashtable to topic channels#12445
xHaMMaDy wants to merge 20 commits into
nf-core:masterfrom
xHaMMaDy:narfmap_hashtable_topics_migration

Conversation

@xHaMMaDy

Copy link
Copy Markdown
Contributor

Description

  • replace the legacy versions.yml output with a versions_narfmap tuple broadcast to topic: versions
  • use dragen-os --version 2>&1 to report the NARFMAP version
  • update the module metadata, nf-tests, and generated snapshots for the structured version output

Generated by Codex

PR checklist

Closes #12319

  • This comment contains a description of changes (with reason).
  • Tests cover the changed version output.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label.
  • Use BioConda and BioContainers if possible to fulfil software requirements.

Tests

  • nf-core modules lint narfmap/hashtable (59 passed)
  • Docker nf-test (2 passed)
  • Singularity nf-test (2 passed)
  • Micromamba nf-test using the Conda environment (2 passed)

Copilot AI review requested due to automatic review settings July 27, 2026 18:09

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread modules/nf-core/narfmap/hashtable/tests/main.nf.test Outdated
@xHaMMaDy
xHaMMaDy enabled auto-merge July 28, 2026 19:22
@xHaMMaDy
xHaMMaDy requested a review from mashehu July 28, 2026 19:23
Comment thread modules/nf-core/narfmap/align/tests/main.nf.test Outdated
@xHaMMaDy
xHaMMaDy requested a review from mashehu July 29, 2026 12:41
Comment thread modules/nf-core/narfmap/align/meta.yml Outdated
type: string
description: The name of the tool
- samtools version | sed '1!d;s/.* //':
- samtools version | sed '1!d;s/.* //;s/^1[.]23[.]1\$/1.23/':

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.

What is this for?

@LouisLeNezet LouisLeNezet left a comment

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.

Hi @xHaMMaDy,
Would you have some time to finish this PR ?

Comment thread modules/nf-core/narfmap/align/tests/main.nf.test Outdated
Comment thread modules/nf-core/narfmap/align/tests/segfault.nf.test Outdated
{ assert snapshot(
process.out.bam,
process.out.bam.collect { meta, bamFile -> [meta, file(bamFile).name, file(bamFile).exists(), file(bamFile).size() > 0] },
file(process.out.log[0][1]).readLines().findAll { it.startsWith("decompHash") },

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 line should be as a separate assertion outside the snapshot.

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.

I meant

            assertAll (
                { assert process.success },
                { assert file(process.out.log[0][1]).readLines().findAll { it.startsWith("decompHash") } },
                { assert snapshot(
                    process.out.bam.collect { meta, bamFile -> [meta, file(bamFile).name, bam(bamFile).getReadsMD5()] },
                    process.out.findAll { key, val -> key.startsWith("versions") },
                ).match() }
            )

Comment thread modules/nf-core/narfmap/align/main.nf Outdated

@LouisLeNezet LouisLeNezet left a comment

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.

You just need now to update the meta and snapshot

{ assert snapshot(
process.out.bam,
process.out.bam.collect { meta, bamFile -> [meta, file(bamFile).name, file(bamFile).exists(), file(bamFile).size() > 0] },
file(process.out.log[0][1]).readLines().findAll { it.startsWith("decompHash") },

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.

I meant

            assertAll (
                { assert process.success },
                { assert file(process.out.log[0][1]).readLines().findAll { it.startsWith("decompHash") } },
                { assert snapshot(
                    process.out.bam.collect { meta, bamFile -> [meta, file(bamFile).name, bam(bamFile).getReadsMD5()] },
                    process.out.findAll { key, val -> key.startsWith("versions") },
                ).match() }
            )

@xHaMMaDy

xHaMMaDy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Pushed an update for the align test snapshots.

meta.yml already mirrors the simplified Samtools eval from main.nf.

I also tested the exact getReadsMD5() form locally for the segfault regression fixture, but it fails inside HTSJDK because the emitted BAM contains an invalid sequence name:

htsjdk.samtools.SAMException: Sequence name ... doesn't match regex

So for this specific malformed-BAM regression test, I moved the decompHash log check outside the snapshot, kept the BAM existence/non-empty checks outside the snapshot, and snapshot only the BAM meta/name plus versions. The normal BAM tests still use getReadsMD5().

Local checks:

  • narfmap/align/tests/segfault.nf.test passes with Docker and Nextflow 25.10.2
  • narfmap/align/tests/main.nf.test --update-snapshot passed and updated the normal BAM snapshots
  • one full targeted local run hit a transient raw.githubusercontent.com staging timeout, and the isolated retry of that same shard passed

Comment added by Codex

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.

Migrate to topic narfmap/hashtable

5 participants