Skip to content

fix(autodiff): close four gaps between the two expansion passes - #139

Open
AlekseiChirkovVention wants to merge 3 commits into
chore/mean-expansion-regression-coveragefrom
fix/expansion-artifact-guards
Open

fix(autodiff): close four gaps between the two expansion passes#139
AlekseiChirkovVention wants to merge 3 commits into
chore/mean-expansion-regression-coveragefrom
fix/expansion-artifact-guards

Conversation

@AlekseiChirkovVention

Copy link
Copy Markdown
Collaborator

Close four defects that only became visible once the whole change was read as one artifact.

Why these were not caught earlier

Each branch in this series was reviewed against its own diff. That bounded review is what kept
each one tractable, but it means no reviewer was positioned to see an inconsistency between
two branches. All four fixes here are of that kind, and each was reproduced against the code
before being approved — none is speculative.

The four

The two passes held contradictory contracts for the same invariant. The gradient path
indexes value ids a program merely reads and documents why; the forward path indexed only
values it produces. So a generated identifier could collide with a value a node already reads,
and the result was not an error — the consumer was silently rewired to read a generated
constant instead. Both passes now derive collision candidates from one shared helper covering
every identifier an artifact mentions: produced, read, declared as an input, declared as an
output. The gradient side gained a position it was also missing.

Sharing one helper is the point. Correcting the forward side by copying the gradient side's
logic into a second place would have left the same drift to recur.

The constant reader was not authoritative about its own node's declared type. A node
declaring one dtype and shape while its parameters said another read back as the parameters,
with no complaint — though the declaration is documented as the single authority a consumer
materializes from. A handler would have allocated one tensor while the graph declared another.
The cross-check runs after parameter validation, so every existing rejection keeps its category
and a node with no declaration still reads.

An oversized declared shape raised a bare builtin. An operand declaring very large
dimensions passes every clause of the supported-reduction predicate and then overflowed inside
the reciprocal. It now fails validation naming the node and the count.

The threshold is deliberate: counts are rejected only when they cannot be converted, not when
they exceed the exactness limit. A count above the latter still expands to well within the
documented tolerance, so rejecting there would have narrowed the supported domain rather than
guarded it — a behaviour change wearing a guard's clothing.

A documented requirement had no test. The documentation calls out that the rank-reducing
tier needs a reshape handler in both artifacts, because the derivative transform's own seed
reshape survives expansion. That behaviour was correct and pinned by nothing; the missing
fail-closed case now covers it.

Why this is safe on top of eight reviewed branches

These are guards. For any well-formed artifact both passes emit exactly what they emitted
before, and every pre-existing test passes unmodified — that is the regression signal, and
review confirmed it by expanding sixteen well-formed artifacts through all four entry points
against the previous implementation and finding zero differences.

Focused suite 895 → 910, exactly the new cases. Broad non-integration suite: 1161 passed,
9 failed, 1 skipped — the same 9 environment-dependent failures, identical in composition, that
this series has carried from its first branch.

Two test amendments, both disclosed

One pre-existing test was strengthened, as this fix-task's own plan required: the agreement test
built both sides from a single dict and so could not fail. It now spells them independently and
asserts a disagreeing declaration is rejected. The three original assertions survive verbatim.

One test authored earlier on this branch had its fixture corrected: it fed the reserved
identifier into the very region being expanded, which stopped the region matching, so nothing
was minted and the collision it asserts could never fire. It was red, but for the wrong reason,
and would have stayed red permanently. Assertions unchanged. Worth stating plainly, because a
self-authored test that quietly stops testing what it claims is the same class of defect this
branch exists to close.

Stack position

Stack position 9 of 9 — based on chore/mean-expansion-regression-coverage.

The tip of the series. Cannot merge into main before the eight branches below it.

@AlekseiChirkovVention
AlekseiChirkovVention marked this pull request as ready for review August 28, 2026 06:19
@AlekseiChirkovVention AlekseiChirkovVention self-assigned this Aug 28, 2026
Each case was reproduced against the current code before being written, so
none is speculative.

The constant reader returns a descriptor that can disagree with the node's own
declared type, though the declaration is documented as the single authority a
consumer materializes from. The agreement test could not catch it: it built
both sides from one dict. It now spells them independently and asserts a
disagreeing declaration is rejected.

A mean whose operand declares dimensions too large to convert passes every
clause of the supported-reduction predicate and then raises a bare builtin from
the reciprocal. A large-but-convertible count is pinned as still expanding, so
the boundary is explicit rather than implied.

The two passes disagree about which identifiers a minted one must avoid. The
gradient pass indexes values a program merely reads; the forward pass indexes
only produced values, so a minted identifier can alias a value a node reads and
silently rewire that consumer instead of failing closed. Four cases pin both
passes against both positions.

Two further cases pin the handler requirement the documentation calls the most
likely omission: a rank-reducing derivative program lowered without a reshape
handler must fail closed on the seed reshape the rewrite leaves in place. That
behaviour is already correct and was covered by nothing.
The gradient read-position case fed the reserved identifier into the chain it
was expanding, which stopped the region matching, so nothing was minted and the
collision it asserts could never fire. Read the identifier from a node outside
the region instead. Assertions unchanged.
The constant reader now enforces the agreement its node's declaration is
documented to be the authority for. A descriptor disagreeing with the declared
dtype or shape is rejected naming the node, so a consumer cannot materialize
one tensor while the graph declares another. The check runs after parameter
validation, so every existing rejection keeps its category, and a node with no
declaration still reads.

An operand declaring dimensions too large to convert now fails validation
naming the node and the count, rather than raising a bare builtin from the
reciprocal. Such a shape violates no clause -- it is a positive integer -- so
this is a limit on what the expansion can express, which is what the residual
category covers. The threshold is the conversion limit, not the exactness
limit: a count above the latter still expands to well within the documented
tolerance, and refusing it would narrow the supported domain rather than guard
it.

Both passes now derive collision candidates from one helper covering every
identifier an artifact mentions -- produced, read, declared as an input, and
declared as an output. Previously each half indexed a different set, so the
forward pass could mint an identifier a node already read and silently rewire
that consumer to a generated constant instead of failing closed. Sharing one
helper removes the contradiction rather than correcting it in two places.
@AlekseiChirkovVention
AlekseiChirkovVention force-pushed the fix/expansion-artifact-guards branch from 48775b4 to 18292ee Compare August 28, 2026 08:02
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.

3 participants