fix(util): traverse container subclasses so ModelOutput values are not silently skipped - #692
Closed
Hotragn wants to merge 1 commit into
Closed
fix(util): traverse container subclasses so ModelOutput values are not silently skipped#692Hotragn wants to merge 1 commit into
Hotragn wants to merge 1 commit into
Conversation
…t skipped `apply` and `applyn` matched the builtin container types exactly (`type(data) == list`, `== tuple`, `== dict`), so every *subclass* of them fell through to `return data` untouched. A HuggingFace `ModelOutput` is an `OrderedDict` subclass and it is what every top-level `transformers` module returns, which means `model.output` and `model.transformer.output` were invisible to all five callers of these helpers. `namedtuple`, `OrderedDict`, `defaultdict` and `torch.return_types.*` outputs were skipped for the same reason. Nothing raised -- the traversal just found no tensors -- so each caller degraded silently: * `Batcher.narrow`: an invoke reading `model.output` in a batched trace got the *whole* batch instead of its own row, and both invokes got the same object back (`a.logits is b.logits`). * `Batcher.swap`: writing through `model.transformer.output` inside one invoke landed on every other invoke's rows. * `hooks.cache_output_hook` / `cache_input_hook`: `tracer.cache(...)` stored the full batch for a `ModelOutput` module. * `Cache.add`: `cache(dtype=...)` / `cache(device=...)` left those tensors unconverted, and the default `detach=True` never fired, so the cache pinned the autograd graph. * `nnsight_forward`'s `__nnsight_skip__` merge: a multi-invoke `tracer.skip()` returned one invoke's value rather than the concatenation. Recurse into `list` / `tuple` / `dict` subclasses as well, rebuilding the concrete type so callers still get a `ModelOutput` back. Mappings and lists are rebuilt by mutating a shallow copy, because subclasses like `ModelOutput` are dataclasses whose `__init__` does not accept a mapping; going through `__setitem__` also preserves `ModelOutput`'s item/attribute mirroring, so `.logits` stays in sync with `["logits"]`. Tuples are rebuilt via `_make` for namedtuples and the iterable constructor otherwise (`torch.Size`, structsequences), falling back to a plain tuple only when the subclass cannot be rebuilt at all. The exact-type fast paths are untouched, so the hot path is unchanged: the new branches only run for values that previously fell straight through. Verified against gpt2 on CPU: each invoke's narrowed `ModelOutput` now matches the same prompt traced alone to a relative error of 2e-7, while differing from the other invoke's reference by 1.7e-1.
Author
|
Closing this — checked it against
And the behaviour I was fixing holds on
|
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.
Summary
util.applyandutil.applyndecide which values nnsight is allowed to touch. They matched the builtin containers exactly:so every subclass of them fell through to
return datauntouched. A HuggingFaceModelOutputis anOrderedDictsubclass, and it is what every top-leveltransformersmodule returns — which meansmodel.outputandmodel.transformer.outputwere invisible to all five callers of these helpers.namedtuple,OrderedDict,defaultdictandtorch.return_types.*module outputs were skipped for the same reason.Nothing raised. The traversal simply found no tensors, so each caller degraded silently.
The five symptoms
Reproduced on
gpt2, CPU,dev@ efe4244:Batcher.narrowmodel.outputin a batched trace got the whole batch, and both invokes got the same object (a.logits is b.logits→True)Batcher.swapmodel.transformer.outputinside one invoke landed on every other invoke's rowshooks.cache_output_hook/cache_input_hooktracer.cache(...)stored the full batch for aModelOutputmoduleCache.addcache(dtype=...)/cache(device=...)left those tensors unconverted; the defaultdetach=Truenever fired, so the cache pinned the autograd graphnnsight_forward's__nnsight_skip__mergetracer.skip()returned one invoke's value instead of the concatenationRead leak
Write leak
Zeroing hidden states in invoke 0 only:
Cache
This is the failure mode that worries me most: a batched-invoke user reading
model.outputgets a plausible tensor with a silently wrong batch dimension, andcache(device=...)silently leaves activations on the GPU it was asked to move them off.Fix
Recurse into
list/tuple/dictsubclasses too, rebuilding the concrete type so callers still get aModelOutputback rather than a plaindict.ModelOutputare dataclasses whose__init__does not accept a mapping. Going through__setitem__also preservesModelOutput's item/attribute mirroring, so.logitsstays in sync with["logits"](asserted in the tests)._makefor namedtuples and the iterable constructor otherwise (torch.Size,torch.return_types.*structsequences), falling back to a plain tuple only when the subclass genuinely cannot be rebuilt.The exact-type fast paths are untouched, so the hot path is unchanged: the new branches only run for values that previously fell straight through to
return data.Batcher.narrowalso still early-returns before any of this whenneeds_batchingisFalse, so single-invoke traces do no extra work at all.Deliberately out of scope:
collections.UserDict/UserListandtransformers.BatchEncodingare notdict/listsubclasses (they areMutableMapping/MutableSequence), and objects likeDynamicCacheare not containers at all. Those keep today's behaviour — widening to theabctypes is a bigger semantic question (stris aSequence) and none of the symptoms above need it.Verification
Each invoke's narrowed
ModelOutputnow matches the same prompt traced alone to a relative error of 2e-7, while differing from the other invoke's reference by 1.7e-1 — so the slice is right, not merely the shape.tests/test_container_types.pyadds 33 tests: the traversal itself (apply/applynover 9 container shapes, type preservation,inplace,torch.Size, structsequences, a tuple subclass with an incompatible constructor, non-containers, strings) plus the five behaviours above. 18 of the 33 fail ondevand all 33 pass with this change.Full CI test list on CPU:
1 failed, 276 passed. The one failure istest_lm.py::TestGradients::test_backward_with_multiple_invokers, which is already red ondevandmainand is unrelated to this change (addressed separately in #693).Run with
transformers5.15.1 /torch2.9.1 on CPU. Verified this change introduces no failure difference undertransformers4.57.3 either.