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

Validate config schema #5816

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

Validate config schema #5816

wants to merge 5 commits into from

Conversation

bentsherman
Copy link
Member

Redo of #4201

This PR uses the config scope classes (in the new compiler module) to validate the config at runtime, for the config and run commands.

Validation occurs after plugins are loaded, and the validator uses the plugin system to collect all implementations of the ConfigScope interface, so plugins can use this interface to declare their config options to Nextflow.

I suggest we merge this in an upcoming release and advise plugin developers to implement this interface before 25.04, to make the transition to next stable as smooth as possible.

This work is also a prerequisite to recognizing third-party plugin config in VS Code.

Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman requested a review from a team as a code owner February 25, 2025 21:56
Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit 7576572
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/67c0b4a811a54600080a9982
😎 Deploy Preview https://deploy-preview-5816--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman mentioned this pull request Feb 25, 2025
5 tasks
@bentsherman bentsherman marked this pull request as draft February 26, 2025 01:34
@bentsherman
Copy link
Member Author

Looking deeper into the failing unit test:

params.foo = 'baz'

profiles {
    test {
        params.foo = 'foo'
        profiles {
            debug {
                cleanup = false
            }
        }
    }
}

the test expects the following resolved config:

params.foo = 'baz'

profiles {
   test {
      params.foo = 'foo'
   }
   debug {
      cleanup = false
   }
}

in other words, the nested profiles block is actually merged with the top profiles block, producing two profiles test and debug.

However, while it works with nextflow config, it doesn't work with nextflow run:

$ nextflow run hello -profile debug

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

Unknown configuration profile: 'debug'

I think this is an artifact of the ConfigSlurper on which the v1 config parser was based. It was never a documented feature, and the v2 config parser doesn't support it. I think it just makes configs harder to interpret anyway, if profiles can modify other profiles.

The v2 config parser doesn't support nested profiles, so it just warns about a spurious config option:

$ NXF_SYNTAX_PARSER=v2 ./build/releases/nextflow-25.01.0-edge-dist config -a 
WARN: Unrecognized config option 'profiles.debug.cleanup'
params.foo = 'baz'

profiles {
   test {
      params.foo = 'foo'
      profiles {
         debug {
            cleanup = false
         }
      }
   }
}

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Comment on lines -2 to +1
foo = 'bar'
outputDir = 'results'
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to pick some valid config option here. Alternatively we could leave the invalid options and include the warnings for them in the expected stdout

@bentsherman bentsherman removed the request for review from tom-seqera February 27, 2025 19:01
@bentsherman bentsherman marked this pull request as ready for review February 27, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants