review-workflows-blocks

# Reviewing workflows-blocks changes ## Scope Triggers when a PR touches: - `inference/core/workflows/core_steps/**` — Roboflow Core plugin blocks (`{category}/{block}/v{N}.py`, `common/`, `common/query_language/**`, `loader.py`). - `inference/core/workflows/execution_engine/entities/types.py` — only when a NEW **kind** is added for a block (review it alongside the block). OUT of scope (other skills own these): Execution Engine internals under `inference/core/workflows/execution_engine/v1/**` (compiler, executor, output_constructor — `review-workflows-execution-engine`); model backends under `inference_models/`. Since the tensor-data-representation merge (#2357), the `vN_tensor.py` siblings ARE in scope here — review them with the *Tensor-native parity* items below; authoring-side counterpart: `create-workflow-block`. UQL / `common/query_language/**` has no dedicated skill, so it IS reviewed here (see the query-language standard below). ## Review checklist Severity: **BLOCK** = must fix before merge; **FLAG** = raise it; **NIT** = optional. Each item maps to one canonical rule in `## Standards` below. 1. **BLOCK — Output contract**: enumerate `describe_outputs()` keys; confirm EVERY `run()`/helper return dict (happy, empty, error, zero-area) carries exactly those keys (#2346). 2. **BLOCK — Versioning discipline**: bug-fix patches the existing `vN.py`; behavior change / new inputs-outputs goes in a NEW `vN.py`. Confirm no shipped `type` identifier or legacy alias was removed or repurposed. 3. **BLOCK — Loader wiring**: new block imported into `load_blocks()`; new kind into `load_kinds()` + defined in `types.py` with `*_KIND_DOCS`; serializable data-kind into `KINDS_SERIALIZERS`/`KINDS_DESERIALIZERS`. 4. **BLOCK — `__init__.py`** exists in every new/nested block package (guarded by `test_init_files.py`). 5. **BLOCK — `sv.Detections.data` density**: any new data key holds dense N-d numpy arrays (no `dtype="object"` ragged) and has serializer/deserializer parity if it must round-trip. 6. **FLAG — Tests**: unit test under `tests/workflows/unit_tests/core_steps/{category}/`; integration test under `tests/workflows/integration_tests/execution/test_workflow_with_*_block.py` for new or output-changing blocks. 7. **FLAG — EE compatibility floor**: actively verify `get_execution_engine_compatibility()` against the EE capabilities the block actually uses — contributors habitually leave the copy-pasted default range unexamined. Derive the true minimum from the EE changelog in the roboflow/docs repo (`workflows/developer-guide/execution-engine-changelog.md`) and require the floor raised when the block relies on a capability introduced after the declared minimum. If a capability the block relies on exists only under `## Unreleased` (including one added in the same PR), the EE version must be placed and bumped so the block can gate on it — see the EE-compatibility standard. 8. **FLAG — EE behavior change**: if the block forces an EE behavior change, require a user-facing entry under `## Unreleased` in the EE changelog in the roboflow/docs repo (`workflows/developer-guide/execution-engine-changelog.md`) and surface the maintainer notice **Execution Engine requires a version bump for release** — never ask the contributor to bump `EXECUTION_ENGINE_V1_VERSION` or update version-assert tests (maintainers do both in the release PR), except in the unreleased-dependency case above. Outside that exception, a contributor PR touching that constant is a red flag. 9. **FLAG — Cross-block kinds**: output kinds upstream ⊆ input kinds accepted downstream (esp. detections/segmentation/keypoint interplay). 10. **FLAG — Model-version changes** update defaults/enums (not silent deletion) and keep old identifiers loadable. 11. **FLAG — Batch handling** for model/foundation blocks: `run` iterates `Batch[...]` correctly and declares `get_parameters_accepting_batches()`. 11a. **FLAG — Dependent-resource declaration**: any block that loads a model or external resource overrides `discover_dependent_resources()` (helpers `roboflow_platform_model()` / `third_party_model()`) so resource discovery and model pre-loading see it. A general block contract — not tensor-specific; a missing declaration means video pipelines pay the cold model load on the first frame. 12. **FLAG — UQL correctness over speed**: be skeptical of micro-optimizations to `common/query_language/**` ops that change behavior (#1112/#1161/#1162 were reverted). 13. **NIT — Field metadata**: new inputs have `title`/`description`/`examples`; `default=` correct (no stray `default=None` on required fields); `json_schema_extra` (`relevant_for`, `always_visible`) sensible. ### Tensor-native parity (blocks with `vN_tensor.py` siblings) 14. **BLOCK — Sibling drift**: a PR editing a numpy block's behavior without the mirrored edit in its `_tensor` sibling (or vice versa) is a finding — main-merge compensation sweeps found dozens of drifted siblings; drift ships divergent flag-on behavior silently. 15. **BLOCK — Manifest surface identical across flags**: field set, supported variants, batch-parameter declarations, AND output-kind list **order** must match between siblings — serializer selection is first-kind-that-serializes-wins, so kind-order divergence changes the wire payload per deployment flag (instance-seg v1–v3 RLE-vs-polygon lesson). 16. **BLOCK — Both-sibling companions**: general contracts hold on BOTH manifests — `discover_dependent_resources()` (the general rule is 11a; the tensor trap is that a numpy-side-only declaration silently disappears flag-on and pre-loading skips the block); sink siblings both honor `disable_sinks`. 17. **BLOCK — Host-mirror hygiene**: any tensor sibling rewriting `xyxy`/`class_id`/`confidence` strips the per-box host mirror (`strip_host_mirror_metadata`/`HOST_MIRROR_KEYS`) — a carried mirror makes downstream visualizations draw stale geometry with no error. 18. **FLAG — Mutation declarations**: every `copy_image=False` in-place draw calls `declare_numpy_image_mutated()`/`declare_tensor_image_mutated()` — including NEW mainline blocks arriving via merge without the retrofit (label v2 / rich_label class). 19. **FLAG — CPU-only import safety**: every module registered in the flag-on loader branch must import without CUDA (no module-level device initialization); the flag-on suite runs on CPU-only CI runners. 20. **FLAG — Flag-off registration is frozen**: the loader's flag-off block list and import bindings must stay byte-identical to pre-tensor `main` behavior — flag-off is the compatibility contract; any flag-off divergence riding a tensor PR is a finding. ### Not blocking - Do NOT demand an `inference/core/version.py` bump — inference releases are versioned separately from feature/bugfix PRs. - Do NOT demand a new `vN.py` for a pure in-place bug-fix; surgical patches to a shipped version are correct (#2346, #1339, #1368). - Never demand an `EXECUTION_ENGINE_V1_VERSION` bump from a contributor — maintainers bump it at release. Sole exception: a block gating on unreleased EE capabilities (see the EE-compatibility standard), where the bump must be maintainer-coordinated before the block ships. The `## Unreleased` EE changelog entry is required only when the block forces an EE behavior change; most block changes don't. - Do NOT demand serializer/deserializer entries for a data key that is purely intra-block and never crosses the wire. - Do NOT demand hand-written `docs/workflows/blocks/<block>.md` — those are generated from `Field(description=, examples=)` metadata. ## Standards The one canonical statement of each rule. Past regressions are cited inline; see `## Reference PRs`. - **Output contract**: every dict `run()` emits contains EXACTLY the keys declared by `describe_outputs()`, on every path including empty/zero-area/error branches (`return []`, `{"x": None}`) (#2346 — `dynamic_crop` zero-area branch dropped `predictions`; #1341 — empty-zone branch). - **Versioning**: bug-fixes patch the existing version in place; behavior changes / new inputs-outputs go into a NEW `vN.py` module — even at the cost of code duplication (`docs/workflows/versioning.md`, `blocks_bundling.md`). Never break `type` identifiers or legacy aliases; e.g. `dynamic_crop/v1.py` keeps `Literal["roboflow_core/dynamic_crop@v1", "DynamicCrop", "Crop"]`. Versioning foundation: #565. - **New block = full package**: new dir with `__init__.py` + `v1.py`, imported and added to `load_blocks()` in `loader.py`, plus a unit test AND an integration test (#2362 — overlap_analysis added all of these + a new kind). - **`__init__.py` in every package** (including nested): its absence breaks docs generation and block import; guarded by `test_init_files.py` (#2351, #830). - **New kind discipline**: define the kind in `types.py` with a `*_KIND_DOCS` string, return it from `load_kinds()`, and route serialization through `KINDS_SERIALIZERS`/`KINDS_DESERIALIZERS` (#2362 — `DETECTIONS_OVERLAPS_KIND`; #804 — `INFERENCE_ID_KIND` as a distinct semantic kind across blocks). - **`sv.Detections.data` must be dense numpy arrays**: object-dtype ragged arrays break supervision indexing / `is_data_equal`; pad to uniform shape (#2170 — keypoints; #1368 — velocity elements must be `np.array`). - **Serializer/deserializer parity for round-tripping data keys**: a data key that must survive serialization needs matching entries in `common/serializers.py` + `common/deserializers.py` (#1368 — velocity/speed; #1156 — timestamps). - **New input fields keep backward compat**: add with a `default=` and update `describe_outputs()`/`run()`; behavior-changing additions warrant a new version. Question `default=None` on non-optional inputs (#2384 — added in-place across v1/v2/v3 with defaults; #658 — removed wrong `default=None`). - **EE compatibility**: `get_execution_engine_compatibility()` declares the minimum EE version the block relies on (e.g. `">=1.3.0,<2.0.0"`); the EE version is a single source of truth in `EXECUTION_ENGINE_V1_VERSION`. The floor is a contract, not boilerplate: derive it from the EE changelog in the roboflow/docs repo (`workflows/developer-guide/execution-engine-changelog.md`) — a block needing a capability added in `1.3.7` declares `">=1.3.7,<2.0.0"`, not the copy-pasted default. A block that forces an EE behavior change makes an EE version bump mandatory at release: the contributor adds the `## Unreleased` changelog entry only; maintainers apply the bump + final changelog heading + version-assert test updates in the release PR (#2527 — revert rolled back both the block and the EE version and re-asserted it in tests). **Unreleased-dependency exception**: when the block relies on a capability still under `## Unreleased`, there is no released floor to declare — the EE version must be placed (the `## Unreleased` entries get their final `vX.Y.Z` heading) and `EXECUTION_ENGINE_V1_VERSION` bumped with mirrored version-assert tests, maintainer-coordinated, in or before this PR, so the block's range can reference it. This is the one case where the bump cannot be deferred to a release PR. - **Model-version deprecation**: when a hosted model version is deprecated, update the block default and enum; do not just delete, keep old identifiers loadable (#1668/#1669 — Claude 3.5→4.5; #500 — GPT-4→GPT-4o; #2395 — Gemini default). - **Batch handling for model/foundation blocks**: `run` iterates `Batch[...]` and declares `get_parameters_accepting_batches()` (#718 — VLM blocks failed on batch inference). - **Segmentation/mask edge cases**: mask→polygon must handle 0/1/degenerate contours (returns `None`, duplicates points); empty masks must serialize (#1339; #697 — SAM2 masks as points/lines). - **Visualization blocks are fragile across supervision versions**: verify against the pinned supervision public API, not internals (#814/#811/#1868 — label/line regressions; #1725/#1951 — sv 0.27.0 handling). - **Query language (UQL)**: `common/query_language/**` ops must preserve exact behavior; reject micro-optimizations that alter results (#1112/#1161/#1162 — automated speed-up PRs reverted). - **Rich `Field` metadata**: `title`, `description`, `examples`, and `json_schema_extra` (`relevant_for`, `always_visible`) drive the FE app; block docs are generated from these (#623/#704; #2058 — property options propagated to FE). - **Tensor-sibling contract** (#2357): siblings are standalone verbatim re-implementations against native objects — no delegation/method-binding to the numpy manifest (wholesale subclassing OK). Same-name registration in the loader's flag-on branch; the flag swaps implementations, never the registry (block-name lists identical both directions). Testing convention: per-file `_TENSOR_ONLY`/`_NUMPY_ONLY` skipif markers, mirrored assertions, and the shared `image_as_workflow_input` integration fixture parametrizing image inputs as numpy AND `torch.Tensor` — new tensor-native integration tests should take it. ## Required companions Block a functional change that lacks these (canonical rules above; conditions here): - **Loader registration** — new block in `load_blocks()`; new kind in `load_kinds()` + `types.py`; new serializable data-kind in `KINDS_SERIALIZERS`/`KINDS_DESERIALIZERS`, all in `loader.py` (#2362, #804). - **Tests** — unit test always; integration test for new or output-changing blocks (#2362, #718). - **`__init__.py`** — in every new/nested block package (#2351, #830). - **EE changelog** — ONLY when the block forces an EE behavior change: add an entry under `## Unreleased` in the EE changelog in the roboflow/docs repo (`workflows/developer-guide/execution-engine-changelog.md`); maintainers bump `EXECUTION_ENGINE_V1_VERSION` at release time. ## Key files & entry points - `inference/core/workflows/core_steps/loader.py` — `load_blocks()`, `load_kinds()`, `KINDS_SERIALIZERS`/`KINDS_DESERIALIZERS`, `REGISTERED_INITIALIZERS`. - `inference/core/workflows/core_steps/{category}/{block}/vN.py` — block modules (manifest + block class). - `inference/core/workflows/core_steps/common/{serializers,deserializers,utils}.py` — `sv.Detections` <-> wire conversion, keypoints/mask helpers. - `inference/core/workflows/core_steps/common/query_language/**` — UQL operations/evaluation engine. - `inference/core/workflows/execution_engine/entities/types.py` — `Kind` definitions + `*_KIND_DOCS`. - `inference/core/workflows/execution_engine/entities/base.py` — `WorkflowImageData`, `Batch`, `OutputDefinition`. - `inference/core/workflows/prototypes/block.py` — `WorkflowBlock`, `WorkflowBlockManifest`, `BlockResult`. - `inference/core/workflows/execution_engine/v1/core.py` — `EXECUTION_ENGINE_V1_VERSION`. - Docs: the Workflows developer guide on docs.roboflow.com (create a block, versioning, block bundling, EE changelog): <https://docs.roboflow.com/workflows/developer-guide/developer-guide>. - Tests: `tests/workflows/unit_tests/core_steps/**`, `tests/workflows/integration_tests/execution/**`, `test_init_files.py`. ## Reference PRs - [#2362](https://github.com/roboflow/inference/pull/2362) — new fusion block + new kind + both test tiers (canonical new-block shape). - [#2346](https://github.com/roboflow/inference/pull/2346) — missing declared output key on empty branch (output-contract). - [#2170](https://github.com/roboflow/inference/pull/2170) — object-dtype ragged keypoint arrays break supervision. - [#1368](https://github.com/roboflow/inference/pull/1368) — `sv.Detections.data` must hold numpy arrays + serializer parity. - [#2384](https://github.com/roboflow/inference/pull/2384) — backward-compat input field added in-place to v1/v2/v3 with defaults. - [#804](https://github.com/roboflow/inference/pull/804) — new `INFERENCE_ID_KIND` across blocks (kinds discipline). - [#565](https://github.com/roboflow/inference/pull/565) — Workflows block versioning foundation. - [#2351](https://github.com/roboflow/inference/pull/2351) — missing `__init__.py` (packaging/docs). - [#2527](https://github.com/roboflow/inference/pull/2527) — revert: block removal + EE version rollback + test re-assert. - [#658](https://github.com/roboflow/inference/pull/658) — stray `default=None` in manifests.