fix(envoy): reject a rename alias that would shadow something - #701
Open
Hotragn wants to merge 1 commit into
Open
fix(envoy): reject a rename alias that would shadow something#701Hotragn wants to merge 1 commit into
rename alias that would shadow something#701Hotragn wants to merge 1 commit into
Conversation
`_bind_aliases` binds each alias with `object.__setattr__`, which overwrites
whatever name is already there without a word. Three things can be underneath,
and all three fail silently or far from the cause:
* **An `Envoy` attribute.** `rename={"b": "trace"}` replaces the `trace`
method with an envoy, so `model.trace(x)` no longer opens a trace:
TypeError: 'torch.Tensor' object does not support the context manager
protocol (missed __exit__ method)
`rename={"b": "output"}` did raise, but accidentally and from the wrong
place -- `ValueError: Cannot access 'model.output' outside of interleaving`,
which never mentions `rename`.
* **A sibling module.** `rename={"head": "b"}` leaves the real `b` in the tree
but makes `model.b` read as `head`, so an intervention written against `b`
lands on `head` and nothing says so. This is the one that worries me: no
error at any point, just wrong activations.
* **An alias an earlier key already claimed.** `{"b": "dup", "head": "dup"}`
resolves both, and one wins by dict insertion order.
`docs/usage/rename-modules.md` only advised around this ("Avoid alias names
that collide with Envoy attributes ... They will shadow or be shadowed"), and
said nothing about the sibling-module or duplicate-alias cases.
Raise at construction instead, naming the alias, the key it came from, and
what it would displace. `_alias_conflict` finds an eproperty by scanning the
MRO's own namespaces rather than via `hasattr`, so `output`'s descriptor is
detected without its `__get__` running.
Deliberately unchanged: a key that resolves to nothing is still skipped
silently. One `rename` is meant to be reusable across architectures that spell
a module differently (`{"attn": "att", "self_attn": "att"}`), and on any given
envoy only one spelling exists. Aliasing a key to its own name
(`{"attn": "attn"}`) is how that pattern is normally written, so it is treated
as a no-op rather than a self-collision -- as are two keys reaching one module
through tied weights. Tests cover all three.
Suite is unchanged apart from the additions: collection is identical to
`0.8` (853 both ways), and the full CPU run is 856 passed / 7 skipped /
0 failed. 7 of the 10 new tests fail without this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on
0.8. Found while checking whether #535 still reproduces there — it mostly doesn't (cachenavigation resolves against the envoy tree now, so Butanium's minimal repro works), but therenamebinding underneath it has three silent failure modes.Summary
_bind_aliasesbinds each alias withobject.__setattr__, which overwrites whatever name is already there without a word:Three things can be underneath.
1. An
Envoyattribute — breaks the library's entry pointmodel.traceis now an envoy, so opening a trace fails with a message about tensors:rename={"b": "output"}did raise, but accidentally and from the wrong place —ValueError: Cannot access 'model.output' outside of interleaving, which never mentionsrename.2. A sibling module — silently wrong activations
No error at any point. An intervention written against
blands onhead. This is the one that concerns me most — it produces wrong numbers rather than a traceback.3. An alias an earlier key already claimed
docs/usage/rename-modules.mdonly advised around case 1 — "Avoid alias names that collide with Envoy attributes ... They will shadow or be shadowed by those attributes" — and said nothing about 2 or 3.Fix
Raise at construction, naming the alias, the key it came from, and what it would displace:
_alias_conflictfinds an eproperty by scanning the MRO's own namespaces rather than viahasattr, sooutput's descriptor is detected without its__get__running (reading it outside interleaving raises).What is deliberately not changed
A key that resolves to nothing is still skipped silently. One
renameis meant to be reusable across architectures that spell a module differently —{"attn": "att", "self_attn": "att"}binds whichever exists — and that's the nnterp usage behind #535. Breaking it would be worse than the bug.Two related no-ops, each with a test, so that pattern keeps working:
{"attn": "attn"}) — how the cross-architecture form is normally written. Treated as a no-op, not a self-collision.The rule is identity-based: if the name already points at this envoy, nothing is displaced.
Verification
0.8The 10 new tests in
TestRenamecover each collision (sibling child,Envoy.trace, the four eproperties, duplicate alias) and each non-collision (self-alias, unresolvable key, tied weights).Checked against every existing
rename=usage in the suite first — none of the aliases in use (my_mlp,blocks,layers,first_layer,second_layer,denoiser,e,emb,zero,m,first,block_mlp,first_mlp,second,model) collide, so this is not a breaking change for anything in tree. Worth a note in release notes regardless, since it turns a previously-silent case into an error.0.8@ 8f7546c, Python 3.14.3,transformers5.15.1,torch2.9.1, CPU.