Repository navigation
refactor(nextflow): lower from a resolved view of the graph - #583
Conversation
3b9d7e9 to
7710ab6
Compare
6e2d9bb to
c0001d6
Compare
The resolved view of the graph; partial failure as one CWL policy that keeps an authored when; step-only inputs and inherited requirements on Nextflow; checks before writing. The backend design's §2, §3, §4 and §6 follow it.
Its parse, type check, canonical form and source text leave nf_expr, which keeps the Groovy rendering and reading.
testkit compiles each case against a registry of its tools and lowers the result. Step-to-step sources are edges; cases no compiled workflow reaches leave.
8c4ec36 to
ba07f58
Compare
darkclad
left a comment
There was a problem hiding this comment.
I checked the byte-identity claim by compiling all 36 .wic files under docs/tutorials and docs/cookbook with --target nextflow on the merge base and on this head. All 136 generated files are identical, the exit codes match, and every refusal from base is still there, now with a line and step. The lang.subset move is a faithful move, and the import-linter contracts pass. The one red CI job, Run Workflows, failed on quay.io timeouts during cwl-docker-extract, which has nothing to do with this change.
I left three comments inline. The first is a behavior change rather than a text change: an output the lowering used to refuse is now dropped without a refusal.
| if given is not None and _plain(given): | ||
| child_env[port.name] = through(given.sources[0]) | ||
| elif given is not None or port.declaration.has_default: | ||
| child_env[port.name] = (Crossing(step.name, port.name),) |
There was a problem hiding this comment.
When a call leaves a nested input unbound and the input has no default, the input gets no entry in child_env. through() then resolves it to () (line 481). In _workflow_output_connections, a root output whose outputSource relays that input gets no connection and no refusal, so it is dropped from the Nextflow workflow without a word.
Repro: a nested child.wic declares extra: 'File?' and passed: {type: 'File?', outputSource: extra}, and the call binds only message. The CWL target emits the root output child~passed. output_sources(view) gives () for it, and NEXTFLOW.lower succeeds with only child_out and child_say_out. nextflow_names(view) still lists child~passed. Base refused the same workflow (resolved edge names non-executable source step ... 'child.wic'). That message was poor, but it was a refusal.
Two possible fixes: give an unbound, no-default input a Crossing, or refuse a kept output whose resolved sources are empty. Either restores the refusal, now at the output's line.
There was a problem hiding this comment.
Fixed in cadb0cd and 852cfc2, and it is now a refusal again, at the line that causes it.
The resolver records each miss where it happens: an input that a call leaves unbound and that has no default. output_unbound, beside output_sources, gives the misses each root output's resolution reached. The lowering refuses every kept output that resolves to nothing, once per unbound input, at the call that leaves it unbound:
step 'child': input 'extra': a workflow output relays it, but nothing binds it and it has no default
rejected/calls.yaml covers your repro, the same relay one call deeper, and a chain that reaches the output through another call's output. Tool-step inputs still resolve as before, and the 36 docs .wic files compile identically to master.
| run: | | ||
| python -m pytest -k test_cwl_docker_extract | ||
| python -m pytest -m "not skip_pypi_ci and not nextflow" -k "not test_cwl_docker_extract" -vv | ||
| python -m pytest -m "package and not nextflow and not skip_pypi_ci" -vv |
There was a problem hiding this comment.
Before this change, this job ran -m "not skip_pypi_ci and not nextflow", nearly the whole suite. It was the only PR lane for about 94 test functions, which now run only in the weekly lanes. Most of them are fast core tests: test_lang_parser.py (32), test_lang_schema.py (11), test_names_emitted.py (10), test_lang_render.py (9), test_canonical_path.py and test_reference_compatibility.py (6 each), and test_leak_boundary.py (5). The corpus parse and round-trip checks over the in-repo tutorials (test_corpus_files_parse, test_corpus_round_trips, test_accepts_the_whole_corpus) are now marked cwl_corpus, but neither Run Workflows nor Lint And Test collects them. Only Property Weekly runs them. So the new cwl_corpus marker text in pyproject.toml and the header of python_matrix_weekly.yml overstate where they run.
After this change, a parser, schema or emitted-name regression passes PR CI and shows up only in the weekly run. Could these fast files keep a PR lane, either by marking them package or by giving them a Lint And Test step?
There was a problem hiding this comment.
You're right that these no longer gate PRs. Putting all 94 back as they are isn't the fix, though. About 35 of them are already properties, and most of the rest are examples written before the property in the same file covered them, or checks on how a module is built rather than what it does. test_parse_case alone runs 137 inline snippets. A follow-up on top of this stack folds them into about 25 tests, one property per rule plus a few readable .wic examples in files for the diagnostics and the emitted names, and those gate PRs again. Until it lands they run in the weekly lanes.
On cwl_corpus: CORPUS (tests/core/wic_corpus.py) is whatever search_paths_wic reaches. That is the same discovery test_run_workflows_on_push runs, and in CI it covers mm-workflows, image-workflows and docs/tutorials. The in-repo fallback is only what Property Weekly sees, because it has no config. So the marker is right. You're also right that no PR lane collects these three: Run Workflows picks its tests by name. The follow-up makes them one corpus test.
| _lower(workflow, run) | ||
| _position(cwl_class)(workflow, run)[name] = "x" | ||
| refused = _refused(workflow, run, FIELDS[cwl_class][name].note) | ||
| assert [(refusal.construct in _constructs(cwl_class, name), name in f"{refusal.subject} {refusal.reason}") |
There was a problem hiding this comment.
This assertion is looser than the one it replaces. Base matched the exact path plus the note (- steps[0].run.inputs.x.secondaryFiles: <note>). Here, _refused already keeps only the refusals whose reason ends with the note, and the notes contain the field name, so name in f"{subject} {reason}" is true for almost every row. That leaves only the construct check, and tool covers both input and output secondaryFiles. A refusal on the wrong port or class would still pass. Could this pin refusal.subject the way the NetworkAccess and requirement-class tests below do, for example T.cwl: inputs.x.secondaryFiles?
There was a problem hiding this comment.
Fixed in 0b58b86. The test now asserts refusal.subject exactly, the way the NetworkAccess and requirement-class tests do: the port for a workflow-side class, and the path in the tool otherwise, e.g. T.cwl: inputs.x.secondaryFiles. A refusal moved to outputs.o now fails three rows that the old assertion passed.
ir.resolved reads a finished graph and its tool documents; compiler.resolved_view builds it from a result, and tool_steps resolves sources through calls. A test holds it to today's projection.
Every workflow-level analysis reads view constructs, not emitted CWL. The command line, run backend and Python API pass run values and kept outputs through the view.
A refusal names the step, construct and subject at the span its author wrote; workflow ports carry their spans. nf_support.CONSTRUCTS joins each construct to its fields.
CompiledNextflowSource, compilation_result_source and the agreement test go. Two import-linter contracts keep the target on the view and the view on the graph.
Package runs the tests marked package and the Nextflow tests that do not run Nextflow. cwl_corpus marks the tests that run or read mm-workflows and image-workflows; Python Matrix Weekly runs the rest of the suite, minus nextflow_corpus and skip_pypi_ci, on 3.11-3.14.
ba07f58 to
6bce14c
Compare
|
Thanks for checking. The inline points are answered in their threads: the dropped output is a refusal again, at the call that leaves the input unbound; the support-table rows assert the exact subject; and the fast core tests will gate PRs again through a follow-up that shrinks them first. Run Workflows on the new head is the run to read. |
darkclad
left a comment
There was a problem hiding this comment.
LGTM. My repro of the dropped output is now refused at the call, at wf.wic:4:3: step 'child': input 'extra': …, and the support-table rows pin the exact subject. On 6bce14c the 36 docs .wic files still give Nextflow output byte-identical to master. The Nextflow tests that don't run Nextflow pass locally (1283), and CI is green. I'll look for the follow-up that puts the fast core tests back on a PR lane.
What
sophios.ir.resolved, a read-only view of the compiled graph, instead of a copy of the CWL Sophios emitted. The view resolves what the graph already says (each step input's sources and the type it delivers, the parsedwhenandscatter, workflow inputs and outputs, the run policy) and gives none of it a meaning of its own. The CWL target does not use it and is unchanged.docs/cookbook/when.wicwith--target nextflow:- before:
- after:
From Python the line is in the script. An input Sophios made for a step is refused at the line that gave it its value, asscatter_pyapi.py:13:1: step 'array_indices': ….nf_exprtolang.subset, since the view types awhenwith it.nf_exprkeeps the Groovy rendering and reading.compile_sourceagainst their own tools, so each case is a workflow someone could write.type: string[]withoutputEval: $(self[0].contents)) crashed withTypeError. It is refused at its line now: one string cannot fill astring[].Deleted
CompiledNextflowSourceandcompilation_result_source, with the analyses that read CWL fields back out of it. They rebuilt the workflow from a deep copy of the emitted CWL, which is why refusals named CWL paths. The agreement test that held the view to the projection goes with it; two import-linter contracts keep the target off emitted CWL and the view on the graph.CI
test_build_wheel's package job runs only what no other pull-request lane runs: the tests markedpackageand the Nextflow tests that do not run Nextflow. Tests that only weekly lanes run no longer gate a pull request.cwl_corpusmarker names the tests that run or read the CWL corpus (mm-workflows and image-workflows). Python Matrix Weekly runs the rest of the suite on 3.11-3.14, leaving outcwl_corpus,nextflow_corpusandskip_pypi_ci.test_ci_coverage.pycounts pull-request and weekly lanes, not the release lane, so a test no such lane runs fails it.What changes for users
Only refusal text. Every generated Nextflow file and all emitted CWL are byte-identical over the docs examples, the test cases and both corpora; CI does not check that, so I did. The Python API, the command line and
.wicare unchanged.Why
The Nextflow target worked backwards from the CWL Sophios wrote, so its refusals pointed at CWL nobody wrote, and every new target would have had to do the same. The view is the one place where a compiled workflow is read, for every target but CWL.