Repository navigation
Container-aware control flow: If/Loop sequence threading (#449 inc3a) - #527
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
========================================
Coverage 80.58% 80.59%
========================================
Files 315 315
Lines 122900 123077 +177
Branches 122900 123077 +177
========================================
+ Hits 99037 99191 +154
- Misses 19838 19859 +21
- Partials 4025 4027 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… flow (#449 inc3a) Increment 3a of the #449 container-type roadmap: sequence/optional/map `ValueType`s now propagate through `If` branches and `Loop` carried dependencies, the two headline control-flow cases where sequences are used in real models (an If returning a sequence from both branches; a Loop carrying a sequence accumulator). - If: output container type = unify(then_out, else_out) via a new recursive `unify_value_type`; extent disagreement degrades to a fresh symbol, dtype disagreement and container-vs-tensor branches are errors. - Loop: a container carried operand seeds the body formal input (`body_container_seeds`); the carried body-output container type flows back to the Loop output, remapped to the parent symbol space. Scan outputs stack tensors and are never containers. - `InferenceReport.containers` is the observation + body->parent threading channel; `infer_graph_scoped` gains a `seed_containers` param and raises the child interner floor above seeded container symbols. - DRY: promoted the foundation's merge_tensor/merge_shape to context::unify_tensor_type/merge_element_shape (shared by handlers and infer.rs); container symbol remap mirrors the tensor CF per-dim rule. - Byte-identical tensor path preserved (all container work gated on a non-empty container map; no extra fresh_dim on tensor graphs). Regression + all existing If/Loop/Scan tensor tests stay green. Scoped out: Scan container state vars (inc3b); SequenceMap + cross-subgraph container capture (inc4). Tests: graph_inference 27 -> 33 (+6 container CF tests). Catalog unchanged (217 ops / 262 entries — no new operators). refs #449 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5808e94 to
684a60d
Compare
|
VERDICT: APPROVE Independent review by Lori (reviewer). Author Harry locked out. Worktree 1. Byte-identical tensor control-flow [CRITICAL] — PASS, gate is realThe container path is gated on a genuinely-empty container map on tensor graphs:
2. If unify semantics + disagreement — PASS, honest
3. Loop carried-container threading + remap — PASS, sound
4. Tests meaningful (assert types, not non-crash) — PASSgraph_inference 27→33 (+6). All six assert real container types: matching-branch unify keeps concrete [2,3]; extent-disagreement asserts a symbol in slot 1 with slot 0 concrete; two disagreement tests assert error attribution ("If"/"container"); Loop accumulator asserts element shape [2,3] preserved across SequenceInsert; passthrough asserts dtype + concrete-dim preservation + symbolic degradation. None are non-panic-only. 5. DRY promotion genuine — PASSOld 6. Verification — ALL GREEN
Scoped-out items (inc3b Scan container state vars; inc4 SequenceMap + cross-subgraph capture with |
✅ Benchmarks — No RegressionComparison 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
|
…aph container capture + Scan state Closeout slice of the #449 container-type arc, stacked on inc3a (#527). - SequenceMap (opset 17): maps a subgraph body over sequence element(s), wrapping each body output back as a Sequence. Handled in infer_control_flow like If/Loop/Scan (subgraph ops are not registry rules). Zips multiple input sequences; broadcasts additional tensor inputs. Composed from the inc3a body-threading driver (set_body_input / body_container_seeds / read_body_output / map_container_to_parent) — no fork. - Cross-subgraph container capture: remap_node_io now threads the captured value's full ValueType (new remap_container_type helper, mirroring the tensor path via remap_dim_expr) instead of dropping it; bind_captures / extend_visible_scope gained a containers channel so a body referencing an outer-scope sequence sees its element type. - Scan container state (folded in, previously deferred inc3b): Scan state vars can be containers; reuses seed_container_input / body_container_seeds and infer_scan_outputs' container check. All four subgraph ops now container-complete. Byte-identical tensor path preserved (every new path gated on a container being present; regression test stays green). Catalog stays 217 ops / 262 entries — SequenceMap is a control-flow op, not a registry rule (no phantom bump). Tests: graph_inference 33 -> 39 (SequenceMap identity/transform/zip/broadcast, If capturing an outer-scope sequence, Scan carrying a sequence state var). refs #449 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ture + Scan state (#531) ## #449 increment 4 — closeout slice (DRAFT — do NOT merge) Rebased onto **main** now that inc3a (#527) is merged — a trivial superset (single commit on top of main). CPU-only, pure `onnx-runtime-shape-inference` crate. Closes out the #449 container-type arc: foundation → seq ops → seq↔tensor → If/Loop control flow → **this**. ### What landed 1. **SequenceMap (opset 17)** — maps a subgraph body over sequence element(s), wrapping each body output back as a `Sequence`. Handled in `infer_control_flow` like `If`/`Loop`/`Scan` (subgraph ops are not registry rules). Zips multiple input sequences; broadcasts extra tensor inputs. **Composed from the inc3a body-threading driver** (`set_body_input` / `body_container_seeds` / `read_body_output` / `map_container_to_parent`) — no fork. 2. **Cross-subgraph container capture** — `remap_node_io` previously dropped a captured value's `ValueType` (`value_type: None`). New `remap_container_type` threads the full container type (recursing, remapping tensor-leaf dims via `remap_dim_expr`, mirroring the tensor path). `bind_captures` / `extend_visible_scope` gained a `containers` channel, so a body referencing an outer-scope sequence now sees its element type. 3. **Scan container state** (folded in — previously deferred inc3b) — Scan state vars can be containers; reuses `seed_container_input` / `body_container_seeds` + `infer_scan_outputs`' container check. All four subgraph ops now container-complete. ### Hard bar - **Byte-identical tensor path preserved** — every new path is gated on a container being present; regression test `tensor_only_path_is_byte_identical_after_container_type_model` stays GREEN. - **DRY** — reuses inc3a body-threading helpers + foundation `unify_value_type`/`unify_tensor_type`/`merge_element_shape`. No per-op copy-paste. - **Catalog stays 217 ops / 262 entries** (HONEST) — SequenceMap is a control-flow op, not a registry rule (like `If`/`Loop`/`Scan`); there is no 218th handler, so bumping the pin would be a phantom declaration. - **Tests**: `graph_inference` 33 → 39 — SequenceMap identity/transform/zip/broadcast, `If` capturing an outer-scope sequence (proves the `remap_node_io` fix), `Scan` carrying a sequence state var. ### Verification (all GREEN, post-rebase) `cargo test -p onnx-runtime-shape-inference` (16 op_rules + 39 graph_inference + 275 unit + 1 doctest); `cargo fmt --all --check`; `cargo clippy -p onnx-runtime-shape-inference --all-targets -- -D warnings`; `cargo build -p onnx-runtime-session -p onnx-runtime-eager`. ### Recommendation **Close #449.** The sequence container-type arc is complete end to end. Only Optional/Map op handlers remain — not implemented, not used by any in-tree target model, no real-model demand → optional low-priority follow-up, not a load-bearing gap. refs #449 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follows #486 (inc2 sequence mutations + conversions, now merged). Rebased onto
mainsince #486's branch was deleted on merge. refs #449. CPU-only, pure shape-inference crate.Summary
Increment 3a of the #449 container-type roadmap:
ValueType(Sequence/Optional/Map) container types now thread through If branches and Loop carried dependencies — the two headline control-flow cases where sequences are actually used in real models (an If returning a sequence from both branches; a Loop carrying a sequence accumulator).What's implemented
unify(then_out, else_out)via a new recursiveunify_value_type(Tensor⨝Tensor at the leaves reusing the foundation'sunify_tensor_type; Sequence/Optional recurse; Map requires an equal key dtype). Element-extent disagreement → fresh symbol; element dtype disagreement → error; one branch container + other tensor → error.body_container_seeds); the body threads/mutates it (SequenceInsert/Identity/passthrough); the carried body-output container type flows to the Loop output, remapped to the parent symbol space. Loop scan outputs stack tensors → never containers.InferenceReport.containersis the observation channel for tests and the body→parent threading channel.infer_graph_scopedgains aseed_containersparam and raises the child interner floor above seeded container symbols. NewCfOutput::Containervariant.DRY
Promoted the foundation's
merge_tensor/merge_shapefromhandlers/container.rstocontext.rsasunify_tensor_type/merge_element_shape(shared by the container handlers andinfer.rs). Container symbol remap mirrors the exact per-dim rule the tensor CF path uses.Byte-identical tensor path
All container work is gated on a non-empty container map — empty for pure-tensor graphs, no extra
fresh_dimminted → symbol numbering unchanged.tensor_only_path_is_byte_identical_after_container_type_modelstays GREEN, as do all existing If/Loop/Scan tensor tests.Honest limitation
A seeded symbolic element dim crossing a Loop body degrades to a fresh parent symbol (container seeds never touch a body IR
Value, so it's absent fromchild_to_parent) — SOUND, dtype/structure/concrete extents preserved. Covered byloop_passthrough_preserves_seeded_sequence_dtype.Scoped out
remap_node_iosetsvalue_type: Nonetoday).Verification
cargo test -p onnx-runtime-shape-inference: graph_inference 27 → 33 (+6 container CF tests); op_rules 275, container 16, doctest 1 — all GREEN; byte-identical regression GREEN.cargo fmt --all --checkclean;cargo clippy -p onnx-runtime-shape-inference --all-targets -D warningsclean.cargo build -p onnx-runtime-session -p onnx-runtime-eagerGREEN.Design + status notes:
.squad/decisions/inbox/harry-449-inc3-design.md,.squad/decisions/inbox/harry-449-inc3.md.Do NOT merge.