nextflow-io / nextflow

A DSL for data-driven computational pipelines
http://nextflow.io
Apache License 2.0
2.61k stars 605 forks source link

Add Wave and Fusion info to workflow metadata #4945

Closed marcodelapierre closed 2 months ago

marcodelapierre commented 2 months ago

This PR closes https://github.com/nextflow-io/nextflow/issues/4851 by adding the following metadata information:

Checklist:

netlify[bot] commented 2 months ago

Deploy Preview for nextflow-docs-staging canceled.

Name Link
Latest commit 941f894cd51bf72d8b35f8f08e863257876e504a
Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/6634e32d2951f20008eabaa7
marcodelapierre commented 2 months ago

@pditommaso first round at tackling https://github.com/nextflow-io/nextflow/issues/4851

Do not review entirely yet.

The main caveat, as discussed with Ben inside the issue above, is that at the moment I am gathering information on Wave and Fusion from session.config, rather than WaveConfig and FusionConfig. The main issue I see with this approach (you may see others) is that I am not capturing the Fusion version if it is not explicitly requested by the user.

Thoughts? Thank you

marcodelapierre commented 2 months ago

Almost there, one last clarification, see above my reply to one outstanding review comment

marcodelapierre commented 2 months ago

Ok, ready for review.

Other than the question on NXF_DISABLE_WAVE_SERVICE, see comment above in threaded change request, I have addressed the other feedback, in particular the one on refactoring the implementation for the Fusion metadata.

pditommaso commented 2 months ago

Testing using this snippet

println "Wave enabled  : ${workflow.wave.enabled}"
println "Fusion enabled: ${workflow.fusion.enabled}"
println "Fusion version: ${workflow.fusion.version}"

Test 1: OK ✅

» ./launch.sh run test.nf 

 N E X T F L O W   ~  version 24.03.0-edge

Launching `test.nf` [nauseous_kare] DSL2 - revision: 7e81b68063

Wave enabled  : false
Fusion enabled: false
Fusion version: null

Test 2: OK ✅

» ./launch.sh run test.nf -with-wave

 N E X T F L O W   ~  version 24.03.0-edge

Launching `test.nf` [kickass_brattain] DSL2 - revision: 7e81b68063

Wave enabled  : true
Fusion enabled: false
Fusion version: null

Test 3: looks missing Fusion version ❌

» ./launch.sh run test.nf -with-wave -with-fusion

 N E X T F L O W   ~  version 24.03.0-edge

Launching `test.nf` [festering_pasteur] DSL2 - revision: 7e81b68063

Wave enabled  : true
Fusion enabled: true
Fusion version: null

Test 4: looks missing Fusion version ❌

» NXF_DISABLE_WAVE_SERVICE=true ./launch.sh run test.nf -with-wave -with-fusion

 N E X T F L O W   ~  version 24.03.0-edge

Launching `test.nf` [gloomy_kalam] DSL2 - revision: 7e81b68063

Wave enabled  : false
Fusion enabled: true
Fusion version: null
pditommaso commented 2 months ago

Think the problem is here

https://github.com/nextflow-io/nextflow/blob/add%2Fwfmeta_wave_fusion/modules/nextflow/src/main/groovy/nextflow/fusion/FusionConfig.groovy#L100-L100

it should be

        this.version = this.enabled ? retrieveFusionVersion(this.containerConfigUrl ?: DEFAULT_FUSION_AMD64_URL) : null

But at this point does not make much sense to have version as attribute and just keep this logic into version() method

pditommaso commented 2 months ago

Ok, think we are there. Let's wait the tests

pditommaso commented 2 months ago

Done!