Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New kma module #7251

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

New kma module #7251

wants to merge 33 commits into from

Conversation

Krannich479
Copy link
Contributor

@Krannich479 Krannich479 commented Dec 19, 2024

PR checklist

Closes PR #7242

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • 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.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda

@Krannich479 Krannich479 added the new module Adding a new module label Dec 19, 2024
@Krannich479 Krannich479 self-assigned this Dec 19, 2024
@Krannich479 Krannich479 added the WIP Work in progress label Dec 19, 2024
Copy link
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

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

Looks very good already. You are missing the snapshot. Can you run
nf-core modules test kma/kma and push the .snap file?

modules/nf-core/kma/kma/main.nf Outdated Show resolved Hide resolved
modules/nf-core/kma/kma/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/kma/kma/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/kma/kma/tests/nextflow.config Outdated Show resolved Hide resolved
@Krannich479
Copy link
Contributor Author

Looks very good already. You are missing the snapshot. Can you run nf-core modules test kma/kma and push the .snap file?

Thanks for reviewing! nf-test is still WIP. In fact, I need my KMA_INDEX module merged to nf-core first in order to test the KMA_KMA module because I'd like to avoid pushing tool-specific testdata to nf-core. I am aiming for a nf-test similar to bwa mem, like this.

Copy link
Contributor

@mazzalab mazzalab left a comment

Choose a reason for hiding this comment

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

one stylistic hint and a question about the module folder structure

modules/nf-core/kma/kma/main.nf Outdated Show resolved Hide resolved
modules/nf-core/kma/kma/tests/main.nf.test Show resolved Hide resolved
@Krannich479
Copy link
Contributor Author

Thanks @mazzalab , good catch about the Harshil Alignment style. Even though it hurts my eyes 😉 I'll adhere to the style with the next fix.

@Krannich479
Copy link
Contributor Author

Error: nf-test produces no output

⚠️ I am getting an error, local on my machine as well as here via GitHub CI, stating that the test script has a syntax error and produces no output.

$nf-test test --profile singularity modules/nf-core/kma/kma/

🚀 nf-test 0.9.2
https://www.nf-test.com
(c) 2021 - 2024 Lukas Forer and Sebastian Schoenherr


Test Process KMA_KMA

  Test [9a676bf1] 'sarscov2 - fasta - sparse' Assertion failed:

assert process.success
       |       |
       KMA_KMA false

java.lang.RuntimeException: Process has no output channels. process.out can not be used.
FAILED (3.227s)

  Assertion failed:

  2 of 2 assertions failed

  Nextflow stdout:

  ERROR ~ Script compilation error
  - file : /foo/bar/nf-core-modules/.nf-test-9a676bf15be8a667a7c56d0363f0eb1f.nf
  - cause: Unexpected input: '{' @ line 26, column 10.
     workflow {
              ^

  1 error

  NOTE: If this is the beginning of a process or workflow, there may be a syntax error in the body, such as a missing or extra comma, for which a more specific error message could not be produced.

   -- Check '/foo/bar/nf-core-modules/.nf-test/tests/9a676bf15be8a667a7c56d0363f0eb1f/meta/nextflow.log' file for details
  Nextflow stderr:

  curl: (22) The requested URL returned error: 403

FAILURE: Executed 1 tests in 3.239s (1 failed)

My goal is to reproduce the same logic as with the nf-tests of bwa index and bwa mem (see here for comparison), i.e. the test is supposed to create an index with the nf-core/kma/index module and the test the kma/kma alignment module.

🚧 What I investigated so far:

  1. The prerequisite, the kma_index module, works as intended i.e. produces the an index repository, was tested successfully, and got merged into nf-core lately.
  2. The mock.nf script body that allegedly has a syntax error seems fine from what I can tell
workflow {

    // run dependencies
    
    {
        def input = []
        
                input[0] = [
                    [ id: 'MT192765.1', single_end:false ],
                    file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true)
                ]
                
        KMA_INDEX(*input)
    }
    

    // process mapping
    def input = []
    
                input[0] = [
                    [ id:'nfcore_testsample', single_end:false ],
                    [
                        file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test_1.fastq.gz', checkIfExists: true),
                        file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test_2.fastq.gz', checkIfExists: true)
                    ]
                input[1] = KMA_INDEX.out.index
                input[2] = false
                input[3] = false
                ]
                
    //----

    //run process
    KMA_KMA(*input)

    if (KMA_KMA.output){

        // consumes all named output channels and stores items in a json file
        for (def name in KMA_KMA.out.getNames()) {
            serializeChannel(name, KMA_KMA.out.getProperty(name), jsonOutput)
        }	  
      
        // consumes all unnamed output channels and stores items in a json file
        def array = KMA_KMA.out as Object[]
        for (def i = 0; i < array.length ; i++) {
            serializeChannel(i, array[i], jsonOutput)
        }    	

    }
  
}

