graph_ir/runtime: honour scalar_side so a left-side scalar literal lowers correctly - #589
Conversation
…wers correctly `_OpExtractor._try_map_binop` lifts a literal operand out of a BinOp into the `scalar` attribute and records which side it came from in `scalar_side`. Nothing in python/, src/, or tools/ read that attribute — a Decision #29 violation (a declaration with no consumer). It was not merely dead. The extractor lifts scalars for `tessera.sub` and `tessera.div`, not just the commutative `add`/`mul`, so `2.0 - x` and `x - 2.0` emitted IROps that were identical except for the unread key. Every consumer of `scalar` binds it as the RIGHT operand, and `runtime._apple_gpu_dispatch_mpsgraph_binary` covers ten non-commutative opcodes (sub, div, pow, mod, floor_div, atan2, and all six comparisons). Fed the extractor's own kwargs it computed `x - 2.0` for both spellings — sign-flipped for `sub`, reciprocal for `div`, with no diagnostic. `_apple_gpu_execute_artifact` forwards an op's kwargs verbatim to the lane handler, so the whole path is connected. The bug is latent today only because these graphs report `compile_bundle.executable=False` and fall back to eager Python; the correct answers come from not executing the IR at all. Fix: the dispatcher honours `scalar_side`, swapping the operands before the Metal/numpy lane split. Absence means "right" — the definition of the `scalar=` kwarg on the eager op surface that this module's own `{"scalar": s}` call sites rely on — and any other value raises rather than being guessed (Decision #21). The two commutative-only consumers (`matmul_pipeline._execute_op`, `runtime._execute_runtime_cpu_op`) implement the scalar form for `add`/`mul` alone, so operand order cannot change their result; the ROCm binary lane requires two real operands. No other consumer needed a change. Deleting the attribute instead was considered and rejected: the tracer that E2E-REAL-6 puts in `_OpExtractor`'s place cannot express infix scalar binops at all (`Tracer` has no arithmetic dunders, and `record_op` refuses non-Tracer positional operands), so there is no successor to hand the case to, and refusing the lift turned `y = 2.0 - x` as an intermediate from a correct answer into a hard error on every target. tests/unit/test_binop_scalar_side.py pins all four sides of the contract: the negative fixture (left-side scalar across every non-commutative opcode), a guard that left and right actually differ per opcode, a producer→consumer round-trip feeding the extractor's own kwargs to the dispatcher — the assertion that would have caught this, since each half was self-consistent and only the join was wrong — and the tracer's fail-closed behaviour, so retiring `_OpExtractor` cannot silently inherit the bug. Reverting just the consumer fix fails 23 of the 75. Verified on the Mac (M1 Max): 13943 passed, 48 failed — all 48 fail identically with these changes stashed, every one `requires a fresh Tessera Apple GPU runtime dylib` (no build/ in this worktree). mypy clean over 467 files, ruff clean, generated-doc drift gate in sync. With no dylib present the dispatcher tests exercised its numpy fallback rather than the live MPSGraph symbol; the swap happens before that branch so both are covered by construction, but the Metal lane itself is unproven on this run. No lit suite and no ROCm/CUDA lane was run — this change touches neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a19362f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…r backends CI (Linux) failed 18 of the new tests. Root cause is NOT the operand-ordering fix: on non-Darwin hosts `_load_apple_gpu_runtime` compiles `runtime/apple_gpu_runtime_stub.cpp`, whose binary switch implements opcodes 0-8 and whose `default:` arm assigns `out[i] = x`. So `mod`(9), `floor_div`(10), the six comparisons(11-16), and the logical/bitwise ops(17-22) silently return the LEFT operand there instead of computing anything — including for calls that carry no `scalar_side` at all, which behave identically on main. The Mac loads the real MPSGraph symbol, so the first run could not see it. Consumer tests now run through BOTH dispatcher lanes via a `lane` fixture: * `host_reference` — forces the symbol lookup to miss, which is the dispatcher's own documented fallback. Deterministic on every host, so operand ordering stays covered in CI. The swap happens before the lane split, so this exercises exactly the code under test. * `live_kernel` — Darwin-gated, keeping real Metal coverage where the opcode table is fully implemented. Gated rather than probed because probing would mean asserting the very thing these tests assert; the skip reason names the stub gap so it is not mistaken for absent coverage. Mac: 123 passed (both lanes). Simulated non-Darwin: 75 passed, 48 skipped, 0 failed. The stub defect is left for its own change — it is pre-existing, independent of operand ordering, and a Decision #21 violation in its own right (a lowering the backend cannot carry must diagnose, never silently return an operand). Also records the AGENTS.md cross-backend assessment this PR owed, under sync key `SCALAR-SIDE-ORDERING-2026-08-19`: * apple — parity validated on Metal; follow-up required for the portable stub. * nvidia — not applicable; no NVIDIA path consumes the `scalar` kwarg (verified by an exhaustive sweep for `get("scalar"`/`["scalar"]`/`get("other"`). * rocm — not applicable; `_execute_rocm_compiled_binary` binds both operands positionally and raises when fewer are present, so the lifted-scalar form cannot reach the gfx1151 lane. Fails closed by construction. * x86 — not applicable; `_execute_x86_compiled_binary` raises the same way. No device evidence is produced or claimed for nvidia, rocm, or x86. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…re boundary `test_apple_test_inventory.py::test_inline_apple_capability_gates_are_globally_inventoryed` failed: the `lane` fixture's inline `pytest.skip` naming a Darwin host is an APPLE-TEST-1 inline capability gate, which that ratchet requires be migrated to the centralized boundary rather than written at the test site. The `live_kernel` parameter now carries `pytest.mark.hardware_apple_gpu`, so `tests/conftest.py`'s `require_apple_metal()` owns the gate. Same coverage, no inline gate: `host_reference` still runs everywhere and carries the operand- ordering contract in CI. My error, not a new defect — I pushed the previous commit without re-running the full suite after restructuring the file, and this reproduces locally. Full unit sweep on the Mac after this change: 13991 passed, 48 failed, and the failure set is byte-identical to the pre-existing baseline (all 48 the missing Apple runtime dylib). Zero new failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Valid finding, now addressed in 127d454. I had not read Sync key
No device evidence is produced or claimed for nvidia, rocm, or x86. A real defect this surfaced, filed separatelyCI red-flagged 18 of the new tests, and the cause was not the ordering fix. On non-Darwin hosts default: out[i] = x; break;So That is a Decision #21 violation in its own right, pre-existing, and independent of operand ordering. It is not fixed here — it deserves its own change with its own backend assessment. The follow-up is recorded in Consumer tests now run through both dispatcher lanes: |
What
_OpExtractor._try_map_binop(graph_ir.py:1946) lifts a literal operand out of aBinOpinto thescalarattribute and records which side it came from inscalar_side. Nothing inpython/,src/, ortools/read that attribute — a Decision #29 violation (a declaration with no consumer).Why it isn't merely dead
The extractor lifts scalars for
tessera.subandtessera.div, not just the commutativeadd/mul. So2.0 - xandx - 2.0emitted IROps identical except for the unread key:Every consumer of
scalarbinds it as the right operand, and_apple_gpu_dispatch_mpsgraph_binarycovers ten non-commutative opcodes (sub,div,pow,mod,floor_div,atan2, and all six comparisons):_apple_gpu_execute_artifact(runtime.py:32345) forwards each op'skwargsverbatim to the lane handler, so the path is connected end to end.Currently latent, not live. These graphs report
compile_bundle.executable=Falseand fall back to eager Python — the correct answers today come from not executing the IR at all.The fix
The dispatcher honours
scalar_side, swapping operands before the Metal/numpy lane split. Absence means"right"— the definition of thescalar=kwarg on the eager op surface that this module's own{"scalar": s}call sites rely on — and any other value raises rather than being guessed (Decision #21).Audited all
scalarconsumers; only one needed changing:runtime._apple_gpu_dispatch_mpsgraph_binarymatmul_pipeline._execute_opadd,mulruntime._execute_runtime_cpu_opadd,mulruntime._execute_rocm_compiled_binaryWhy not delete the attribute instead
That was the cheaper-looking option, and I implemented it first. It fails on its own precondition: the tracer that E2E-REAL-6 puts in
_OpExtractor's place cannot express infix scalar binops at all —Tracerhas no arithmetic dunders andrecord_oprefuses non-Tracer positional operands. There is no successor to hand the case to, and refusing the lift turnedy = 2.0 - xas an intermediate from a correct answer into a hard error on every target. When_OpExtractoris retired, droppingscalar_sidebecomes a mechanical cleanup guarded by the tests here.Tests
tests/unit/test_binop_scalar_side.py— 75 tests over four sections:_OpExtractorcannot silently inherit this.Reverting only the consumer fix fails 23 of the 75.
Verification
Mac (M1 Max), Homebrew
python33.14.6:pytest tests/unit -m "not slow"→ 13943 passed, 48 failed. All 48 fail identically with these changes stashed — every oneRuntimeError: requires a fresh Tessera Apple GPU runtime dylib(nobuild/in this worktree). Zero new failures.mypy python/tessera/→ clean, 467 files (ratchet 0).ruff check→ clean.scripts/check_generated_docs.sh→ 26 docs in sync (test_coverageregenerated via its CLI, not hand-edited).Limits of this evidence: with no dylib present, the dispatcher tests exercised its numpy fallback rather than the live MPSGraph symbol — the swap happens before that branch so both are covered by construction, but the Metal lane itself is unproven on this run. No lit suite and no ROCm/CUDA lane was run; this change touches neither.
Follow-up
A separate, independent crash surfaced while investigating:
JitFn._establish_tracer_authorityindexeslegacy.functions[0]unguarded, so any function with an unlowerable intermediate statement raises a bareIndexError. Reproducible onmain, unrelated to this diff, and shipping as a stacked PR on top of this one.🤖 Generated with Claude Code