feat: Finalize Conditional Execution - #2657
Conversation
🎩 PreviewA preview build has been created at: |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Carry-over audit: does this stack close out #2574's review comments?#2574's comments aren't meant to be actioned there — the expectation is that #2651 and #2657 deal with them. This is the audit of that, posted at the top of the stack so it's in one place. 23 comments on the root PR: 10 resolved, 8 still open, 1 changed shape, 1 partial. Resolved by #2651 / #2657
Still open
Changed shapeBoolean port type / quick-connect compatibility. The original comment was about the Partially addressedAccessible labelling. The Config panel switch now has a real Cross-stack note
|
325c67e to
2d0ce95
Compare
2d0ce95 to
20b5ec3
Compare
## Description Implements **Option 1 — Derived**. Two alternatives were prototyped alongside it (#2652, #2654); this is the one we settled on. Also a general pass over the UI, UX and frontend architecture of the feature. ### The question these options were answering `TaskSpec.isEnabled` decides whether a task runs. It either holds a literal (`"false"`) or points at an upstream value — a graph input, or another task's output. The editor needs to know one thing the spec doesn't record: whether the user wants the conditional UI on this task at all — the extra handle on the node, the condition control in the Config panel. A task with no `isEnabled` looks exactly like a task that was never meant to be conditional, so where does that piece of state live? ### The three answers - **Option 2 — Annotation (#2652):** keep an `isConditional` annotation on the task, fully decoupled from `isEnabled`. The annotation controls the UI; `isEnabled` controls the backend. - **Option 4 — Always-on (#2654):** don't track it. Show the conditional UI on every task, always. - **Option 1 — Derived (this PR):** don't track it either, but infer it. A task is conditional when it *has* an `isEnabled` value — a literal on the task, or a connection to the reserved port. ### Why derived **Two sources of truth drift, and this one drifts immediately.** Pipelines don't only come from our editor — they come from the SDK, from other tools, from hand-written YAML. Those have `isEnabled` set and no annotation, so the annotation version opens them showing no condition and no edge: the connection is right there in the file and invisible on the canvas. The fix would be to backfill the annotation on import, which is deriving it anyway, one round trip late. **It keeps editor state out of the user's pipeline.** Conditional-ness is already expressed by `isEnabled`. An annotation would write a second copy of it into the file for something we can compute. **Most tasks aren't conditional.** Always-on avoids the state problem, but it spends vertical space on every node and adds a concept to every task for the benefit of the few that use it. A switch keeps it opt-in and keeps ordinary nodes looking ordinary. **One question, one answer.** `isTaskConditional(task, spec)` is what the node, the panel and serialization all ask, so they can't disagree. The honest cost: deriving couples the switch to the value. Turning it on has to write something (`"true"` — Always), and turning it off has to clear it, so "conditional but not configured yet" isn't a state we can represent. We think that's fine — Always is a sensible starting point, and the alternative was the drift above. ### Also in here - **The condition control was a three-way dropdown** (*Enable task*: True / False / Conditional), which folded two different questions into one control — "should this be conditional" and "what's the condition". It's now a switch for the first and a toggle for the second. - **Deleting the condition edge resets the task to enabled** instead of resurfacing the previous literal. For an ordinary input, bringing back the old literal when you disconnect it is helpful. For a condition it isn't: resurfacing `"false"` would leave the task silently disabled after the user deliberately removed the condition. ## Related Issue and Pull requests Stacked on #2574. Alternatives: #2652 (annotation), #2654 (always-on). Followed by #2657, which finishes the UI. ## Type of Change - [x] New feature ## Checklist - [ ] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Screenshots (if applicable) <!-- Include any screenshots that might help explain the changes or provide visual context --> ## Test Instructions The `conditional-execution` flag gates all of this. 1. Select a task → **Config** tab → toggle **Conditional task** on, then set the condition to False and confirm the task's `isEnabled` follows in the YAML. 2. Toggle it back off and confirm `isEnabled` is cleared and no stray annotation is written. 3. Connect a graph input or an upstream task output to the condition handle, then delete that edge — the task should stay conditional and reset to enabled. 4. Import a pipeline whose YAML sets `isEnabled` to a reference (e.g. one produced by the SDK) and confirm the editor shows both the conditional UI and the edge without us having written anything into the file first. ## Additional Comments <!-- Add any additional context or information that reviewers might need to know regarding this PR -->
20b5ec3 to
5ff8846
Compare
…ns (#2658) ## Description Conditional execution only works on tasks that run a container. The backend rejects it on subgraphs, so a pipeline that gates one fails at submission with a raw error the user can't act on. **The control is hidden for subgraphs.** The Conditional execution box no longer appears when the selected node is a subgraph, so it can't be switched on by accident. **Validation covers what hiding the control can't.** A pipeline can arrive with the condition already set — from the SDK, from hand-written YAML, or from an earlier version of the editor. A subgraph with a run condition is now flagged as an error that blocks submission, with a **Remove Condition** fix offered in the issue panel. Three related checks, since the same panel was already open: - **A component that declares an input named `__is_enabled__`** collides with the name the editor reserves for run conditions. Connecting such an input would silently save it as the task's run condition and drop the argument. It's a warning as soon as the component is used, and an error once the input is actually connected. - **A condition wired to a value that can't be read as true or false** — a number, say — is an error. Anything else blocks submission too, and for the same reason: the run doesn't fail up front, it fails at the moment the task is reached, after everything upstream has already spent its compute. Catching it before submission is worth doing. - **A fixed condition that isn't true or false** — someone's hand-written `yes`, or a value the editor can't make sense of — is an error, and the message quotes the offending value. Casing and stray whitespace are fine. A condition that's wired up rather than fixed is left alone, since its value isn't known until the run. ## Related Issue and Pull requests Stacked on #2657. ## Type of Change - [x] Bug fix ## Checklist - [ ] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Screenshots (if applicable) <!-- Include any screenshots that might help explain the changes or provide visual context --> ## Test Instructions The `conditional-execution` flag is off by default — turn it on in Settings first. 1. Select a subgraph node → **Config** tab. There should be no Conditional execution box. Select an ordinary task and confirm it's still there. 2. Hand-edit a pipeline's YAML so a subgraph task is gated on a condition, then open it. The issue panel should show an error, submission should be blocked, and **Remove Condition** should clear it. 3. Gate an ordinary task on a number-typed output. You should get an error explaining the value can't be read as a condition, and submission should be blocked. Re-wire it to a String or Boolean output and the error should clear. 4. Hand-edit a task's condition to something like `yes`. You should get an error quoting that value. Change it to `TRUE` or ` false ` and it should be accepted. ## Additional Comments The reserved-name check is a guard against a name collision rather than something a user is likely to hit — a component would have to declare an input called `__is_enabled__`. It's here because the failure is silent: the argument disappears on save with nothing to indicate why. The type check accepts `str`, `bool` and `text` alongside the canonical type names. Since it blocks submission, a hand-written component using a lowercase alias shouldn't be caught by it. One thing deliberately not done: the accepted conditions aren't normalized on load. The checks read leniently but write nothing — quietly rewriting a value in someone's pipeline is worse than reporting it.

Description
Final pass over conditional execution. Nothing changes about what gets sent to the backend — this is UI, vocabulary and test coverage. The visual direction is borrowed from the exploration in #2649.
"Run when", not "isEnabled".
isEnabledis spec jargon. The UI now says Run when, and the two literal choices read Always / Never rather than true/false.Conditional execution gets its own purple box in the Config tab. A single switch turns it on for a task. When it's on, the Run when control appears inside the box — either the Always/Never toggle, or, if something is wired into it, the upstream source it follows (
→ Flag.flag), formatted the same way bound inputs are formatted everywhere else in the editor.On the node, the condition is separate from the inputs. It used to be injected into the task's input list as a fake input, which meant the list had to know about it in every place it did anything (splitting, condensing, counting). It's now its own purple row above the inputs with its own handle, which means:
Dragging off the "Run when" handle no longer creates a graph input called
__is_enabled__. The editor names an auto-created input after the port you dragged from, and the reserved port name was leaking into the user's pipeline. It now creates an input calledrun_condition, typedString— that's the form the condition is actually read in, and it's what lets the input connect to the ports components declare.Naming and comments. Internal names now say what they are (
resetRunCondition,runConditionBinding,setRunCondition, …). Comments that narrated the code were deleted; the ones left explain a decision the code can't.Tests. New coverage for the shared helpers, the enable/disable actions (including that switching conditional execution off clears both the literal and the connection while leaving other connections alone), the node rendering, the auto-created input, and each of the fixes below.
Fixes from review
false. That now reads as Never; before, it showed as Always while the backend skipped the task — the display and the behaviour disagreed.run_conditioninput, and a fixed condition survives the round trip.Related Issue and Pull requests
Stacked on #2651. UI direction borrowed from #2649. Followed by #2658, which stops conditional execution being set where the backend won't honour it.
Type of Change
Checklist
Screenshots (if applicable)
Test Instructions
The
conditional-executionflag is off by default — turn it on in Settings first.run_conditionString graph input, not one named__is_enabled__.false, and the task should still read as gated off — not silently switch to running.→ Task.output.false(no quotes), then open it. The task should read Never.__is_enabled__should appear anywhere.Additional Comments
Turning conditional execution off deliberately clears the condition (both the literal and any connection) rather than remembering it, so re-enabling starts from Always. That's the trade discussed in #2651.