Add config-declared process dependencies and DAG execution (#323) - #326
Conversation
Config-declared process steps had no way to depend on each other, so the extras work in flepimop2-extras #16 and #22 would each have had to invent ordering and cache-skipping for themselves. This settles the core contract those can build on. The issue left three questions open; the answers here are proposals, and each is the least-committal option I could find that still supports the motivating fetch -> transform pipeline. 1. Where is `depends` declared? On the process configuration entry, naming sibling keys of the `process:` section, and typed as a field on ProcessABC so it validates and documents itself. No new config section and no separate run-plan concept. ModuleBase is extra="allow", so the key already flowed through untyped; this makes it explicit. 2. Are targets opaque identifiers or paths? Opaque. `depends` names steps, never files. Core orders steps and never learns what any of them produces, which keeps filesystem semantics in the providers where the issue wanted them. 3. Does core own topological execution only, or also freshness and provenance? Execution only. A step answers `is_satisfied()` for itself and core simply skips it; core never inspects a target, because only the module knows what it produces and what counts as fresh. The `depends` metadata is preserved for the status and provenance reporting the issue anticipates, but nothing here interprets it. What that buys: - resolve_plan() validates every reference, rejects self-dependencies and cycles, and returns steps in dependency order. Problems are collected and reported together rather than one per run, and validation reads the raw config, so a typo in a `depends` name does not require importing every module in the section first. - Ties are broken by configuration order, so a plan is reproducible between runs and reads the way the file does. - Asking for one step plans that step plus its transitive dependencies, so `flepimop2 process --target transform` fetches first. - execute(force=) gives a target the three states the issue names: absent, present, present-with-force. Only the step actually requested is forced; dependencies keep their own skip behaviour, so forcing a transform does not re-download its inputs. Verified: 596 tests pass, 19 of them new, plus ruff format, ruff check, mypy strict, and the resolve_plan doctest.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| ), | ||
| "force": ( | ||
| click.option( | ||
| "--force", |
There was a problem hiding this comment.
provide a shorthand -f as well.
if there's a shared agents file, let's document that when creating an option, both short and full option flags should be provided.
pearsonca
left a comment
pearsonca
left a comment
There was a problem hiding this comment.
Minor notes; feel free to merge once addressed.
| force: bool = False, | ||
| ) -> ExitCode: | ||
| """ | ||
| Execute the processing step. | ||
| Execute the processing step and anything it depends on. | ||
|
|
||
| The requested step's `depends` are resolved first, so asking for one | ||
| step also runs whatever it needs, in dependency order. A step that | ||
| reports its target already satisfied is skipped, which is what makes | ||
| re-running a pipeline cheap. | ||
|
|
||
| Args: | ||
| config: Path to the configuration file. | ||
| dry_run: Whether dry run mode is enabled. | ||
| target: Optional target process config to use. | ||
| force: Run the requested step even when its target is already | ||
| present. Dependencies keep their normal skip behaviour, so | ||
| forcing a transform does not re-fetch its inputs. |
There was a problem hiding this comment.
Does it make sense to let the force option here be a counter option? i.e. i could provide -f -f or similar to also force dependencies?
| section = {"transform": _step(["fetch"]), "fetch": _step()} | ||
| assert resolve_plan(section) == ["fetch", "transform"] | ||
|
|
||
|
|
There was a problem hiding this comment.
Need a test for a step that depends on multiple other steps. should the dependencies first, otherwise in independent order.
Three notes from @pearsonca: `--force` becomes a counter with a `-f` shorthand. One -f forces the step you asked for, `-ff` forces its dependencies too, so "re-run this" and "re-run this and everything it needs" stop being the same request. The decision moves into _should_force() so it is testable rather than inline in the loop. The shorthand is load-bearing, not just convenient: commands are re-dispatched to job backends by rendering bound options back into argv, and _render_param() renders a counter by repeating its *short* flag, so without -f the option would have rendered as "-forceforce". CONTRIBUTING now states the both-flags convention and why counted options depend on it; there is no tracked agents file in this repo to put it in. Tests: a step with several dependencies, a diamond, and narrowing past multiple dependencies, all asserting dependencies precede their dependent while independent steps keep configuration order; plus the force counter and the argv round-trip.
|
Thanks, all three addressed in 2ca9015.
Convention documented in CONTRIBUTING.md, since there is no tracked agents file Tests added for multiple dependencies, a diamond, and narrowing past several |
2 participants
Draft, because #323 left three design questions open and this answers them. The code is complete and green; the point of the draft is to review the contract before flepimop2-extras #16 and #22 build on it.
The three questions, answered
Each answer is the least-committal option I could find that still supports the motivating
fetch -> transformpipeline.1. Is
dependsdeclared on the process configuration, the process module, or a separate run plan?On the process configuration entry, naming sibling keys of the
process:section, typed as a field onProcessABCso it validates and documents itself. No new config section, no separate run-plan concept.ModuleBaseisextra="allow", so adependskey already flowed through untyped; this just makes it explicit.2. Are targets opaque identifiers or filesystem paths?
Opaque.
dependsnames steps, never files. Core orders steps and never learns what any of them produces, which keeps filesystem semantics in the providers, where the issue wanted them.3. Does core own topological execution only, or also cache freshness and provenance?
Execution only. A step answers
is_satisfied()for itself and core skips it; core never inspects a target, because only the module knows what it produces and what counts as fresh. Thedependsmetadata is preserved for the status and provenance reporting the issue anticipates, but nothing here interprets it.What this gives you
resolve_plan()validates every reference, rejects self-dependencies and cycles, and returns steps in dependency order. Problems are collected and reported together rather than one per run.dependsname is caught without importing every module in the section first.flepimop2 process --target transformfetches first.execute(force=)gives a target the three states the issue names: absent, present, present-with-force. Only the step actually requested is forced; dependencies keep their own skip behaviour, so forcing a transform does not re-download its inputs. That felt like the least surprising choice, but it is a judgment call worth confirming.Verification
ruff format --check,ruff check --no-fix: cleanmypy(strict): cleanresolve_plandoctest passesNot in scope
Provider-specific fetching and transforms stay in flepimop2-extras, per the issue. Nothing here reads or writes a file target.
Closes #323