Convert a typed process input to its declared type in process entry mode - #7384
Open
pditommaso wants to merge 1 commit into
Open
Convert a typed process input to its declared type in process entry mode#7384pditommaso wants to merge 1 commit into
pditommaso wants to merge 1 commit into
Conversation
`nextflow run` in process entry mode (and `nextflow module run`) mapped a param to a typed (V2) process input by parsing the text into "whatever it looks like" rather than into the type the process declared. Consequences: - A `Double` input was not handled at all -- neither branch matched, so the raw String reached the task: `--completeness_pct 96.4` on a `completeness_pct: Double` input logs `invalid argument type at index 1 -- expected a Double but got a String` and the task body receives a String. - A `Float` input given integral text got an Integer (the integer branch is tried first for both Integer and Float), so the same warning fires. - A non-string value supplied programmatically was passed through untouched: a JSON number is a BigDecimal for `96.4` and an Integer for `40`, so a `Double` input receives the wrong Number. - An invalid value was accepted silently: `--n50 abc` on an `Integer` input passed the String `abc` to the task. Convert the value to the DECLARED type instead. Numeric types go through a single path that parses text as a decimal number (Groovy's cast does not parse strings into numbers) and then casts to the declaration; `String`/`Character` use a plain cast. A value that cannot be converted now fails up front with the same message the record/collection path already produces, rather than reaching the task as the wrong type. An integral declared type rejects fractional text (`--n50 3.7` on an `Integer`) instead of silently truncating it. Boolean keeps accepting only the explicit `true`/`false` spellings (never Groovy truthiness) and Path keeps resolving with the existence check; both now also accept an already-typed value, which previously short-circuited earlier. Verified end to end: on master, `nextflow run qc.nf --n50_kb 45 --completeness_pct 96.4` against a typed process warns `expected a Double but got a String`; with this change the warning is gone and the task receives a Double. Tests: 12 conversion cases, 4 rejection cases and 5 boolean cases in ProcessEntryHandlerTest (15 of them fail without this change). The nextflow.script.*, nextflow.module.*, nextflow.cli.module.* and nextflow.processor.* suites show the same 7 pre-existing failures as an untouched master (CmdModuleRunTest 3, RegistryClientFactoryTest 2, TaskInputResolverTest 1, BaseScriptTest 1). Assisted-by: Claude Code (Opus 5) Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
✅ Deploy Preview for nextflow-docs canceled.
|
Member
|
I think we need to write out the mapping rules for CLI params -> Nextflow types as part of #7208 . Then we can just test them all as in this PR. I also want to see if I can unify the params mapping logic for I will try to finalize an ADR and test suite as part of #7208 before we make any more quick fixes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In process entry mode (
nextflow run script.nf --param valuewith no workflow, andnextflow module run), a param is mapped to a typed (V2) process input by parsing the text into whatever it looks like rather than into the type the process declared. On current master:Four distinct failures in
ProcessEntryHandler.getValueForInputV2:Double"96.4"(CLI text)String— no branch handlesDoubleat allFloat"40"Integer— the integer branch is tried first forIntegerandFloatDouble96.4(aBigDecimal, e.g. a value supplied programmatically)BigDecimal— non-CharSequencevalues return early, unconvertedInteger"abc"String, silently — the task receivesabcThe
type == Floattest appears twice (ProcessEntryHandler.groovy:387,393) andDoublenever appears, which is the root of the first two.Fix
Convert the value to the declared type:
String/Characteruse a plain cast.Parameter--n50with type Integer cannot be assigned to abc [String]) instead of reaching the task as the wrong type.--n50 3.7on anInteger) rather than silently truncating.Booleanstill accepts only the explicittrue/falsespellings — never Groovy truthiness, so--flag yesis passed through unchanged as before — andPathstill resolves with its existence check. Both now also accept an already-typed value, which previously short-circuited on thevalue !instanceof CharSequenceearly return.The V1 (untyped) path and its
meta.yml-driven coercion are untouched.Verification
End to end, same command on both trees:
Tests —
ProcessEntryHandlerTestgains 21 cases (12 conversions, 4 rejections, 5 boolean/path). 15 of them fail without the production change; all 50 pass with it.Regression sweep —
nextflow.script.*,nextflow.module.*,nextflow.cli.module.*,nextflow.processor.*: 1130 tests with the same 7 pre-existing failures as an untouched master, measured on both trees (CmdModuleRunTest3,RegistryClientFactoryTest2,TaskInputResolverTest1,BaseScriptTest1). No class got worse.Note on the behaviour change
Two inputs that previously reached the task as the wrong type now raise an error instead:
--n50 abcon anIntegerinput (previously the Stringabcwas passed to the task)--n50 3.7on anIntegerinput (previously the String3.7was passed)Both were user errors that surfaced later as a confusing warning or a task failure, so failing up front with the param name and the declared type seems the better contract — but say the word if you'd rather keep them lenient.
🤖 Generated with Claude Code