fix(workflows): skip steps disabled in the builder at compile time - #2826
fix(workflows): skip steps disabled in the builder at compile time#2826nkuneman wants to merge 1 commit into
Conversation
|
👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely. 🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits. Warning 💸 The Claude reviewer bills in credits, not vibesAutomated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared. Draft when unsure, Ready when you mean it:
However you get there, arrive prepared:
Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.
|
The Workflow builder stores 'disable block' as a UI-only flag at metadata.ui.nodes["$steps.<name>"].disabled and strips such steps client-side before preview runs. The persisted spec still contains them, so any runtime fetching the workflow by id (inference server, edge, DD, serverless) compiled and executed every step, loading model weights for disabled model blocks. Mirror the builder's stripDisabledForExecution in the execution engine: drop manually disabled steps, cascade to steps whose required inputs or conditional gates are all disabled, remove dangling references and outputs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1201e9c to
166b4ca
Compare
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
| if manifest_class is None: | ||
| return False | ||
| for field_name, field_info in manifest_class.model_fields.items(): | ||
| if field_name in RESERVED_STEP_KEYS or field_name == NEXT_STEPS_FIELD: |
There was a problem hiding this comment.
High — disabling the only downstream target of a flow-control block makes the whole workflow fail to compile.
next_steps is excluded from the cascade-(a) check here, but it is not excluded from _strip_references (L185-197). _clean_value on a list that ends up empty returns _DELETE (L223), and _strip_references then drops the key entirely.
next_steps is a required field (no default) on every flow-control block that has it:
core_steps/flow_control/continue_if/v1.py:115core_steps/flow_control/rate_limiter/v1.py:104core_steps/flow_control/delta_filter/v1.py:93
Concrete failure — this is the mirror image of the scenario your own test covers:
steps: [
{"type": "roboflow_core/continue_if@v1", "name": "gate", ..., "next_steps": ["$steps.second_model"]},
{"type": "roboflow_core/roboflow_object_detection_model@v2", "name": "second_model", ...}
]
metadata: {"ui": {"nodes": {"$steps.second_model": {"disabled": true}}}} // user disables the TARGET, keeps the gate- seed =
{second_model} - cascade (a) for
gate:condition_statementhas no step refs,next_stepsis skipped by this line →gatesurvives - cascade (b):
gatehas no control predecessors → survives _strip_references(gate):["$steps.second_model"]→ empty list →_DELETE→next_stepskey removedparse_workflow_definition→ pydanticfield required→WorkflowSyntaxError
So the user disables one block in the builder and the entire workflow stops running with a syntax error on every runtime that fetches it by id — strictly worse than the bug being fixed.
Same crash from a second trigger: _clean_value returns _DELETE for a collection that was already empty before stripping (L222-223 doesn't distinguish "emptied by us" from "empty to begin with"). A continue_if persisted with "next_steps": [] — explicitly documented as valid ("If empty, the branch terminates even when the condition is true", continue_if/v1.py:115) — will lose the key and fail to parse as soon as any step anywhere in the workflow is disabled. More generally, an explicitly-empty list/dict on a surviving step is silently replaced by the manifest default rather than preserved.
Suggested direction: when a cleaned collection becomes empty, keep []/{} instead of deleting when the field is required (or at minimum special-case next_steps → [], which the blocks already handle as "terminate branch"), and only fall back to _DELETE for optional fields that were non-empty to begin with.
| result: Dict[str, Set[str]] = {} | ||
| for step in steps: | ||
| source = _step_name(step) | ||
| if not source or NEXT_STEPS_FIELD not in step: |
There was a problem hiding this comment.
High — cascade (b) misses switch_case@v1, so disabling a Switch Case makes its gated branches run unconditionally.
The control-predecessor map only looks at a field literally named next_steps. roboflow_core/switch_case@v1 routes control through cases: Dict[str, StepSelector] and default_next_steps (core_steps/flow_control/switch_case/v1.py:99 and :115) — neither is named next_steps.
Failure path with $steps.router (a Switch Case) disabled and on_red / on_blue reachable only through it:
- seed =
{router}, cascade (b) finds no control predecessors foron_red/on_blue(map is empty) - cascade (a) doesn't fire either — their required fields reference
$inputs.image, not the router routeris stripped;on_red/on_bluesurvive with their data dependencies intact
Control flow only suppresses steps in the executor, so once the gate is gone both branches execute on every run. Before this PR they ran only on a matching case. That inverts the PR's stated goal: disabling a Switch Case now causes more model weights to be loaded and more branches to execute than before, on every runtime that fetches the workflow by id.
rate_limiter and delta_filter happen to be covered because they use the literal next_steps name — the map should be driven by the manifest's StepSelector-typed fields (you already have manifests_by_type available) rather than by a hardcoded field name, so any current or future flow-control block is picked up.
| available_blocks=available_blocks, | ||
| profiler=profiler, | ||
| ) | ||
| inlined_raw_workflow_definition = strip_disabled_steps( |
There was a problem hiding this comment.
Medium — running after inline_inner_workflow_steps makes the fix a silent no-op for disabled Inner Workflow blocks.
inline_inner_workflow_steps replaces a roboflow_core/inner_workflow@v1 step named my_sub with its children, renamed to {inner}__{child} (inner_workflow/inline.py:75-92, _unique_prefixed_step_name). The my_sub step no longer exists in steps by the time strip_disabled_steps runs.
So for metadata.ui.nodes["$steps.my_sub"].disabled = true:
disabled = {"my_sub"}matches no surviving step name (my_sub__child, …) → nothing droppeddisabled_node_ids = {"$steps.my_sub"}matches no surviving reference either, because inlining already rewrote$steps.my_sub.<output>to the child selectors
Net effect: a user who disables an Inner Workflow block in the builder still gets every child step compiled and executed, weights included — exactly the reported symptom the PR is fixing. (The related gap: disabled flags stored in a child workflow's own metadata are never read, since only root-level metadata is consulted.)
Running the strip on raw_workflow_definition before inlining would handle the outer case naturally. If ordering has to stay as-is, the inner-workflow case needs explicit handling — and either way it needs a test, since nothing here fails loudly.
Review summarySkills: The intent is sound and the module is readable, but the stripping rules diverge from the block manifests in ways that turn a "disable one block" action into either a hard compile failure or more execution than before. Three findings posted inline:
A separate release-coordination comment covers the missing EE changelog entry. Tests to addThe existing 5 unit cases all exercise
Minor doubts (non-blocking)
Commands that informed this review: Reviewed at HEAD: 166b4ca |
📦 Release coordination — Execution EngineMedium finding (contributor-actionable): this PR changes Execution Engine compile behavior — The entry should be behavior-level, and should call out the compatibility edge explicitly, because it is a silent behavior change on upgrade for every already-saved workflow: any workflow that carries Please do not pick or bump a version — just add the @PawelPeczek-Roboflow @grzegorz-roboflow @dkosowski87 — maintainer notice (non-blocking, not a gate on the contributor): Execution Engine requires a version bump for release. Compile behavior changed, so at release time the
Reviewed at HEAD: 166b4ca |
|
⏳ This PR is on hold pending your answers — the review will not advance to sign-off until the IMPORTANT questions below are answered. Unanswered questions may keep this change out of a release. 1. Is
2. Does the builder's Specifically: when a 3. Is the silent, ungated behavior change on upgrade the intended rollout? Every already-saved workflow with a disabled flag changes results the moment a runtime picks up this build: steps stop running and their output keys disappear from the response. There is no EE-version gate, no env flag, and no way for an operator to fall back. New commits are not auto-reviewed — add the Reviewed at HEAD: 166b4ca |
PawelPeczek-Roboflow
left a comment
There was a problem hiding this comment.
This PR gets blocked as it looks it changes blocks disabled into compilation error in some cases.
Plus I insist on making disable flag core part of the syntax, not UI metadata treated so far as opaque.
Problem
Reported by client: a model block toggled to Disabled in the Workflow builder still loads its weights onto the GPU at runtime. They have to manually evict it from the device.
Root cause: "Disable" is a UI-only flag stored at
metadata.ui.nodes["$steps.<name>"].disabled. The builder strips disabled steps client-side (stripDisabledForExecutioninWorkflowBuilderV2/services/disabledBlocks.ts) but only for preview/test-block runs. The persisted spec still contains the steps, and the execution engine has no concept of the flag, so every run byworkflow_id(inference server, edge, Dedicated Deployments, serverless) compiles and executes disabled steps.Fix
New
compiler/disabled_steps.py, called incompile_workflow_graphright beforeparse_workflow_definition. It mirrors the builder's logic:disabled: trueundermetadata.ui.nodesnext_steps) that are all disabled is itself disabledNo-op (returns the same object) when nothing is disabled.
Tests
tests/workflows/unit_tests/execution_engine/compiler/test_disabled_steps.py(5 cases). Fullcompiler/unit dir passes (86) in a python:3.12 container.Open question
Alternative would be stripping server-side in the
GET /:workspace/workflows/:workflowUrlAPI response. Went with the engine so it also covers inline specs posted to/workflows/runand doesn't change the public API's representation of a saved workflow. Happy to move it if the Workflows team prefers otherwise.🤖 Generated with Claude Code