fix(bridge): remove orphaned convert_weights override from nanogpt adapter - #1602
Conversation
…apter NanogptArchitectureAdapter.convert_weights ended in super().convert_weights(remote_module), but ArchitectureAdapter has had no such method since 3efbd6e ("Cleanup (TransformerLensOrg#1129)"), so every call raised AttributeError. The failure was invisible to CI because of the # type: ignore[misc] on that line; without it mypy reports '"convert_weights" undefined in superclass'. The override had no callers anywhere in the tree, and its _orig_mod. prefix strip was a no-op regardless: nn.Module.state_dict() returns a fresh dict, so the loop mutated a throwaway copy before passing the original module to super(). Removing it cannot regress behaviour, since every path through it raised. Dropping the ignore leaves the CI type-check job as the guard against reintroduction. Not re-homed into preprocess_weights: that hook runs on self.state_dict() inside process_weights, i.e. after load with TL-renamed keys, so it never observes _orig_mod.-prefixed checkpoint keys.
|
Sibling instance of the same pattern, from the same commit: The same commit also removed its two callers ( Fold the removal into #1602, separate PR, or leave it? If component-level HT-vs-bridge weight comparison is still wanted, restoring the two extractors from |
|
@sohv in response to your question, feel free to make that as a separate PR, I am going to merge this because it looks great! |
|
@jlarson4 Thank you! I will create it as a separate PR and will request merging with the |
…rs (#1604) _enable_ht_attention dispatched on three attention layouts, but two of the three branches called methods that no longer exist: _extract_linear_ht_format (split Q/K/V - Llama, Mistral, Qwen, Gemma) and _extract_qkv_neox_style (fused query_key_value - GPT-NeoX, Falcon). 3efbd6e ("Cleanup (#1129)") deleted both definitions while keeping the call sites, so only the GPT-2 branch could run; the other two raised AttributeError on entry. Four # type: ignore[attr-defined] comments kept mypy quiet about it. The same commit removed both callers (architecture_adapter.py:1249 and bridge.py:1144 as of 3efbd6e^), leaving the method unreachable - no user impact, but it reads as a finished "architecture-agnostic" helper to anyone wiring it into component testing. Its three remaining helpers are called only from it, so they go too: _extract_qkv_gpt2_style, _extract_output_proj, and _disable_hook_conversions (whose body was already `pass`). einops becomes unused and is dropped. setup_component_testing and _wire_rotary_for_testing are untouched. Follow-up to #1601/#1602, which fixed the same pattern in the nanogpt adapter; separate PR per maintainer request.
2 participants
Description
NanogptArchitectureAdapter.convert_weightsended inreturn super().convert_weights(remote_module), butArchitectureAdapterhas had noconvert_weightsmethod since3efbd6e("Cleanup (#1129)", 2025-11-15). Every call therefore raisedAttributeError: 'super' object has no attribute 'convert_weights'.The defect was invisible to CI because of the
# type: ignore[misc]on that line. With the ignore removed,uv run mypy .reports it directly:This PR deletes the orphaned override together with its ignore, and drops the now-unused
torchimport.Why removal rather than repair:
*.py/*.ipynb/*.md, plus a check for dynamicgetattrdispatch, finds only two definitions ofconvert_weightsin the tree — this one andbd3lm.py:208(a different signature returning{}, likewise never called). Both are orphans from the base-class removal._orig_mod.prefix strip was a no-op regardless.nn.Module.state_dict()returns a fresh dict, so thepop/reassign loop mutated a throwaway copy and then passed the originalremote_moduletosuper().Deliberately not re-homed into
preprocess_weights. That hook runs onself.state_dict()insideprocess_weights(transformer_bridge.py:608) — after loading, with TL-renamed keys — so it would never observe_orig_mod.-prefixed checkpoint keys. Moving the logic there would look like a fix while doing nothing.Regression guard: with the
# type: ignoregone, the CItype-checkjob fails if the override is reintroduced.Adjacent gap, not addressed here:
TransformerBridgeconsequently has no_orig_mod.handling fortorch.compile()'d nanoGPT checkpoints, while the HookedTransformer path does (transformer_lens/pretrained/weight_conversions/nanogpt.py:14). That is HT→Bridge drift, but adding it is a feature with its own design question (which load path should own the strip), so it is out of scope for a dead-code removal. Happy to file it separately.On unit coverage:
nanogptsits in_COVERAGE_EXEMPT_MODULES(tests/unit/model_bridge/supported_architectures/test_thin_subclass_adapters.py:94) as an internal-only adapter, so no test file accompanies this change. I have a structural unit suite for the adapter ready if you want it — note that adding it also requires dropping"nanogpt"from that exemption set, sincetest_coverage_exemptions_are_still_uncoveredis self-cleaning. That is a policy call on an intentionally-exempt adapter, so I did not make it unilaterally.Fixes #1601
Type of change
Checklist:
nanogptis an exempt module, and the# type: ignoreremoval makes the CItype-checkjob the regression guard. A structural unit suite is ready if you want the exemption retired.