Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ This version contains some breaking changes to the nf-schema API. See the [migra

1. Fixed an issue where the summary creation functions would fail if the default of a parameter was set in the schema, but not in the pipeline.
2. Fixed an issue where parameters with the `Path` type containing a remote file would show the wrong file path in the summary.
3. Fixed an issue where `validateParameters()` would hang, and the params summary would print an object name, when a parameter holds a `Channel` or `Value` (e.g. a typed `Channel<...>` param). The value the parameter was created from (given on the command line, otherwise set in the `params` config scope) is validated and printed in its place, also when the dataflow value is nested in a record parameter. A parameter without such a value is left out.
Comment thread
nvnieuwk marked this conversation as resolved.
Outdated

# Version 2.8.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package nextflow.validation
import static nextflow.validation.utils.Colors.getLogColors
import static nextflow.validation.utils.Common.getBasePath
import static nextflow.validation.utils.Common.getLongestKeyLength
import static nextflow.validation.utils.Common.replaceDataflowParams

import groovy.json.JsonBuilder
import groovy.util.logging.Slf4j
Expand Down Expand Up @@ -197,7 +198,7 @@ class ValidationExtension extends PluginExtensionPoint {
options,
session.workflowMetadata,
session.baseDir,
session.params
replaceDataflowParams(session.params, session.cliParams, session.config?.params)
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package nextflow.validation.parameters

import static nextflow.validation.utils.Common.replaceDataflowParams
import static nextflow.NF.isSyntaxParserV2

import static nextflow.validation.utils.Colors.getLogColors
Expand Down Expand Up @@ -124,7 +125,11 @@ class ParameterValidator {
final Map options = [:],
Session session
) {
Map<String, Object> params = initialiseExpectedParams(session.params)
Map<String, Object> params = replaceDataflowParams(
initialiseExpectedParams(session.params),
session.cliParams,
session.config?.params
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

String schemaFilename = options?.containsKey('parameters_schema') ?
options.parameters_schema as String :
config.parametersSchema as String
Expand Down
36 changes: 36 additions & 0 deletions src/main/groovy/nextflow/validation/utils/Common.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,40 @@ public class Common {
}
}

// Matched by package because the dataflow classes are not on the plugin's compile classpath
static boolean isDataflowValue(Object value) {
String className = value?.getClass()?.name ?: ''
return className.startsWith('groovyx.gpars.dataflow.') || className.startsWith('nextflow.dataflow.')
}

// Channel and Value params hold live dataflow objects: reading them blocks, and they print as object
// names. The value they were created from (given on the command line, else set in the config) is used
// in their place, and a param without one is left out. Params nested in a record (e.g. the params of an
// included pipeline) are handled the same way.
static Map replaceDataflowParams(Map params, Object cliParams, Object configParams) {
return replaceDataflowValues(params, cliParams, configParams) as Map
}
Comment thread
nvnieuwk marked this conversation as resolved.
Outdated

private static Object replaceDataflowValues(Object value, Object cliValue, Object configValue) {
if (isDataflowValue(value)) {
Object source = cliValue != null ? cliValue : configValue
return source != null && !isDataflowValue(source) ? source : null
}
if (value in Map) {
Map<Object, Object> result = [:]
(value as Map<Object, Object>).each { Object name, Object entry ->
Object replaced = replaceDataflowValues(
entry,
cliValue in Map ? (cliValue as Map)[name] : null,
configValue in Map ? (configValue as Map)[name] : null
)
if (replaced != null || !isDataflowValue(entry)) {
result[name] = replaced
}
}
return result
}
return value
}

}
53 changes: 53 additions & 0 deletions src/test/groovy/nextflow/validation/ParamsSummaryLogTest.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import nextflow.plugin.extension.PluginExtensionProvider
import org.junit.Rule
import org.pf4j.PluginDescriptorFinder
import spock.lang.Shared
import spock.lang.Timeout
import test.Dsl2Spec
import test.OutputCapture

Expand Down Expand Up @@ -112,6 +113,58 @@ class ParamsSummaryLogTest extends Dsl2Spec {
stdout ==~ /.*outdir : outDir.*/
}

@Timeout(60)
void 'should leave a param that holds a dataflow value out of the params summary'() {
given:
String schema = Path.of('src/testResources/nextflow_schema.json').toAbsolutePath()
String script = """
include { paramsSummaryLog } from 'plugin/nf-schema'
workflow {
params.outdir = 'outDir'
params.input = new groovyx.gpars.dataflow.DataflowVariable()
log.info paramsSummaryLog(parameters_schema: '${schema}')
}
"""

when:
Map opts = ['config': ['validation': ['monochromeLogs': true]]]
runScript(opts, script)
String stdout = capture

then:
noExceptionThrown()
stdout ==~ /(?s).*outdir : outDir.*/
!stdout.contains('DataflowVariable')
!stdout.contains('input ')
}

@Timeout(60)
void 'should print the config value of a param that holds a dataflow value in the params summary'() {
given:
String schema = Path.of('src/testResources/nextflow_schema.json').toAbsolutePath()
String script = """
include { paramsSummaryLog } from 'plugin/nf-schema'
workflow {
params.outdir = 'outDir'
params.input = new groovyx.gpars.dataflow.DataflowVariable()
log.info paramsSummaryLog(parameters_schema: '${schema}')
}
"""

when:
Map opts = ['config': [
'validation': ['monochromeLogs': true],
'params': ['input': 'src/testResources/correct.csv']
]]
runScript(opts, script)
String stdout = capture

then:
noExceptionThrown()
stdout ==~ /(?s).*input : src\/testResources\/correct.csv.*/
!stdout.contains('DataflowVariable')
}

void 'should print params summary - nested parameters'() {
given:
String schema = Path.of('src/testResources/nextflow_schema_nested_parameters.json').toAbsolutePath()
Expand Down
182 changes: 182 additions & 0 deletions src/test/groovy/nextflow/validation/ValidateDataflowParamsTest.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/* groovylint-disable LineLength, MethodName */
package nextflow.validation

import groovy.transform.CompileDynamic
import groovyx.gpars.dataflow.DataflowQueue
import groovyx.gpars.dataflow.DataflowVariable
import nextflow.Session
import nextflow.dataflow.ChannelImpl
import nextflow.dataflow.ValueImpl
import nextflow.validation.config.ValidationConfig
import nextflow.validation.exceptions.SchemaValidationException
import nextflow.validation.parameters.ParameterValidator
import spock.lang.Specification
import spock.lang.Timeout

import java.nio.file.Path

/**
* Validation of params that hold a dataflow value (a typed `Channel` or `Value` param), where the
* value given on the command line or in the config is validated in place of the dataflow object.
* The session is mocked so that the command line params can be set directly.
*/
@CompileDynamic
@Timeout(60)
class ValidateDataflowParamsTest extends Specification {

private static final String SCHEMA = 'src/testResources/nextflow_schema.json'
private static final String NESTED_SCHEMA = 'src/testResources/nextflow_schema_nested_parameters.json'

void 'should accept a valid command line value for a dataflow param'() {
given:
Session session = mockSession(topLevelParams(new DataflowVariable()), [input: 'src/testResources/correct.csv'], [:])

when:
validate(session, SCHEMA)

then:
noExceptionThrown()
}

void 'should reject an invalid command line value for a dataflow param'() {
given:
Session session = mockSession(topLevelParams(new DataflowVariable()), [input: 'src/testResources/correct.txt'], [:])

when:
validate(session, SCHEMA)

then:
SchemaValidationException error = thrown(SchemaValidationException)
error.message.contains('--input (src/testResources/correct.txt)')
}

void 'should validate the command line value over the config value - invalid command line value'() {
given:
Session session = mockSession(
topLevelParams(new DataflowVariable()),
[input: 'src/testResources/correct.txt'],
[input: 'src/testResources/correct.csv']
)

when:
validate(session, SCHEMA)

then:
SchemaValidationException error = thrown(SchemaValidationException)
error.message.contains('--input (src/testResources/correct.txt)')
}

void 'should validate the command line value over the config value - valid command line value'() {
given:
Session session = mockSession(
topLevelParams(new DataflowVariable()),
[input: 'src/testResources/correct.csv'],
[input: 'src/testResources/correct.txt']
)

when:
validate(session, SCHEMA)

then:
noExceptionThrown()
}

void 'should accept a valid command line value for a Value param'() {
given:
Session session = mockSession(topLevelParams(new ValueImpl(new DataflowVariable())), [input: 'src/testResources/correct.csv'], [:])

when:
validate(session, SCHEMA)

then:
noExceptionThrown()
}

void 'should reject an invalid command line value for a Value param'() {
given:
Session session = mockSession(topLevelParams(new ValueImpl(new DataflowVariable())), [input: 'src/testResources/correct.txt'], [:])

when:
validate(session, SCHEMA)

then:
SchemaValidationException error = thrown(SchemaValidationException)
error.message.contains('--input (src/testResources/correct.txt)')
}

void 'should accept a valid command line value for a Channel param'() {
given:
Session session = mockSession(topLevelParams(new ChannelImpl(new DataflowQueue())), [input: 'src/testResources/correct.csv'], [:])

when:
validate(session, SCHEMA)

then:
noExceptionThrown()
}

void 'should reject an invalid command line value for a Channel param'() {
given:
Session session = mockSession(topLevelParams(new ChannelImpl(new DataflowQueue())), [input: 'src/testResources/correct.txt'], [:])

when:
validate(session, SCHEMA)

then:
SchemaValidationException error = thrown(SchemaValidationException)
error.message.contains('--input (src/testResources/correct.txt)')
}

void 'should accept a valid command line value for a dataflow value nested in a record param'() {
given:
Session session = mockSession(
[map: [is: [so: [deep: new ValueImpl(new DataflowVariable())]]]],
[map: [is: [so: [deep: true]]]],
[:]
)

when:
validate(session, NESTED_SCHEMA)

then:
noExceptionThrown()
}

void 'should reject an invalid command line value for a dataflow value nested in a record param'() {
given:
Session session = mockSession(
[map: [is: [so: [deep: new ValueImpl(new DataflowVariable())]]]],
[map: [is: [so: [deep: 'maybe']]]],
[:]
)

when:
validate(session, NESTED_SCHEMA)

then:
SchemaValidationException error = thrown(SchemaValidationException)
error.message.contains('--map.is.so.deep')
}

private Session mockSession(Map params, Map cliParams, Map configParams) {
Session session = Mock(Session)
session.params >> params
session.cliParams >> cliParams
session.config >> [params: configParams]
session.baseDir >> Path.of('.').toAbsolutePath()
return session
}

private void validate(Session session, String schema) {
ValidationConfig config = new ValidationConfig([monochromeLogs: true], session)
new ParameterValidator(config).validateParametersMap(
[parameters_schema: Path.of(schema).toAbsolutePath().toString()],
session
)
}

private Map topLevelParams(Object input) {
return [input: input, outdir: 'src/testResources/testDir']
}

}
Loading
Loading