envoy: len, iteration and indexing answer for the same entries - #725
Open
JadenFiotto-Kaufman wants to merge 1 commit into
Open
JadenFiotto-Kaufman wants to merge 1 commit into
JadenFiotto-Kaufman wants to merge 1 commit into
Conversation
A container may keep its modules one level down and reach them through its own `__getitem__` -- a set of transcoders holding them in a child `ModuleList`. Its `__len__` counts the modules while the envoy tree's children are the container's own entries, so `len(envoy)` reported eighteen while iteration yielded one and every index in that range raised. Indexing asks the container when no child answers to the name, and names the envoy of the module it returns; iteration walks it the same way. Torch's own containers are untouched: their entries are the children, so a name always resolves and the delegation never runs -- which is what keeps a shared entry at its own name, and what keeps `layers[2]` an envoy rather than a bare module. The delegation only yields an `Envoy`. A `__getitem__` returning a tensor element, or a module built on the fly that this tree does not wrap, raises rather than handing back something no trace can address. A container with no `__iter__` of its own is walked over the length it reports, since Python would otherwise synthesize iteration from `__getitem__` and stop only on `IndexError`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The bug
A container may keep its modules one level down and reach them through its own
__getitem__— a set of transcoders or SAEs holding them in a childModuleList:Envoy.__len__delegates to the module, while__iter__and__getitem__go through the envoy children. For this shape those disagree three ways:len(envoy)__len__len(list(envoy))envoy[0]AttributeErrorSo the envoy reported a length whose every index raised.
The change
Indexing asks the container when no child answers to the name, and names the envoy of the module it returns. Iteration walks it the same way.
__len__already delegated — it was the other two that disagreed with it.Torch's own containers are untouched.
ModuleList,SequentialandModuleDicthold their modules as their own entries, so a name always resolves and the delegation never runs. That is what keeps a shared entry at its own name (#721) and what keepslayers[2]an envoy rather than a bare module.The delegation only ever yields an
Envoy. A__getitem__returning a tensor element, or a module built on the fly that this tree does not wrap, raises rather than handing back something no trace can address. A container with no__iter__of its own is walked over the length it reports, since Python would otherwise synthesize iteration from__getitem__and stop only onIndexError.Behaviour
leniter[k]ModuleList/Sequential/ModuleDicttset.items[2]__iter____getitem__→ tensor elementAttributeError__getitem__→ untracked moduleAttributeErrorHow this surfaced
circuit-tracer'sTranscoderSetis exactly this shape. Before #721,model.transcoders[0]indexed_childrenpositionally and returned the ModuleList — the container's sole entry — sofor t in model.transcoders[0]iterated the transcoders by accident. #721 moved indexing to by-name, correctly, and the accident became anAttributeError.Note this PR does not restore that expression: under these semantics
transcoders[0]correctly means the first transcoder, not the list. Downstream code relying on the old positional accident still needs updating — that is the right outcome, and this PR is about thelen/iter/[]disagreement rather than about restoring it.Tests
Nine tests in a new
TestContainerDundersclass covering the torch containers, a delegating container, one with no__iter__, negative indexing, both raising cases, and tracing through a delegated index.tests/test_envoy.py: 110 passed.Full suite (excluding
tests/vllm, not installed here): 1040 passed. The 16 failures and 29 errors are alltests/tp/*andtest_tensor_parallel_rules.py, which needtorchrunon a multi-GPU box — byte-identical sets with and without this change (common the sorted lists is empty in both directions).🤖 Generated with Claude Code