Validate and print the value of Channel and Value params instead of hanging

[pinin4fjords]

Fixes #229

Use case

A typed param can be a Channel<...> or Value<...>: a Channel<E> param takes a samplesheet path on the command line, which Nextflow loads as a channel of records (typed parameters). This exists for pipeline composition (nextflow-io/nextflow#7213, ADR), where an including pipeline gives the included one a channel instead of a file. At validation time the param is a dataflow object even though the user gave a path, and the path is what should be validated.

What changes

validateParameters() serialises session.params, and reading a dataflow object blocks forever, so a pipeline with such a param hangs. The params summary prints the object name instead of a value.

Each dataflow param is replaced by the value it was created from, which Nextflow keeps in session.config.params (command line, params file and config values). A dataflow param without one, for example one given by the dataflow of an including pipeline, is left out. Nested values (params.other_pipeline.input) are handled too. Dataflow values are detected by class (ChannelImpl, ValueImpl, the gpars channels), which the minimum Nextflow version (26.04.0) provides.

The validation does the replacement in the Map converter of the JSON generator, next to the CLI casting. The summary does not use the generator, so it calls the same helper (Common.replaceDataflowParams). The behaviour is described in docs/parameters/validation.md.

Tests

ValidateParametersTest, ParamsSummaryLogTest and a new ValidateDataflowParamsTest (mocked Session) cover a DataflowVariable, a ValueImpl and a ChannelImpl, valid and invalid values, nested values, the summary, and cast_cli_params: false. Also checked end to end with a plugin built from this branch on a Nextflow master build.

Related

#234 (nested command line values) edits the same Map converter and CHANGELOG section, so the second of the two to merge needs a rebase.

[nvnieuwk]

Thank you! This fix does make a lot of sense! I have some remarks that I would like you to take a look at first though

[nvnieuwk]

could you move this transformation to a converter in the JsonGenerator.Options section? It would make more sense to have all type conversions in the same place.

[pinin4fjords]

Converter doesn't fit well, I think. It only gets the value, not where it sits, so it can't find the matching entry in the config params, and nested params (params.other_pipeline.input) are the main case here. The only way I see to do it in the options is calling the helper from the root Map converter:

.addConverter(Map) { Map map ->
    map.is(params) ? replaceDataflowParams(map, session.config?.params) : map
}

but that clashes with the existing Map converter for the CLI casting (they'd need merging), and the summary doesn't use the generator, so it needs the helper anyway. I kept it as a step before the generator. Happy to merge them if you prefer.

[nvnieuwk]

Those converters will also convert nested values. For example the Path converter also works for a Path parameter like this: params.some_pipeline.input. This makes the implementation a bit simpler since you only need to worry about the actual conversion instead of adjusting for the structure of the value

[pinin4fjords]

Moved it into the Map converter in 123b70d, next to the CLI cast. It calls the same helper for the root map, so the nesting is still handled by the helper and not by the generator. A converter per class can't do it: the object is empty at that point (bound later, in an igniter) and the value is only in session.config.params, which we walk by key path here. The summary doesn't use the generator, so it calls the helper directly.

[pinin4fjords]

I don't really know what I'm doing here- might need your help for missing pieces if this doesn't cover them.

[nvnieuwk]

If it's empty at that point, then the validation itself will also find nothing to validate there, right? Maybe we should just skip validation of Channel objects then? 🤔

[pinin4fjords]

Answered together with your converter question here: #230 (comment)

[nvnieuwk]

Why don't you directly add a converter on a Dataflow type here instead? (I'm not too familiar with those data types but that seems the most logical to me?)

[pinin4fjords]

Honest disclaimer: this is beyond me, so I worked through it with Claude (Claude Code). This also answers your other comment about skipping these params.

A converter on the dataflow types can't do it, for two reasons. It only gets the object and its key name, not the full path, so for nested params it can't tell rnaseq.input from diffab.input. And the object itself doesn't hold the original value in a usable form: at this point a Channel/Value param is still a live dataflow object, and reading it can block. That's why the converter is on the Map: the generator sees the params map before it converts the entries, so the dataflow params can be replaced there.

The original value, such as the samplesheet path, is in session.config.params (which includes values from the CLI, params files and config), so we resolve it from there by its full path. Returning null from a converter isn't a way to remove an entry either, because null filtering happens before converters are applied.

Skipping these params wouldn't be equivalent. The original value of a Channel param is usually a file path, and that's what the schema validates (format: file-path, the samplesheet schema, etc.). Omitting it would bypass that validation, and a required param would be reported as missing.

This may get simpler if Nextflow gets a core way to give these plain values (nextflow-io/nextflow#7758; I've proposed one, also with Claude). Typed Channel/Value params from nextflow-io/nextflow#7213 aren't in a released Nextflow yet either, so I'd keep this implementation for now and revisit if that happens.

[nvnieuwk]

Thanks for the honesty and now I get why you did it like this. Fine by me to use this implementation now and convert to a more solid approach in the future. Could you add a comment that links to your PR/issue in Nextflow and this PR so we can refer back there in the future?

[pinin4fjords]

Thanks! Added in e804b6b, as a TODO on replaceDataflowParams in Common.groovy (both the validation and the summary use it), linking nextflow-io/nextflow#7758 and this PR.

I also merged master after #234 went in (8a009f9). The CLI cast is now a pass before the generator, so the dataflow replacement had to move out of the Map converter into a pass just before it, otherwise a dataflow param given on the command line would not get cast. There is a test for that in d945c9a.

[nvnieuwk]

Thank you!

[pinin4fjords]

Mais non, thank you @nvnieuwk !