Skip to content

Inline raw multimodal images in the v1 train client - #2120

Open
eligotts wants to merge 23 commits into
mainfrom
feat/v1-inline-raw-images
Open

Inline raw multimodal images in the v1 train client#2120
eligotts wants to merge 23 commits into
mainfrom
feat/v1-inline-raw-images

Conversation

@eligotts

@eligotts eligotts commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Intermediate multimodal storage mode — the inline bundle. Keeps the v1 raw-descriptor training contract (sidecars carry renderer descriptors + hashes + placeholders; processed tensors are rejected) but base64 data images stay inline in message content end to end — no ingress offload, no shared image directory.

  • Removed verifiers/utils/multimodal.py (the ingress offload walker), the Client.prepare_request_body hook, its TrainClient override, and the interception server's request-preparation step: requests flow to the model boundary unmodified.
  • graph.py sidecar validation requires the inline raw_image_data field on raw descriptors; processed-tensor payloads (pixel_values etc.) are still rejected.
  • The v0 renderer_client passes messages through unmodified.
  • Rollout records and traces carry the inline base64 images (deliberate for this bundle — expect larger results.jsonl on multimodal runs).

Relationship to the offload bundle

Cut from the offload branch (#1746); same processor-ownership architecture (renderer geometry math, vLLM-front + trainer materialization in prime-rl), minus the storage layer. The offload bundle (renderers #89 / verifiers #1746 / prime-rl #2836) adds content-addressed file:// offload at ingress on top of this contract.

Companion inline PRs: renderers #110, prime-rl feat/v1-inline-mm.

Validation

  • Full suite with the inline renderers checkout (minus PRIME_API_KEY-gated e2e/envs): all passing.

Note

Inline raw multimodal images in the v1 train client turn bridging

  • Multimodal prompts can now bridge turns in TrainClient.get_response: the renderer's previous_multi_modal_data (raw image descriptors) is passed through bridge_to_next_turn instead of blocking bridging entirely.
  • Adds validation helpers in graph.py that reject processed multimodal payloads (pixel_values, image_embeds, image_features) and require raw raw_image_data descriptors throughout serialization, deserialization, and attribution.
  • PendingTurn.previous_multi_modal_data merges multimodal sidecars (items, hashes, placeholders) across the reusable prefix to supply bridging context.
  • Branch.multi_modal_data and _attribute_mm now include mm_placeholders in their merged multimodal payloads.
  • Legacy token conversion in legacy.py now preserves multimodal sidecar data in TurnTokens and derives rollout outputs from state to include live trajectories.
  • Risk: serialization and attribution now raise errors for any processed multimodal payload, which is a breaking change for callers that previously passed pre-processed pixel data.

Macroscope summarized 10da86a.

Update: merged main — renderer pool reconciliation (10da86af0)

Merged verifiers main (44 commits), same train.py resolution as the offload branch (#1746): adopted the process-shared renderer pool (#2218) and slot.run, dropped the multimodal bridging gate (this PR's raw inline descriptors make mm bridging safe), re-applied previous_multi_modal_data bridge kwargs. Additionally:

  • graph.py: kept this branch's raw-mm sidecar validators; took main's MessageNode(BaseModel) base (main's pydantic refactor refactor(v1): reuse Pydantic adapters across validation #2233).
  • trace.py: took the already-reconciled shape — main has upstreamed the sidecar EXCLUDE_FIELDS (multi_modal_data/routed_experts/kept_tokens), so this branch's parallel _NODE_DUMP_EXCLUDE is gone.

Full suite: 913 passed, 67 skipped.


Note

Medium Risk
Processed multimodal sidecars now fail validation (breaking for old callers), and multimodal turn bridging changes training/inference token paths; legacy rollout output construction is also altered.

Overview
Multimodal sidecars now follow a raw inline descriptor contract: items must include raw_image_data, and payloads with processed keys (pixel_values, image_embeds, image_features) are rejected on validate/serialize/deserialize in graph.py. Wire serialization stores plain descriptor dicts instead of numpy-encoded tensors; per-node attribution and Branch.multi_modal_data also carry placeholder ranges alongside hashes and items.

Train turn bridging no longer blocks on image content in the prompt. When the incremental tail is valid, TrainClient passes merged prefix sidecars via PendingTurn.previous_multi_modal_data() into bridge_to_next_turn for multimodal renderers (is_multimodal).

Legacy v0 bridge maps multi_modal_data on TurnTokens and builds rollout output from in-memory state (live trajectory) so cumulative multimodal sidecars are not lost to JSON save deltas when converting to v1 traces.

Docs/comments in shared types describe sidecars as raw descriptors rather than processor tensors.

Reviewed by Cursor Bugbot for commit c71aad5. Bugbot is set up for automated code reviews on this repo. Configure here.

Update: current-main integration (1e35f768b)

Merged verifiers main at 8233c8446. The sole content conflict retained this PR's multimodal bridge (is_multimodal plus previous_multi_modal_data); main's v1/config/tooling changes merged around it.

Validation after the merge: touched-file Ruff and format checks passed; tests/v1/test_graph.py, tests/v1/test_legacy.py, and tests/test_renderer_client.py passed against renderers PR #110 at 50ce3c3e (two PRIME_API_KEY-gated tests skipped).

eligotts and others added 19 commits June 25, 2026 06:39
Every image carries its ref, so no cache miss can occur. Removes _generate_with_image_ref_retry / _has_descriptor_only_images / _retryable_mm_error_type / _json_error_type / _RETRYABLE_MM_ERROR_TYPES; rollouts call generate() directly. Obsolete retry tests removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fload

# Conflicts:
#	verifiers/v1/clients/train.py
…fload

# Conflicts:
#	verifiers/v1/cli/dashboard/eval.py
- prepare_images_inplace handles the full renderer part treaty: nested
  image_url dicts, direct-string image_url, direct image strings, and
  typed pydantic parts; non-string sources raise with the shape named.
- Interception server labels prepare_messages failures as
  InterceptionError instead of misattributing them to the user simulator.
- Test covers all shapes plus http rejection (skips until the renderers
  pin ships mm_store).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fload

Reconciles this branch's raw-image offload work with main's independent
retry-coalescing and harness-segment-resume refactor of the interception
server, and its v1 trace/graph model evolution:

- interception/server.py: main's single-shot handle_request (graph-atomicity
  retry coalescing) and _stream (record_call/error tracking, tools threading)
  are authoritative. Dropped this branch's obsolete mid-request user-simulator
  injection (session.user/session.opening, prepare_messages call sites) —
  main's d21100b moved that to Harness.launch/resume, so injected messages
  now re-enter as normal request bodies already covered by
  prepare_request_body (kept, re-wired at the top of handle_request).
- graph.py: kept finish_reason/usage/multi_modal_data/previous_multi_modal_data
  (this branch) alongside main's SkipJsonSchema wrapping convention and
  commit()'s new (tools, -> assistant node id) signature. Caught and fixed an
  auto-merge dropping the FinishReason/Usage imports (caused a pydantic
  model-not-fully-defined failure at Trace construction).
- trace.py: kept this branch's more accurate multi_modal_data docstring;
  took main's tuple-based bridge-mutation assertion in the test suite.
- ARCHITECTURE.md: main deleted this file (moved to the shorter docs/v1/
  architecture.md, which doesn't cover this depth of internals) — accepted
  the deletion; folded the one load-bearing fact (why multi_modal_data is
  JSON-excluded) into _NODE_DUMP_EXCLUDE's docstring instead of resurrecting
  a dedicated architecture doc.

Full suite (with local renderers checkout, minus PRIME_API_KEY-gated e2e):
all passing.
Its only callers were the interception server's mid-request user-simulator
injection sites, which main's harness-segment-resume refactor removed —
resumed user turns now re-enter as ordinary request bodies, already covered
by prepare_request_body at request ingress.
Intermediate storage mode: base64 data images stay inline in messages
through the interception server, trace graph, and rollout records; raw
descriptor sidecars carry the inline source (raw_image_data) instead of
file:// run-asset URIs.

- Removed verifiers/utils/multimodal.py (the ingress offload walker),
  the Client.prepare_request_body hook, its TrainClient override, and
  the interception server's request-preparation step.
- graph.py sidecar validation requires the inline raw_image_data field;
  processed-tensor payloads are still rejected.
- v0 renderer_client passes messages through unmodified.

Suite (with the inline renderers checkout, minus PRIME_API_KEY-gated
e2e/envs): all passing.
Comment thread verifiers/v1/graph.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

MessageNode.model_construct(

The assistant node created in _commit_turn omits finish_reason and usage, so every committed response records finish_reason=None and usage=None even though Response carries those values. Truncation detection and provider usage/cost dashboards cannot work because the data is silently dropped. The MessageNode.model_construct(...) call for the assistant node needs to pass finish_reason=response.finish_reason and usage=response.usage.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/graph.py around line 633:

The assistant node created in `_commit_turn` omits `finish_reason` and `usage`, so every committed response records `finish_reason=None` and `usage=None` even though `Response` carries those values. Truncation detection and provider usage/cost dashboards cannot work because the data is silently dropped. The `MessageNode.model_construct(...)` call for the assistant node needs to pass `finish_reason=response.finish_reason` and `usage=response.usage`.

@eligotts
eligotts marked this pull request as ready for review July 27, 2026 23:40
Comment thread verifiers/v1/graph.py
@macroscopeapp

macroscopeapp Bot commented Jul 27, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This PR makes significant changes to how multimodal images are processed and validated in the training pipeline. Two unresolved review comments identify bugs: missing finish_reason/usage population in committed nodes, and validation that can leave orphan nodes on failed payloads.

You can customize Macroscope's approvability policy. Learn more.

eligotts and others added 3 commits July 27, 2026 23:51
# Conflicts:
#	verifiers/v1/clients/train.py
…ages

Reconciled with main's renderer pool (#2218) the same way as the offload
branch: adopted RendererSlot/ElasticRendererPool and slot.run, dropped
the multimodal bridging gate (this PR's feature), re-applied
previous_multi_modal_data bridge kwargs. graph.py keeps the raw-mm
sidecar validators over main's BaseModel MessageNode; trace.py takes the
already-reconciled shape (main upstreamed the sidecar EXCLUDE_FIELDS).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 10da86a. Configure here.

Comment thread verifiers/v1/graph.py
…ages

One conflict in graph.py: main's per-token advantages field (#2245)
landed adjacent to this branch's finish_reason on MessageNode — union
of both.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants