Repository navigation
feat(pipeline): drive every_step components via ComponentSession trait (native multi-component inc1) - #450
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t (native multi-component inc1) Route the pipeline decode loop's every_step (step) components through the backend-neutral ComponentSession trait instead of the concrete ORT Session, so the same run_step_components code path drives an ORT session or a native nxrt component with no forked native copy. - PipelineDecodeLoopBackend.step_components becomes Vec<(StepComponentBinding, Box<dyn ComponentSession>)>. - run_step_components crosses the value-type seam: pool ORT Value -> neutral host ComponentTensor -> trait run -> ComponentTensor -> pool Value. - Add OrtComponentSessionRef, a borrowing ORT ComponentSession adapter, so the default path drives already-loaded sessions unchanged (behaviour-identical). - Select native every_step components at runtime via ONNX_GENAI_PIPELINE_NATIVE_STEP_COMPONENTS (empty/unset => all ORT). - Parity test: the tiny-gemma4-vlm embedding every_step component produces identical token ids [0,5,6,7] on ORT and native while the decoder stays ORT. The decoder itself and cross-component value handoff remain ORT-owned; that is inc2/inc3 (see .squad/decisions/inbox/mary-native-pipeline-plan.md). Refs #384. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
- Coverage 80.56% 80.56% -0.01%
==========================================
Files 314 314
Lines 122637 122637
Branches 122637 122637
==========================================
- Hits 98801 98799 -2
- Misses 19817 19818 +1
- Partials 4019 4020 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
|
VERDICT: APPROVE Independent review by Melina (senior reviewer). Author locked out; reviewed at commit 72d7374 in a clean worktree. Evidence below with file:line. 1. Seam correctness (the critical point) — LOSSLESS + unsupported-dtype-safe
Round-trip verdict: lossless and unsupported-dtype-safe. 2. Default ORT path — round-trips, proven behavior-identicalThe env-unset path is NOT short-circuited: every_step ORT components are driven through OrtComponentSessionRef (pipeline/mod.rs:426-430), so each step does pool Value -> ComponentTensor -> Value -> Session::run -> Value -> ComponentTensor -> pool Value. Because the conversion is raw-byte + 1:1 dtype + exact shape (section 1), the output Value is bit-identical to the pre-PR direct Session::run. Confirmed empirically: gemma4_vlm_pipeline_e2e (exercises the ORT every_step embedding) passes 1/1, and the parity test baselines the ORT token ids to the fixture's closed-form [0,5,6,7]. Cost note: this adds one host copy per every_step component per step; the author documents it as negligible for the small embedding outputs (mod.rs:1374-1380) — acceptable. 3. DRY — one path, no native forkstep_components is a single Vec<(StepComponentBinding, Box)> driven by one run_step_components loop (paged_decode.rs:87-92, 131-171); there is no forked native copy. OrtComponentSessionRef genuinely shares its run body with the owning OrtComponentSession via the single run_ort_component fn (component.rs:132-215). The new unit test borrowing_ref_adapter_matches_owning_adapter asserts the two adapters produce identical bytes — passes. 4. Token-parity — reproduced, real, deterministicnative_step_component_parity::native_every_step_embedding_matches_ort_token_ids passes. It sets ONNX_GENAI_PIPELINE_NATIVE_STEP_COMPONENTS=embedding inside the test, constructs the native session, greedy-decodes (temperature 0.0), asserts native == ort AND ort == [0,5,6,7] on tiny-gemma4-vlm. Real assertion on token ids, deterministic, and genuinely exercises the native path. 5. Scope honestyGenuinely the every_step slice only: the decoder still runs as a concrete ORT Session (flat_autoregressive.rs:153-156); only step_components are boxed behind the trait. Native selection is opt-in per named component via the env var; without native-backend the native request bails with an actionable message (mod.rs:412-419). Inc2/inc3 (decoder KV-cache ownership, cross-component handoff) are correctly deferred and documented, not half-wired. 6. Tests + feature sets
ConclusionThe value-type seam is lossless and fails loudly on any out-of-vocabulary dtype; the default ORT path is proven behavior-identical (extra host copy only); there is a single trait-parameterized decode path with no native fork; the token-parity test is a real, deterministic assertion that exercises the native path. No new test failures vs main. APPROVE. |
…nent trait (native multi-component inc2a) (#478) ## What & why Increment 2a of the native multi-component pipeline work (refs #384). Inc1 (#450, merged) routed the **stateless** `every_step` components through the backend-neutral `ComponentSession` trait. The **decoder** cannot reuse that seam: it is **stateful** — its KV cache grows across steps and, for the native backend, lives device-resident — so a stateless host round-trip would drop KV continuity and re-stage the whole cache every step, destroying decode throughput. This PR introduces the **stateful** counterpart and lands it as a **pure, provably-behavior-identical refactor** (no native decoder yet), de-risking the native wiring (Inc2b). ## The stateful decoder seam - New `trait PipelineDecoderComponent` (`pipeline/decoder_component.rs`): the flat autoregressive decode loop calls `step()` once per token; the impl **retains its own per-step outputs**, so the loop reads `next_token_logits()` / `mirror_last_present_kv()` without ever handling a concrete tensor type. Same DRY principle as Inc1, but stateful (KV stays inside the backend). - `OrtPipelineDecoder` is the ONNX Runtime impl, behaviour-identical to the previous inline `run_decode_step_with_extra` → mirror → extract path. - `PipelineDecodeLoopBackend` now holds `decoder: Box<dyn PipelineDecoderComponent>` instead of a concrete `&Session` + `&mut DecodeState`; `next_logits()` drives it entirely through trait methods. - Removed the now-redundant one-line `extract_next_token_logits_with_io` wrapper in favour of the slice-based `extract_next_token_logits_from_outputs` (no per-step clone of retained KV outputs); test callers updated. ## Inc2a / Inc2b split - **Inc2a (this PR):** stateful trait + ORT adapter, loop calls `.step()`, ORT token output UNCHANGED. Pure refactor. - **Inc2b (follow-up):** native decoder impl wrapping `NativeDecodeSession` keeping device-resident KV across steps + native-decoder-in-pipeline token parity. Requires extending `NativeDecodeSession::step` to accept routed/`inputs_embeds` per-step inputs and expose present-KV — substantial, its own increment. Design note: `.squad/decisions/inbox/mary-pipeline-inc2-design.md` (committed 6209729). ## Proof the ORT path is unchanged - **New equivalence unit test** `pipeline::decoder_component::tests::ort_decoder_component_matches_inline_step_path`: asserts the trait wrapper's logits equal the inline helper path **bit-for-bit** across a 3-token prefill + two decode steps on `tiny-multiaxis-state-decoder`. - Flat-AR e2e token-id goldens unchanged: `gemma4_vlm` (embedding every_step + decoder → `[0,5,6,7]`), `vlm_multibinding` (2), `multimodal_reuse` (14), `optional_modality` (8), `pipeline_executor` (1). - Inc1 native `every_step` parity still green. ## Verification - `cargo test -p onnx-genai-engine --lib`: **283 passed, 0 failed, 1 ignored**. - Flat-AR e2e: gemma4_vlm 1, vlm_multibinding 2, multimodal_reuse 14, optional_modality 8, pipeline_executor 1 — all pass. - `native_step_component_parity` (native-backend): 1 passed. - `cargo fmt --all --check`: clean. - clippy clean (no warnings) for **default / native-backend / cuda / cuda,native-backend** (cfg-correct imports).⚠️ Draft — do not merge. Stops at the largest PROVEN slice (Inc2a); Inc2b native decoder is deferred. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s native multi-component decode, #82/#384/35B-A3B) (#546) ## Summary The task was to introduce a backend-neutral ownership seam so the pipeline decode loop can drive **either** ORT **or** native component sessions per step, then route the existing ORT path through it byte-identically (increment-1). **On inspection, that seam already fully exists on `origin/main`** — it landed across the inc1→inc3c chain (#450, #478, #479, #485, #487, #533) and was hardened by #543. `PipelineDecodeLoopBackend` owns **no** ORT `Session`/decode-state; it holds only `Box<dyn PipelineDecoderComponent>` (stateful decoder seam) and `Vec<(_, Box<dyn ComponentSession>)>` (stateless every_step seam), and both ORT and native backends are driven through one decode loop via runtime env selection. So increment-1 here is the piece the chain had **not** locked: a parity test proving the seam's **keystone** end-state — *every declared component running natively at once* (native every_step embedding **+** native device-KV decoder in the same loop), the exact shape a large multi-component package (up to the 35B-A3B 3-component package) decodes through. ## What changed (test-only, zero production change) - **`crates/onnx-genai-engine/tests/native_full_pipeline_parity.rs`** — drives the `tiny-gemma4-vlm` composite with both `ONNX_GENAI_PIPELINE_NATIVE_STEP_COMPONENTS=embedding` **and** `ONNX_GENAI_PIPELINE_NATIVE_DECODER=decoder`, asserting the fully-native run is token-identical to the ORT baseline `[0, 5, 6, 7]`. - **`crates/onnx-genai-engine/Cargo.toml`** — registers the test (`required-features = ["native-backend"]`, CPU-only). - **`.squad/decisions/inbox/mary-pipeline-native-ownership.md`** — full assessment (ownership map), design affirmation, and the deferred next increment. Prior increments proved each slice in isolation: inc1 (native embedding + ORT decoder), inc2b (ORT embedding + native decoder). Nothing exercised **both** natively at once until now. ## ORT byte-identical proof No production source is touched, so the ORT decode path is byte-identical to `origin/main` by construction. Empirically the ORT baseline `[0,5,6,7]` and the fully-native run `[0,5,6,7]` match exactly. ## Tests - `native_full_pipeline_parity` — **pass** (new) - `native_step_component_parity`, `native_pipeline_decoder_parity` — **pass** - 343 engine lib unit tests — **pass**, 1 ignored - `cargo fmt --all --check` — clean CUDA-gated native tests were not run in this CPU environment (unchanged by this PR). ## Deferred to the next increment (native wiring completion) The one genuine remaining hard limitation the code itself flags (`decoder_component.rs:244-260`): `NativePipelineDecoder::mirror_last_present_kv` bails — the native decoder keeps KV session-resident and does not expose host present tensors, so native selection runs the non-paged, fresh-decode path with no cross-request KV reuse. Wiring native present-KV exposure + paged mirroring is higher blast radius and is intentionally **not** bundled here. Refs #82, #384. Working as Mary (native-decode / pipeline engineer). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What & why (refs #384)
Unblocks native decode of pipelined multi-component models (Qwen3.6-35B-A3B = embedding + decoder + vision, and any multimodal pipeline). The blocker:
PipelineDecodeLoopBackendhardcoded the concrete ORTSession/Value, so it could only drive ORT components even though native (nxrt custom-EP) component sessions load fine.This is increment 1 of the refactor scoped in
.squad/decisions/inbox/mary-native-pipeline-plan.md(committed here).The value-type seam (verdict)
The
ComponentSessiontrait boundary is a backend-neutral, host-residentComponentTensor(raw little-endian bytes), not ORTValueand not an nxrt tensor. The decode-loop pool holds ORTValue, so routing step components through the trait requires a poolValue⇄ComponentTensorconversion at the loop boundary — that host round-trip is the crux of the work.Changes
PipelineDecodeLoopBackend.step_components→Vec<(StepComponentBinding, Box<dyn ComponentSession>)>.run_step_componentscrosses the seam generically: poolValue→ComponentTensor→ComponentSession::run→ComponentTensor→ poolValue. One code path, no forked native copy.OrtComponentSessionRef<'a>: a borrowing ORTComponentSessionadapter so the default path drives already-loaded sessions unchanged (behaviour-identical); shares its run body with the owningOrtComponentSession.ONNX_GENAI_PIPELINE_NATIVE_STEP_COMPONENTS(empty/unset ⇒ all ORT, so the ORT decode path is untouched by default).Validation
native_step_component_parity::native_every_step_embedding_matches_ort_token_idsruns thetiny-gemma4-vlmcomposite pipeline with itsembeddingevery_step component on ORT (baseline[0,5,6,7]) and on native nxrt (decoder stays ORT in both), asserting identical token ids. CPU-only, deterministic.borrowing_ref_adapter_matches_owning_adapter(onnx-genai-ort).cargo fmt --all --checkclean; clippy clean for default, native-backend, cuda, and cuda,native-backend.Scope / follow-ups (honest increment boundary)
The decoder itself (
decoder: &Session,run_decode_step_with_extra, logits) and cross-component value handoff /static_cross_kv/ device placement remain ORT-owned — those are inc2/inc3 in the plan doc. This PR proves the seam on the every_step slice only.Note: a pre-existing, unrelated failure in
onnx-genai-ort(loader::model_package_tests::flat_directory_with_unrelated_manifest_remains_backward_compatible) reproduces onorigin/mainwith this branch's changes stashed — not introduced here.Do not merge (draft).