Fix cache navigation through collapsing/re-mount renames - #681
Closed
Butanium wants to merge 2 commits into
Closed
Conversation
Attribute-style access into a cache (e.g. `cache.model.layers[0].output`)
raised `AttributeError` when a rename re-mounts a nested module at the root
via a dotted rename key (e.g. `{"model.layers": "layers"}`). Two root causes:
- `CacheDict.__getattr__`/int-indexing only matched navigation paths against
the real storage keys and the lossy inverted leaf-alias map, never the
authoritative `_alias_paths` (alias-path -> real-key) map. A collapsing
rename's alias path is not a prefix of any real key, so navigation dead-ended.
- `_add_alias_path` built alias paths with naive substring replacement, which
also rewrote the fixed root component (`model.model.layers.0` ->
`foo.foo.layers.0` instead of `model.foo.layers.0`).
Fixes:
- Navigate the `_alias_paths` keyspace as a fallback route in `__getattr__`
and int-indexing; resolve a landed `_path` back to its real storage key in
`output`/`inputs`/`input`.
- Protect the root component in `_add_alias_path`.
- Copy underlying storage into sub-views based on raw dict length, so an
alias-space sub-view (scoped length 0) still propagates storage to children.
Adds regression tests for both the collapsing-rename and root-preservation
cases from the issue.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGecSNRudbfxSX14gsBco5
…ame cache tests Follow-up to the #535 fix. Out-of-bounds indexing on a renamed/collapsed modulelist (e.g. `cache.model.layers[999]`) leaked a `KeyError` instead of the `IndexError` contract honored by non-renamed access, because the bounds check only consulted the real storage keys. Extend it to the alias-path keyspace. Broaden the collapsing-rename regression test to also cover `.input` via a renamed path, out-of-bounds `IndexError`, and navigating into a submodule of a re-mounted block under a full (no-`modules=`) cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGecSNRudbfxSX14gsBco5
Member
Author
|
#682 for fable less sloppy version |
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
2 participants
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.
Summary
Fixes cache attribute/index navigation when using collapsing or re-mount renames (e.g.,
rename={"model.layers": "layers"}). These renames re-mount nested modules at the root, creating alias paths that don't exist in real storage keys. The cache navigator now resolves these paths correctly through an alias-path map.Key Changes
Added
_resolved_path()method: Translates aliased paths (from collapsing renames) back to their real storage keys using the_alias_pathsmap.Updated property accessors (
.output,.inputs,.input): Now use_resolved_path()to look up cache entries, enabling access through renamed paths.Added
_alias_path_prefix()helper: Checks if a path is an alias path or a prefix of one, allowing segment-by-segment navigation through the alias-path keyspace.Refactored
_add_alias_path(): Protects the root cache component from substring replacements. A rename like{"model": "foo"}now correctly producesmodel.foo.layers.0(notfoo.foo.layers.0), sincecache.modelis the fixed root.Enhanced
__getitem__()(integer indexing): Now checks both real keys and alias-path prefixes when validating numeric indices, raisingIndexErrorinstead of leakingKeyErrorfor out-of-bounds access on renamed module lists.Refactored
__getattr__()(attribute access): Implements a three-route resolution strategy:Added
_child()helper: Centralizes sub-view creation to reduce duplication.Tests
Added two new test cases:
test_cache_collapsing_rename: Validates attribute/index access through collapsing renames, including.output,.input, and out-of-bounds indexing.test_cache_rename_preserves_root: Ensures renames matching the root component name don't corrupt the fixedcache.modelroot.Implementation Details
The core issue: when a rename like
{"model.layers": "layers"}is applied, the user-facing access pathcache.model.layers[0]doesn't match any real storage key (which ismodel.transformer.h.0). The fix maintains an_alias_pathsmap that records these mappings and uses it during navigation to resolve paths correctly. The three-route resolution in__getattr__()ensures both simple renames and complex collapsing renames work seamlessly.https://claude.ai/code/session_01CGecSNRudbfxSX14gsBco5