Repository navigation
Accept step output images in video-state blocks so they work in embedded workflows - #3296
Conversation
Embedded inner workflows are inlined before compilation, which replaces the child's `$inputs.image` with the parent binding. When that binding is a step output (e.g. a preprocessing block), blocks typed with WorkflowImageSelector rejected it and the merged definition failed to parse, although each workflow compiled on its own. Use Selector(kind=[IMAGE_KIND]) like the newer tracker blocks so the image can come from an input or a step. Co-Authored-By: Claude <noreply@anthropic.com>
|
👋 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.
|
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
Claude review summarySkills: review-workflows-blocks, review-topic-workflow-state-management, review-topic-backward-compat-and-versioning, review-topic-test-hygiene No blocking findings. What I checked:
Minor doubts (non-blocking)
Commands that informed the review: Reviewed at HEAD: 5b4d389 |
Release coordination
Reviewed at HEAD: 5b4d389 |
|
😎 PR passes the vibe-check and trust-me-bro verification. |
|
Maintainer review requested. Final approval and merge remain in GitHub. |
dkosowski87
left a comment
dkosowski87
left a comment
There was a problem hiding this comment.
Two reproduced P2 integration risks in the newly accepted step-output connections.
| ) | ||
| type: Literal["roboflow_core/cache_get@v1"] | ||
| image: WorkflowImageSelector = Field( | ||
| image: Selector(kind=[IMAGE_KIND]) = Field( |
There was a problem hiding this comment.
[P2] Keep a stable video identity when accepting cropped images
This now allows a static-crop output to feed Cache Get/Set, including through an embedded workflow. However, static cropping generates a new crop ID on every frame, and create_crop appends it to video_metadata.video_identifier. The cache uses that video identifier as its namespace, so successive frames from the same camera and the same crop region cannot read each other's cached values.
flowchart TD
A["Camera A, frame 1"] --> B["Same static crop: new ID crop-1"]
B --> C["Cache Set writes under camera-A + crop-1"]
D["Camera A, frame 2"] --> E["Same static crop: new ID crop-2"]
E --> F["Cache Get reads camera-A + crop-2"]
F --> G["Cache miss: previous value is in a different namespace"]
C -. "Value remains here" .-> G
I reproduced this by replacing grayscale with absolute_static_crop@v1 in the new embedded-cache test scenario. Three frames from one camera produced three cache misses and three retained namespaces. The grayscale control produced one namespace and hits after the first frame. On a long-running stream, the crop path also keeps adding namespaces until the cache blocks are closed.
The crop behavior predates this PR; the newly enabled connection is what exposes it. Previously this workflow failed selector validation rather than running with disconnected state. Please preserve a stable source-video identity, with a stable crop-region identity where separate crop state is needed, and add a multi-frame regression test. The root image's parent ID alone is not necessarily a camera identity.
There was a problem hiding this comment.
Fixed in 978e5c2. create_crop / create_crop_from_tensor take an optional video_crop_identifier. When it's set, it's used for the | crop: suffix in place of crop_identifier. The absolute and relative static crops pass their clamped pixel box (absolute_static_crop:<x_min>,<y_min>,<x_max>,<y_max>). The source video ID is unchanged, the crop region gets an ID that's the same on every frame, and parent_id is still a unique UUID per crop. Other callers (dynamic crop, slicer) don't change.
Regression test: test_embedded_cache_keeps_one_namespace_per_video_across_frames, parametrized over grayscale, absolute and relative static crop. It runs 3 frames from cam-a and 1 from cam-b, then asserts cache hits after the first frame and exactly one retained namespace per camera. Without the fix, both crop cases fail.
| examples=[["$steps.upload"]], | ||
| ) | ||
| video_reference_image: Optional[WorkflowImageSelector] = Field( | ||
| video_reference_image: Optional[Selector(kind=[IMAGE_KIND])] = Field( |
There was a problem hiding this comment.
[P2] Keep video time advancing for calibration outputs
This now allows Camera Calibration's output as video_reference_image, including through an embedded workflow. Calibration creates its output without the source video metadata. Reading image.video_metadata then supplies fallback values of frame number 0 and FPS 30. The rate limiter calculates time as frame_number / fps, so every frame appears to be at time zero.
flowchart TD
A["Video frames with advancing frame numbers"] --> B["Camera Calibration drops video metadata"]
B --> C["Fallback metadata: frame 0, FPS 30"]
C --> D["Rate limiter sees time 0 on every frame"]
D --> E["First frame: allow and save time 0"]
E --> F["Later frames: elapsed time = 0 - 0"]
F --> G["Positive cooldown never expires: suppress downstream steps"]
I reproduced this with frames 1, 3, and 5 at 1 FPS and a one-second cooldown. All three should pass. With calibration, only the first frame reached the downstream Cache Set; the grayscale control passed all three. The system-clock fallback does not help: reading the default metadata succeeds, so the limiter overwrites system time with zero without raising an exception.
The metadata loss predates this PR; previously this connection failed selector validation. It now compiles but silently stops downstream processing after the first frame. Please preserve the original video metadata through calibration, or explicitly handle missing metadata in the limiter, and cover this with a multi-frame regression test.
There was a problem hiding this comment.
Fixed in 978e5c2 by keeping the metadata at the source. Camera Calibration now builds its output with WorkflowImageData.copy_and_replace(origin_image_data=image, numpy_image=dst), so it keeps video_metadata. It also keeps workflow_root_ancestor_metadata, which it used to reset to the input's parent. That's the same pattern grayscale and the other single-image transforms use.
Regression test: test_embedded_rate_limiter_follows_video_time_of_step_output_image, parametrized over grayscale and calibration. It runs frames 1, 2, 3 and 5 at 1 FPS with a 1 s cooldown, and every frame reaches the downstream step. The calibration case fails without the fix.
Not changed here: the rate limiter's fallback still never fires when an image has no video metadata. The video_metadata property synthesizes frame 0 / 30 FPS instead of raising, so the except meant to fall back to wall-clock time never runs. Outputs that build a fresh WorkflowImageData, like stitch_images and grid visualization, still hit that path. I'd fix it in a separate PR, since it also changes how still images behave.
Static crops appended the per-frame crop UUID to video_identifier, so stateful blocks fed by a crop saw a new video every frame. They now pass a stable region identifier via create_crop's new video_crop_identifier, keeping a unique parent_id. Camera Calibration rebuilt its output without video metadata, which pinned the rate limiter's video time at zero; it now uses copy_and_replace. Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
Claude review summary (re-review:
|
|
😎 PR passes the vibe-check and trust-me-bro verification. |
What
Video-state blocks typed
imageasWorkflowImageSelector, which only matches$inputs.*. They now useSelector(kind=[IMAGE_KIND]), the same type the newer tracker blocks (trackers/*/v1,byte_tracker@v3,stabilize_detections@v1) already use, so the image can come from a Workflow input or a step output.Affected blocks (and their
_tensorsiblings):time_in_zone@v2/@v3,velocity@v1,line_counter@v2,path_deviation_analytics@v2,detection_event_log@v1,byte_tracker@v2,cache_get@v1,cache_set@v1,delta_filter@v1, andrate_limiter@v1(video_reference_image).Why
Before compiling, embedded
inner_workflow@v1steps are inlined, which replaces the child's$inputs.imagewith the parent's binding. If the parent bindsimageto a step output (for example a preprocessing block), each of these blocks fails selector validation. The whole Workflow then fails withCould not parse workflow definition, even though both the parent and the child compile on their own.This took down all 4 GPA Fontana streams (renaissant). The shared inner Workflow started passing
$steps.standard_image.imageinto the nestedvehicle-event-cacheWorkflow, andtime_in_zone@v3,velocity@v1,cache_get@v1andcache_set@v1rejected it.Note: the fifth failing block in that Workflow,
VehicleLifecycleBuffer, is a customer custom Python block. Its manifest is defined in the customer's Workflow, so this PR doesn't change it.Compatibility
$inputs.*selector that was accepted before is still accepted.video_metadatafrom the image. Step outputs built withWorkflowImageData.copy_and_replace(preprocessing, grayscale, etc.) keep it, so per-video state keys don't change. The new test checks this end to end for the cache namespace.Tests
workflows/tests/unit_tests/execution_engine/inner_workflow/test_embedded_video_blocks_with_step_output_image.py:convert_grayscale→inner_workflowwhose child uses every affected block compiles. Without the fix it fails with 11string_pattern_mismatcherrors, the same failure as in production.cache_get/cache_setround trip with a step-output image keeps a separate cache namespace pervideo_identifier.cd workflows && pytest tests/unit_tests/execution_engine/inner_workflow tests/unit_tests/core_steps/{cache,analytics,control_flow}→ 307 passed. The new test also passes withENABLE_TENSOR_DATA_REPRESENTATION=True.cd workflows && pytest tests/unit_tests tests/isolation: no new failures. The 75 failures with this change are a subset of the 77 onmainin my local env (CPU-only GPU parity tests, missingfastapi, enterprise census); the 2 extra onmainare the new tests.tests/workflows/suites.🤖 Generated with Claude Code