source: rebindable controller object, the module's own forward as its body, and .source on callable instances - #723
Merged
Merged
Conversation
…its body
`install_controller` wrote a closure into `module.__dict__["forward"]`, which
made three things silently wrong. `copy.deepcopy` treats a function as atomic,
so a copy's forward still dereferenced the original module and computed with
its weights. The weakly-held interleavers in `State.routes` meant a wrapped
module could never be pickled again ("cannot pickle 'weakref.ReferenceType'").
And the body came from `type(module).forward`, so a forward living in the
instance slot -- picked in `__init__`, monkeypatched, `torch.compile`'s -- was
destroyed by the assignment, in and out of traces.
The controller is now a `Controller` object with `__deepcopy__` (rebinding via
deepcopy's memo) and `__reduce__`, `State.__getstate__` drops the routes and the
run-time-built instrumented body, and `module_body` takes the body from the
instance slot when there is one. accelerate's device-alignment wrapper and
transformers' TP wrapper keep coming from the class: nnsight puts those back
around the controller itself, and running one as the body would apply it twice.
Replacing `forward` after wrapping now warns rather than silently disabling the
module's handoffs.
Also: `.source` reaches a callable instance through its `__call__`, which is
where its Python source is -- an attention processor is a plain object, so
`get_attention_scores` was unreachable on every diffusers UNet.
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.
Two findings from the stress sweep, both in
src/nnsight/intervention/source.py.1. The controller closure written into
module.__dict__["forward"](item 4)install_controllerdidstate = State(type(module).forward)andmodule.__dict__["forward"] = make_controller(module, state). Three measured symptoms:copy.deepcopyof a wrapped module computed with the original's weights.copytreatsa function as atomic, so the copy's
forwardwas the original's closure, dereferencing theoriginal module. Setting the copy's weights to 9.0 still returned the original's answer.
cannot pickle 'weakref.ReferenceType' object, from the weakly-held interleavers inState.routes(not from the closure).forwardwas destroyed —self.forward = self._fastpicked in__init__, a monkeypatched layer,torch.compile'sOptimizedModule. A module returning300 before wrapping returned 3 after, or raised
NotImplementedError, permanently, outsidetraces too.
What changed, as decided:
make_controller→ aControllerobject holding the module weakref and theState.Controller.__deepcopy__looks the copied module up indeepcopy's memo (the copy isregistered there before its
__dict__is copied) and binds to it;__reduce__pickles itas
(module, state).State.__getstate__drops the routes — unpicklable, and a copy belongs to whatever wraps itnext — and the instrumented body with its
Compiled, which are built at run time (afunction pickle can't name, a code object it can't write at all). A new
originalslotkeeps the pre-instrumentation body so a copy re-instruments from it on demand.
module_bodytakes the body from the instanceforwardwhen there is one, wrapping analready-bound one so it takes and ignores the module the controller passes (a method of the
module keeps its
__func__, which avoids pinning the module).install_sourcenowinstruments that body rather than the class's.
forwardafter wrapping warns on the nextinstall_controller.Two wrappers are deliberately not treated as bodies: accelerate's device-alignment
wrapper (
run_bodyre-applies it from_hf_hook) and transformers' tensor-parallel wrapper(
_keep_tp_forwardrebuilds it around the controller). Both sit in the same instance slot,and taking either as the body would apply its transforms twice — the TP one would have broken
tests/tpoutright._framework_forwardis the guard, and it also suppresses the new warningfor both, since nnsight is what put them back. That means source.py knows one string about
transformers' TP wrapper (
.install_forward.<locals>., the same testfragments.pymakes);the alternative was a change in
fragments.py, which isn't mine..sourceon a module whose forward is an instance-level plain function now raisesSourceNotAvailablerather than instrumenting the class's forward and installing it as thebody — i.e. it says it can't rather than quietly running something else.
2.
.sourcerefused callable instances (item 20)instrument()handled functions and bound methods; a callable instance has no__code__,so
compiled()raisedSourceNotAvailable("callable has no Python source (builtin or C function)")— untrue, sincetype(fn).__call__is ordinary Python.instrument()now usesfn.__call__in that case (a bound method, which the existing receiver path already handled —the same thing an agent did by hand with
set_attn_processor(AttnProcessor().__call__)), andthe message names the real cases.
get_attention_scores, and so attention probability maps,are reachable on diffusers UNets again.
Testing
Run with
/home/localjadenfk/wd/nnsight-stress/env(transformers 5.16.1, torch 2.14, Python3.12) and
PYTHONPATHat this worktree.tests/test_source.py: 69 passed. Ten new tests inTestInstall/TestRecursive—deepcopy independence and tracing a copy, a pickle round trip (including after
.source,and re-sourcing the restored module), a patched instance
forwardand aself.forward = self._fastone, atorch.compiled module, the replaced-forward warning, an accelerateModelHookfiring exactly once through the controller, and drilling into a callableinstance.
tests/ --ignore=tests/vllm --ignore=tests/tp,CUDA_VISIBLE_DEVICES=""):997 passed, 13 errors — the same 13, identically, on a pristine
git archiveof HEAD. Theyare
HF_HUB_OFFLINE=1artifacts ("You cannot infer task automatically withinpipelinewhenusing offline mode"), not this change.
tests/tp+tests/test_tensor_parallel_rules.pywith the GPU visible: 111 passed, 69skipped, same as baseline. This is what covers the TP-wrapper exclusion.
module-zoo-custom-models/skills/repro_2.py,repro_3.py,noskills/repro_5_instance_forward_clobbered.py) anddiffusion-pipelines/skills/repro_2.py(tiny SD on CPU, prints the processor's__call__and returns
attn_get_attention_scores_0.output,[16, 1024, 77]). The repros' stale.save()idiom was adapted; nothing else.tests/performance/interleave_bench.pyagainst baseline: every row within run-to-run noise(two baseline runs differ by 6–12% on this shared box), so the object call costs nothing
measurable on the pass-through path.
Not tested here: vLLM — not installed in this environment. The change is runtime-agnostic
(the vLLM runtime uses the same controller), but a vLLM module carrying an instance-level
forward would now have it as the body rather than losing it, and nothing here exercises that.
Deliberately left alone
deepcopy(model)on theNNsightwrapper (rather than the raw module) still gives a modelwhose traces raise
OutOfOrderError. That lives in the envoy tree, not this file; droppingstale routes in
State.__getstate__may help it, but I did not verify or claim it.nn.DataParallel._replicate_for_data_parallelcopies__dict__shallowly, so a replicastill shares the original's controller. Same for
copy.copy. Onlydeepcopywas decided.docs/usage/source.md's "When.sourceisn't available" list, per the decision note: itnever claimed callable instances were excluded.
🤖 Generated with Claude Code