-
Notifications
You must be signed in to change notification settings - Fork 312
fix(workflows): skip steps disabled in the builder at compile time #2826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| """ | ||
| Honour steps disabled in the Workflow builder at runtime. | ||
|
|
||
| The 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 specification still contains them, so any | ||
| runtime that fetches the workflow by id (inference server, edge devices, Dedicated | ||
| Deployments, serverless) would otherwise compile and execute every step - including | ||
| loading model weights for disabled model blocks. | ||
|
|
||
| This module mirrors the builder's ``stripDisabledForExecution`` logic: | ||
| * seed: every step manually flagged ``disabled: true`` | ||
| * cascade (a): a step whose required (no-default) field would be emptied entirely | ||
| by removing references to disabled steps is itself disabled | ||
| * cascade (b): a step gated only by conditional-flow blocks (``next_steps``) | ||
| that are all disabled is itself disabled | ||
| * strip: drop disabled steps, remove references to them from surviving steps, | ||
| drop outputs whose selector points at a disabled step | ||
| """ | ||
|
|
||
| import re | ||
| from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Type | ||
|
|
||
| from pydantic.fields import FieldInfo | ||
| from typing_extensions import get_args | ||
|
|
||
| from inference.core.workflows.execution_engine.v1.compiler.entities import ( | ||
| BlockSpecification, | ||
| ) | ||
| from inference.core.workflows.prototypes.block import WorkflowBlockManifest | ||
|
|
||
| STEP_REF_PATTERN = re.compile(r"\$steps\.([^.\s\]\[\"']+)") | ||
| NEXT_STEPS_FIELD = "next_steps" | ||
| RESERVED_STEP_KEYS = {"type", "name", "id"} | ||
|
|
||
|
|
||
| def strip_disabled_steps( | ||
| workflow_definition: Dict[str, Any], | ||
| available_blocks: Iterable[BlockSpecification], | ||
| ) -> Dict[str, Any]: | ||
| """Return a copy of ``workflow_definition`` with disabled steps removed. | ||
|
|
||
| Returns the input object untouched when nothing is disabled, so the common | ||
| path is free of copies. | ||
| """ | ||
| manually_disabled = _collect_manually_disabled_step_names(workflow_definition) | ||
| if not manually_disabled: | ||
| return workflow_definition | ||
| steps = workflow_definition.get("steps") or [] | ||
| manifests_by_type = _index_manifests_by_type(available_blocks) | ||
| disabled = _compute_disabled_step_names( | ||
| steps=steps, | ||
| seed=manually_disabled, | ||
| manifests_by_type=manifests_by_type, | ||
| ) | ||
| disabled_node_ids = {f"$steps.{name}" for name in disabled} | ||
| surviving_steps = [] | ||
| for step in steps: | ||
| if _step_name(step) in disabled: | ||
| continue | ||
| surviving_steps.append(_strip_references(step, disabled_node_ids)) | ||
| surviving_outputs = [ | ||
| output | ||
| for output in workflow_definition.get("outputs") or [] | ||
| if not ( | ||
| isinstance(output, dict) | ||
| and isinstance(output.get("selector"), str) | ||
| and _selector_points_at_any(output["selector"], disabled_node_ids) | ||
| ) | ||
| ] | ||
| result = dict(workflow_definition) | ||
| result["steps"] = surviving_steps | ||
| result["outputs"] = surviving_outputs | ||
| return result | ||
|
|
||
|
|
||
| def _collect_manually_disabled_step_names( | ||
| workflow_definition: Dict[str, Any], | ||
| ) -> Set[str]: | ||
| metadata = workflow_definition.get("metadata") | ||
| if not isinstance(metadata, dict): | ||
| return set() | ||
| ui = metadata.get("ui") | ||
| if not isinstance(ui, dict): | ||
| return set() | ||
| nodes = ui.get("nodes") | ||
| if not isinstance(nodes, dict): | ||
| return set() | ||
| result = set() | ||
| for node_id, node_meta in nodes.items(): | ||
| if not isinstance(node_meta, dict) or node_meta.get("disabled") is not True: | ||
| continue | ||
| if not isinstance(node_id, str) or not node_id.startswith("$steps."): | ||
| continue | ||
| result.add(node_id[len("$steps.") :]) | ||
| return result | ||
|
|
||
|
|
||
| def _compute_disabled_step_names( | ||
| steps: List[Dict[str, Any]], | ||
| seed: Set[str], | ||
| manifests_by_type: Dict[str, Type[WorkflowBlockManifest]], | ||
| ) -> Set[str]: | ||
| disabled = set(seed) | ||
| control_predecessors = _build_control_predecessor_map(steps) | ||
| changed = True | ||
| while changed: | ||
| changed = False | ||
| disabled_node_ids = {f"$steps.{name}" for name in disabled} | ||
| for step in steps: | ||
| name = _step_name(step) | ||
| if not name or name in disabled: | ||
| continue | ||
| if _has_fully_disabled_required_field( | ||
| step=step, | ||
| disabled_node_ids=disabled_node_ids, | ||
| manifests_by_type=manifests_by_type, | ||
| ) or _has_only_disabled_control_predecessors( | ||
| step_name=name, | ||
| control_predecessors=control_predecessors, | ||
| disabled=disabled, | ||
| ): | ||
| disabled.add(name) | ||
| changed = True | ||
| return disabled | ||
|
|
||
|
|
||
| def _has_fully_disabled_required_field( | ||
| step: Dict[str, Any], | ||
| disabled_node_ids: Set[str], | ||
| manifests_by_type: Dict[str, Type[WorkflowBlockManifest]], | ||
| ) -> bool: | ||
| manifest_class = manifests_by_type.get(step.get("type")) | ||
| 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High — disabling the only downstream target of a flow-control block makes the whole workflow fail to compile.
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
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: Suggested direction: when a cleaned collection becomes empty, keep |
||
| continue | ||
| if not _is_required(field_info): | ||
| continue | ||
| value = step.get(field_name) | ||
| if value is None and field_info.alias: | ||
| value = step.get(field_info.alias) | ||
| if value is None: | ||
| continue | ||
| if not _find_step_refs(value): | ||
| continue | ||
| cleaned, _ = _clean_value(value, disabled_node_ids) | ||
| if cleaned is _DELETE: | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def _is_required(field_info: FieldInfo) -> bool: | ||
| try: | ||
| return field_info.is_required() | ||
| except AttributeError: # pragma: no cover - pydantic v1 fallback | ||
| return getattr(field_info, "required", False) is True | ||
|
|
||
|
|
||
| def _build_control_predecessor_map( | ||
| steps: List[Dict[str, Any]], | ||
| ) -> Dict[str, Set[str]]: | ||
| result: Dict[str, Set[str]] = {} | ||
| for step in steps: | ||
| source = _step_name(step) | ||
| if not source or NEXT_STEPS_FIELD not in step: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High — cascade (b) misses The control-predecessor map only looks at a field literally named Failure path with
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.
|
||
| continue | ||
| for target in _find_step_refs(step.get(NEXT_STEPS_FIELD)): | ||
| result.setdefault(target, set()).add(source) | ||
| return result | ||
|
|
||
|
|
||
| def _has_only_disabled_control_predecessors( | ||
| step_name: str, | ||
| control_predecessors: Dict[str, Set[str]], | ||
| disabled: Set[str], | ||
| ) -> bool: | ||
| predecessors = control_predecessors.get(step_name) | ||
| if not predecessors: | ||
| return False | ||
| return all(predecessor in disabled for predecessor in predecessors) | ||
|
|
||
|
|
||
| def _strip_references( | ||
| step: Dict[str, Any], disabled_node_ids: Set[str] | ||
| ) -> Dict[str, Any]: | ||
| result = {} | ||
| for key, value in step.items(): | ||
| if key in RESERVED_STEP_KEYS: | ||
| result[key] = value | ||
| continue | ||
| cleaned, _ = _clean_value(value, disabled_node_ids) | ||
| if cleaned is _DELETE: | ||
| continue | ||
| result[key] = cleaned | ||
| return result | ||
|
|
||
|
|
||
| class _Delete: | ||
| pass | ||
|
|
||
|
|
||
| _DELETE = _Delete() | ||
|
|
||
|
|
||
| def _clean_value(value: Any, disabled_node_ids: Set[str]) -> Tuple[Any, bool]: | ||
| """Return (cleaned_value, changed). ``_DELETE`` means drop the value.""" | ||
| if isinstance(value, str): | ||
| if _string_references_disabled(value, disabled_node_ids): | ||
| return _DELETE, True | ||
| return value, False | ||
| if isinstance(value, list): | ||
| changed = False | ||
| filtered = [] | ||
| for item in value: | ||
| cleaned, item_changed = _clean_value(item, disabled_node_ids) | ||
| if cleaned is _DELETE: | ||
| changed = True | ||
| continue | ||
| changed = changed or item_changed | ||
| filtered.append(cleaned) | ||
| if not filtered: | ||
| return _DELETE, True | ||
| return (filtered if changed else value), changed | ||
| if isinstance(value, dict): | ||
| changed = False | ||
| out = {} | ||
| for key, item in value.items(): | ||
| cleaned, item_changed = _clean_value(item, disabled_node_ids) | ||
| if cleaned is _DELETE: | ||
| changed = True | ||
| continue | ||
| changed = changed or item_changed | ||
| out[key] = cleaned | ||
| if not out: | ||
| return _DELETE, True | ||
| return (out if changed else value), changed | ||
| return value, False | ||
|
|
||
|
|
||
| def _string_references_disabled(value: str, disabled_node_ids: Set[str]) -> bool: | ||
| return any( | ||
| value == node_id or value.startswith(f"{node_id}.") | ||
| for node_id in disabled_node_ids | ||
| ) | ||
|
|
||
|
|
||
| def _selector_points_at_any(selector: str, disabled_node_ids: Set[str]) -> bool: | ||
| return _string_references_disabled(selector, disabled_node_ids) | ||
|
|
||
|
|
||
| def _find_step_refs(value: Any) -> List[str]: | ||
| if isinstance(value, str): | ||
| return STEP_REF_PATTERN.findall(value) | ||
| if isinstance(value, list): | ||
| return [ref for item in value for ref in _find_step_refs(item)] | ||
| if isinstance(value, dict): | ||
| return [ref for item in value.values() for ref in _find_step_refs(item)] | ||
| return [] | ||
|
|
||
|
|
||
| def _step_name(step: Any) -> Optional[str]: | ||
| if not isinstance(step, dict): | ||
| return None | ||
| name = step.get("name") or step.get("id") | ||
| return name if isinstance(name, str) else None | ||
|
|
||
|
|
||
| def _index_manifests_by_type( | ||
| available_blocks: Iterable[BlockSpecification], | ||
| ) -> Dict[str, Type[WorkflowBlockManifest]]: | ||
| result: Dict[str, Type[WorkflowBlockManifest]] = {} | ||
| for block in available_blocks: | ||
| manifest_class = block.manifest_class | ||
| type_field = manifest_class.model_fields.get("type") | ||
| if type_field is None: | ||
| continue | ||
| for type_identifier in get_args(type_field.annotation): | ||
| if isinstance(type_identifier, str): | ||
| result[type_identifier] = manifest_class | ||
| return result | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium — running after
inline_inner_workflow_stepsmakes the fix a silent no-op for disabled Inner Workflow blocks.inline_inner_workflow_stepsreplaces aroboflow_core/inner_workflow@v1step namedmy_subwith its children, renamed to{inner}__{child}(inner_workflow/inline.py:75-92,_unique_prefixed_step_name). Themy_substep no longer exists instepsby the timestrip_disabled_stepsruns.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 selectorsNet 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:
disabledflags stored in a child workflow's ownmetadataare never read, since only root-levelmetadatais consulted.)Running the strip on
raw_workflow_definitionbefore 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.