Fix remote execution for encoder models - #700
elliottower wants to merge 2 commits into
Conversation
|
Confirmed and fixed, by a different route than your patch, so I would like your read on it before this closes. Your diagnosis holds on Your fix makes that class picklable: cache it per What I went after instead is the class itself. It exists only to override On a BERT attention block: attention.output # the block's forward output, as on every other module
attention.E_output # the BertSelfOutput submoduleThis is a breaking change and the one thing I am least sure of. Landed as 518d03f on Two things I would value your view on, since you hit this in real use rather than in a test:
One thing that was our fault: this was opened against Leaving this open until you have had a look. |
BREAKING CHANGE: on a module whose own child is named `output` or `input`
(every BERT-style encoder has one), the child is now reached as `.E_output`
and `.output` keeps the meaning it has on every other module. The child used
to take the name and nnsight's property moved to `.nns_output`, which is gone.
The old scheme overrode the descriptor on a subclass synthesized for that one
envoy. The class belonged to no module, so cloudpickle could not pickle it by
reference, fell back to pickling it by value, and choked on the eproperty in
its class dict. No encoder model could be traced remotely:
TypeError: cannot pickle 'eproperty' object
Renaming the envoy needs no subclass, so the envoy stays a plain `Envoy` and
serializes. The child keeps its own path, so `.path` and `named_modules()` are
unchanged. Attribute access is the only spelling that moved, and `get()` and
`rename` go through it, so both are written against `E_output`; a `rename`
against the plain name reaches the served value and raises at construction.
The mount is recorded in `_aliases` the way `rename` records its own, so the
repr labels the child `E_output/output` with no special case and the alias
displacement check stops a user alias claiming the name.
The collision test narrowed from `hasattr(Envoy, name)` to
`inspect.isdatadescriptor`, so a child named after a plain method (`to`,
`save`) mounts under its own name as before. Only the seven descriptors move a
child.
Reported and originally fixed by @elliottower in #700, which took the other
route: keep the synthesized class, cache it, and bind it into its base's
module so it pickles by reference.
Co-authored-by: Elliot Tower <elliot@elliottower.ai>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Hi all, greatly appreciate all the hard work that goes into NDIF. I am working on a mechinterp project on RNA and DNA foundation models and was looking into whether it was possible to run them here, and it looks like with these two bug fixes it will be. It should also help anyone else who wants to run encoder models in the future. Thanks.
1. Envoy classes for modules that define
.outputcan't be pickled_handle_overloaded_mountbuilds a class per Envoy instance named<cls>.Preserved. The dot makes the name unresolvable and the class is never bound in a module, so pickle can't serialize it:Remotely this shows up as
RemoteException: name 'hooked_output' is not defined. It hits any model with aninputoroutputsubmodule, so all of BERT and ESM (BertLayer.outputis aBertOutput). Decoder-only models never take this path, which is why they work today and encoders don't.(base class, mount point)and bound into the module that defines the base__dict__2.
automodelisn't sent to the serverLanguageModel._remoteable_model_keyserializes onlyrepo_idandrevision, so the server rebuilds every model asAutoModelForCausalLM:automodelis now in the key_remoteable_from_model_keyalready merges the key JSON into constructor kwargs, andTransformersMixinresolves a string automodel, so no server change is neededTests
tests/test_envoy_overloaded_mount.py, tiny models, no cluster needed. 3 fail on main, all pass here. The two remote tests skip withoutNDIF_KEY.Reproduced and fixed on two independent stacks:
Same failure and same fix on both, spanning transformers 4.x and 5.x and torch either side of the
>=2.4.0floor. On Linux I ran this branch against main in the same container:tests/test_tiny.pypasses on both