❓ Has anyone a clue or pointer what is going on here? Where should I continue to investigate? Any help much appreciated.

@Krannich479 Krannich479 requested a review from famosab January 14, 2025 15:43
@Krannich479
Copy link
Contributor Author

Krannich479 commented Jan 15, 2025

Hi @jfy133 , cc @MarieLataretu ,
like you advised me last time, I tried the "proof via gitpod" again, because we cannot use docker at my facility. Interestingly (alias frustratingly), it is only docker that crashes. Here is my nf-test results of the latest commit:

GitHub CI:
🔴 Docker
🟢 Singularity
🟢 Conda

Local:
🟡 Docker (cannot test here)
🟢 Singularity
🟢 Conda

Gitpod:
🔴 Docker
🟢 Singularity
🟢 Conda

This suggests that there is something going on with the docker container, right? The screenshots attached are from gitpod. For conda and singularity outputs I even checked the files manually and it's all correct. The error using docker (on gitpod) is the same you posted above.

kmakmaDOCKER
kmakmaCONDA
kmakmaSINGU

@famosab
Copy link
Contributor

famosab commented Jan 16, 2025

It is jus the sarscov - fasta and the sarscov - fasta - matrix tests which are failing.

   Test [726e4ada] 'sarscov2 - fasta - sparse' 
    > Nextflow 24.10.3 is available - Please consider updating your version to it
    > N E X T F L O W  ~  version 24.10.2
    > Launching `/home/runner/work/modules/modules/.nf-test-726e4ada36f370274ea46e18c45b05ca.nf` [sharp_pauling] DSL2 - revision: 3d9dd9af05
    > WARN: Unknown directive `params` for process `KMA_INDEX`
    > WARN: Unknown directive `params` for process `KMA_KMA`
    > [c0/c5bf5b] Submitted process > KMA_INDEX (MT192765.1)
    > [97/474896] Submitted process > KMA_KMA (nfcore_testsample)
    PASSED (10.318s)

works for example.

@Krannich479 I would try a wave container and check if that works. We will swap to those anyway (I think). https://seqera.io/containers/

@Krannich479
Copy link
Contributor Author

Krannich479 commented Jan 20, 2025

Exactly, @famosab it is only 2/6 tests failing. That's two too many though :)
I'll try to use wave container for Docker as engine of choice.

@Krannich479
Copy link
Contributor Author

Dear @famosab,
I am quite inexperienced with wave containers in practice, so I tried to mimic something similar to flye. Container build below. I used Docker and linux/amd64 to build the wave container, tested it in Gitpod, still the same error 🔴. Error log indicates that wave container was used. Did I set it up and used it right?

kma_wave
kma_wave_error

use wave container if docker is engine
@Krannich479
Copy link
Contributor Author

Krannich479 commented Jan 20, 2025

a3bd776 was my attempt to use this wave container. In Gitpod, the --docker run failed as before. For comparison, --singularity still succeeds.

@famosab
Copy link
Contributor

famosab commented Jan 21, 2025

When I run it locally everything works:

  nf-core-3.1   famke@Famkes-MacBook-Air  ~/Desktop/nf-core-modules   kma_kma ±  nf-core modules test kma/kma --profile docker -u                                                                                                                                                                                   [11:17AM]


                                          ,--./,-.
          ___     __   __   __   ___     /,-._.--~\ 
    |\ | |__  __ /  ` /  \ |__) |__         }  {
    | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                          `._,._,'

    nf-core/tools version 3.1.1 - https://nf-co.re
    There is a new version of nf-core/tools available! (3.1.2)


