Fix: Add no-arg constructors for v2 config parser scope registration#362
Conversation
Nextflow's v2 syntax parser requires ConfigScope implementations to have a no-arg constructor so it can reflectively instantiate them at parse time to discover and validate config options. Without this, the co2footprint scope and its sub-scopes are not registered, causing "Unrecognized config option" warnings for all co2footprint.* settings. This adds no-arg constructors to CO2FootprintConfig and all BaseFileConfig subclasses (TraceFileConfig, SummaryFileConfig, ReportFileConfig, DataFileConfig), following the same pattern used by nf-schema's ValidationConfig. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Jonathan Manning <[email protected]>
50d7f74 to
2745da2
Compare
JosuaCarl
left a comment
There was a problem hiding this comment.
Interesting, I was not up-to-date regarding this requirement. Thanks for your addition 👍
Accidentally still tried it with v1 parser. On v2 the warning still appears.
|
There seems to be something wrong with the recognition of Until then, I would put the approval on halt to preserve the discussion, although it seems like a good alignment to the template. |
|
Apologies if I got stuff wrong! |
|
You need to add the top-level config scope to the list of extension points: Most of the ceremony is only needed for the top-level scope:
All of the nested scope classes should be referenced by fields in the top-level scope class, so they get pulled in that way |
|
Thanks Ben, really helpful review - applied all your suggestions:
|
Per Ben's review, only the top-level scope needs @ScopeName, a no-arg constructor, and extension point registration. Nested scopes are discovered via fields on the parent class. - Register CO2FootprintConfig in build.gradle extensionPoints - Remove @ScopeName from nested scope classes (Trace/Summary/Report/DataFileConfig) - Remove no-arg constructors from nested scopes and BaseFileConfig - Remove unnecessary types=[...] from @ConfigOption annotations (kept types=[Number, BigDecimal] on ci since it constructs CiRecord from a number) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Jonathan Manning <[email protected]>
ae79c64 to
80c3e23
Compare
|
@JosuaCarl hopefully this is better now. FYI the reason I'm doing this is because we use this plugin as an example in our new training: https://training.nextflow.io/0.dev/side_quests/plugin_development/01_plugin_basics/. |
|
Hi, I was on vacation for the last two weeks, so sorry for the wait. @pinin4fjords We are very excited to see our work integrated into the training, thanks :) @bentsherman With the suggested changes the top-scope warnings are gone, but all lower-level/nested scopes are still persistent: Do you have any idea how that could be addressed? |
|
@bentsherman So I take it you have no idea why the nested plugin scopes are still in the warning? |
The
I think there are two reasons:
|
# Conflicts: # src/main/nextflow/co2footprint/CO2FootprintConfig.groovy # src/main/nextflow/co2footprint/Config/DataFileConfig.groovy
Refactor: Removed nested ConfigOption annotations Signed-off-by: Josua Carl <[email protected]>
|
I tried various configurations to get nested scopes on the nested
What I don't want is to change the complete structure by eliminating I will proceed with a workaround by declaring the nested configs as a Map |
…s type annotation Signed-off-by: Josua Carl <[email protected]>
Signed-off-by: Josua Carl <[email protected]>
Signed-off-by: Josua Carl <[email protected]>
Signed-off-by: Josua Carl <[email protected]>
|
Update: In the end I deconstructed some of the |
|
@pinin4fjords I made a PR onto your fix branch (pinin4fjords#1), to preserve this PR. |
…or-2 Fix/config scope noarg constructor 2
Resolves conflicts in:
- src/main/nextflow/co2footprint/Config/BaseFileConfig.groovy
(drop @ConfigOption imports; the annotations live on each subclass now)
- src/main/nextflow/co2footprint/Logging/CustomCaptureAppender.groovy
(take dev's 26.04.0 LogObserver adaptation)
- src/testResources/{cli,observer,report}/* (take dev's calculator outputs)
- Mark file/enabled/overwrite/maxTasks final on the four nested file scopes and final on name/ending/defaultEnabled in BaseFileConfig (each is assigned exactly once in a constructor body) - Normalise whitespace: drop trailing spaces and double-space after `=` - Add CHANGELOG entry under New > Bug Fixes Signed-off-by: Jonathan Manning <[email protected]>
|
@JosuaCarl thanks for all the work fixing this up. @bentsherman as a follow-up I've had a go at writing up the rules in nextflow-io/nextflow#7098 (extensionPoints registration, nested scope discovery, the |
JosuaCarl
left a comment
There was a problem hiding this comment.
Thanks for your work on this topic and the documentation update 👍
Summary
Eliminates
WARN: Unrecognized config option 'co2footprint.*'warnings under Nextflow's v2 syntax parser (NXF_SYNTAX_PARSER=v2, the default in 26.04+).Problem
The v2 parser validates config keys by reflectively inspecting
ConfigScopeimplementations. Without proper registration the parser cannot discover theco2footprintscope, so everyco2footprint.*option triggers an "Unrecognized" warning even though the values still work at runtime.Fix (per Ben's review)
Following nf-schema's
ValidationConfigpattern, with the rules clarified across the review:CO2FootprintConfig) carries the full ceremony:@ScopeName('co2footprint')build.gradleextensionPointsCO2FootprintConfig(trace,summary,report,provenance) carry only@Description- no@ConfigOption(the parser walks fields whose type implementsConfigScope).TraceFileConfig,SummaryFileConfig,ReportFileConfig,ProvenanceFileConfig) drop@ScopeNameand the no-arg constructor.@ConfigOptionfields are not inherited. The v2 parser usesgetDeclaredFields(), soBaseFileConfig's sharedfile/enabled/overwritewere invisible. Each nested scope class now declares those fields itself;BaseFileConfigis demoted to a plain helper providingdefineFile(),defineEnabled(),defineOverwrite()utilities.types=[...]on@ConfigOptionis restricted to its intended purpose (extra accepted standard types). Removed from fields that were using it for hint purposes; kept onci(types=[Number, BigDecimal]) sinceCiRecordis constructed from a number.Test plan
./gradlew compileGroovypasses./gradlew testpasses (151/152 unit tests; the one failure isCO2PluginFullTest, an integration test that requires Linux Docker - passes on CI)NXF_VER=26.04.0 NXF_SYNTAX_PARSER=v2and a config exercising every option (top-level + all four nested scopes includingprovenance.emissionMetricsOnly, plus theClosure<Number> cpuPowerModelandPath customCpuTdpFilefields). Result: zeroUnrecognized config optionwarnings. Sanity-checked by introducing a deliberately-misnamed option, which the validator correctly flagged.