INFO     Generating nf-test snapshot                                                                                                                                                                                                                                                                                               
╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── nf-test output ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                                                                                                                                                                                                                                 │
│ 🚀 nf-test 0.9.2                                                                                                                                                                                                                                                                                                                │
│ https://www.nf-test.com                                                                                                                                                                                                                                                                                                         │
│ (c) 2021 - 2024 Lukas Forer and Sebastian Schoenherr                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│ Load .nf-test/plugins/nft-bam/0.5.0/nft-bam-0.5.0.jar                                                                                                                                                                                                                                                                           │
│ Load .nf-test/plugins/nft-compress/0.1.0/nft-compress-0.1.0.jar                                                                                                                                                                                                                                                                 │
│ Load .nf-test/plugins/nft-vcf/1.0.7/nft-vcf-1.0.7.jar                                                                                                                                                                                                                                                                           │
│ Warning: every snapshot that fails during this test run is re-record.                                                                                                                                                                                                                                                           │
│                                                                                                                                                                                                                                                                                                                                 │
│ Test Process KMA_KMA                                                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│   Test [5f1111b2] 'sarscov2 - fasta - sparse' PASSED (6.179s)                                                                                                                                                                                                                                                                   │
│   Test [aa06bd01] 'sarscov2 - fasta - sparse - stub' PASSED (6.601s)                                                                                                                                                                                                                                                            │
│   Test [6997155d] 'sarscov2 - fasta' PASSED (6.623s)                                                                                                                                                                                                                                                                            │
│   Test [866eb651] 'sarscov2 - fasta - stub' PASSED (5.553s)                                                                                                                                                                                                                                                                     │
│   Test [48ec76fa] 'sarscov2 - fasta - matrix' PASSED (6.075s)                                                                                                                                                                                                                                                                   │
│   Test [6bfbba61] 'sarscov2 - fasta - matrix - stub' PASSED (6.385s)                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│                                                                                                                                                                                                                                                                                                                                 │
│ SUCCESS: Executed 6 tests in 37.461s                                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
INFO     Generating nf-test snapshot again to check stability                                                                                                                                                                                                                                                                      
╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── nf-test output ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                                                                                                                                                                                                                                 │
│ 🚀 nf-test 0.9.2                                                                                                                                                                                                                                                                                                                │
│ https://www.nf-test.com                                                                                                                                                                                                                                                                                                         │
│ (c) 2021 - 2024 Lukas Forer and Sebastian Schoenherr                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│ Load .nf-test/plugins/nft-bam/0.5.0/nft-bam-0.5.0.jar                                                                                                                                                                                                                                                                           │
│ Load .nf-test/plugins/nft-compress/0.1.0/nft-compress-0.1.0.jar                                                                                                                                                                                                                                                                 │
│ Load .nf-test/plugins/nft-vcf/1.0.7/nft-vcf-1.0.7.jar                                                                                                                                                                                                                                                                           │
│                                                                                                                                                                                                                                                                                                                                 │
│ Test Process KMA_KMA                                                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│   Test [5f1111b2] 'sarscov2 - fasta - sparse' PASSED (7.046s)                                                                                                                                                                                                                                                                   │
│   Test [aa06bd01] 'sarscov2 - fasta - sparse - stub' PASSED (6.746s)                                                                                                                                                                                                                                                            │
│   Test [6997155d] 'sarscov2 - fasta' PASSED (6.048s)                                                                                                                                                                                                                                                                            │
│   Test [866eb651] 'sarscov2 - fasta - stub' PASSED (6.199s)                                                                                                                                                                                                                                                                     │
│   Test [48ec76fa] 'sarscov2 - fasta - matrix' PASSED (7.007s)                                                                                                                                                                                                                                                                   │
│   Test [6bfbba61] 'sarscov2 - fasta - matrix - stub' PASSED (6.495s)                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
│                                                                                                                                                                                                                                                                                                                                 │
│ SUCCESS: Executed 6 tests in 39.596s                                                                                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                                                                                                                 │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
INFO     All tests passed!   

I changed the nf test file a little though.

@Krannich479 Krannich479 removed help wanted Extra attention is needed WIP Work in progress labels Jan 21, 2025
@Krannich479 Krannich479 dismissed mazzalab’s stale review January 21, 2025 12:48

Module works now according to nf-tests

@Krannich479
Copy link
Contributor Author

The secret ingredient is to rerun the tests in Github CI until it works for some reason....

@famosab
Copy link
Contributor

famosab commented Jan 21, 2025

Sorry to disappoint but to me it seems like the nf-test is not run at all now 👀 @mashehu Are there recent changes that stopped the runners or something?

@mashehu
Copy link
Contributor

mashehu commented Jan 21, 2025

hmm, something strange going on in the nf-test detection https://github.com/nf-core/modules/actions/runs/12884972870/job/35931026117?pr=7251#step:7:80

@mashehu
Copy link
Contributor

mashehu commented Jan 21, 2025

fix is here: #7340

@Krannich479 Krannich479 requested a review from mashehu January 21, 2025 14:19
@famosab
Copy link
Contributor

famosab commented Jan 21, 2025

I think the issue is related to this warning:

WARNING: Your kernel does not support CPU shares or the cgroup is not mounted. Shares discarded.

@mashehu Could that be related to the runners? (And thank you for the fix)

@mashehu
Copy link
Contributor

mashehu commented Jan 21, 2025

nope that warning is a red herring (as it is just a warning and raised in all runs, not just failing ones).

@sateeshperi any idea why docker throws a 95 status error here?

@Krannich479 Krannich479 added the help wanted Extra attention is needed label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed new module Adding a new module nf-test
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants