fix(bridge): respect use_parallel_residual in the GPTNeoX adapter - #1649
fix(bridge): respect use_parallel_residual in the GPTNeoX adapter#1649Chinmayrawat15 wants to merge 3 commits into
Conversation
The NeoX adapter hardcoded the parallel-residual wiring, ignoring HF's use_parallel_residual. On the sequential branch HF computes a genuine post-attention residual that post_attention_layernorm reads, but ParallelBlockBridge pops the hook_resid_mid alias by design -- so the hook was silently missing on exactly the checkpoints that have one, and cfg.parallel_attn_mlp reported True for a model that is not parallel. Logits stayed correct because the bridge delegates to HF's own block, so nothing raised. All six RedPajama-INCITE checkpoints in the registry are GPTNeoXForCausalLM with use_parallel_residual=false, so resid-mid patching, attribution and SAE work were unavailable on registered models. Pythia is genuinely parallel and is unaffected. Select the block class from the flag, mirroring the guards already in stablelm.py:121 and falcon.py:148, and add use_parallel_residual to _HF_PASSTHROUGH_ATTRS so the adapter can see it -- reading the resolved cfg.parallel_attn_mlp instead would flip hand-built configs to sequential, since it defaults to False while HF's NeoX default is True. Mirror the same hardcode on the HookedTransformer side. Add test_parallel_residual_identities.py asserting the identity ParallelBlockBridge documents in its own docstring (resid_post == resid_pre + attn_out + mlp_out) across all seven adapters that use it, exercising the three config-switchable families in both wirings. Fixtures are seeded tinies from local HF configs, so no hub access is needed. Fixes TransformerLensOrg#1644 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jlarson4
left a comment
There was a problem hiding this comment.
Thanks for tackling this @Chinmayrawat15, just a couple small change requests. Let me know if you have any questions!
| # GPTNeoX ships both parallel (Pythia, HF's default) and sequential | ||
| # variants. Hardcoding parallel drops hook_resid_mid on sequential | ||
| # checkpoints that genuinely have a post-attention residual. | ||
| use_parallel_residual = getattr(cfg, "use_parallel_residual", True) |
There was a problem hiding this comment.
TransformerBridgeConfig has no use_parallel_residual field, so on the build_bridge_from_module(..., tl_config=...) path this fallback fires and a config with parallel_attn_mlp=False still gets ParallelBlockBridge with no hook_resid_mid. Can you make the fallback getattr(cfg, "use_parallel_residual", getattr(cfg, "parallel_attn_mlp", True)) and add a unit case asserting parallel_attn_mlp=False leads to a BlockBridge being created? A plain parallel_attn_mlp read won't work due to its constructor default being False rather than HF's True, as your PR description notes.
| if key not in _CACHE: | ||
| config = config_cls(**config_kwargs) | ||
| config._attn_implementation = "eager" | ||
| torch.manual_seed(42) |
There was a problem hiding this comment.
manual_seed(42)/manual_seed(0) runs only when cache misses and leaks RNG state to later unseeded tests on the same worker. boot_native wraps its seeding in torch.random.fork_rng(devices=[]) for exactly this reason (bridge.py:360), the same two-line wrapper works here.
Review feedback from @jlarson4 on TransformerLensOrg#1649. TransformerBridgeConfig has no use_parallel_residual field, so on the build_bridge_from_module(..., tl_config=...) path the getattr default fired and a config with parallel_attn_mlp=False still got ParallelBlockBridge with no hook_resid_mid. Fall back to parallel_attn_mlp before defaulting to HF's True. The existing _make_cfg fixture relied on the old hardcode, so it now states parallel_attn_mlp=True explicitly -- a caller-supplied NeoX config is otherwise indistinguishable from one that asked for sequential, since the dataclass default is False. Wrap the tiny-model seeding in test_parallel_residual_identities.py in torch.random.fork_rng(devices=[]), matching bridge.py:360. Seeding ran only on a cache miss, so it leaked RNG state into later unseeded tests depending on ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fallback added for caller-supplied configs made the _HF_PASSTHROUGH_ATTRS entry redundant -- reverting it left every test green, since sources/transformers.py already derives parallel_attn_mlp and the fallback picks it up. That only holds while TransformerBridgeConfig keeps defaulting parallel_attn_mlp to False, so assert the adapter sees HF's own flag rather than resting on two defaults agreeing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @jlarson4 — both were real, fixed in a4e0ba6 and pinned in e06b394. Fallback on the That does change behaviour for hand-built configs: previously any Added both requested cases — RNG leak. Good catch, wrapped in One thing that fell out of verifying this. Re-checking the guards against Guard checked against |
Description
Fixes #1644.
The GPTNeoX adapter hardcoded the parallel-residual wiring —
neox.pysetcfg.parallel_attn_mlp = Trueand builtParallelBlockBridgeunconditionally, ignoring HF'suse_parallel_residual. GPTNeoX ships both wirings, and on the sequential branch HF computes a genuine post-attention residual thatpost_attention_layernormreads:ParallelBlockBridgepops thehook_resid_midalias by design, so that hook was silently missing on exactly the checkpoints that have one, andcfg.parallel_attn_mlpreportedTruefor a model that is not parallel. Logits stayed correct (the bridge delegates to HF's own block), so nothing raised — which is why it went unnoticed.This is not hypothetical. All six RedPajama-INCITE checkpoints in
supported_models.jsonareGPTNeoXForCausalLMwithuse_parallel_residual: false, so resid-mid patching, attribution and SAE work were silently unavailable on registered models:use_parallel_residualtogethercomputer/RedPajama-INCITE-7B-BaseFalsetogethercomputer/RedPajama-INCITE-7B-ChatFalsetogethercomputer/RedPajama-INCITE-7B-InstructFalsetogethercomputer/RedPajama-INCITE-Base-3B-v1Falsetogethercomputer/RedPajama-INCITE-Chat-3B-v1Falsetogethercomputer/RedPajama-INCITE-Instruct-3B-v1FalsePythia is genuinely parallel and is unaffected.
The fix
StableLmArchitectureAdapterandFalconArchitectureAdapteralready guard this exact case, so the change mirrors them rather than inventing a pattern:neox.py—block_cls = ParallelBlockBridge if use_parallel_residual else BlockBridge, matchingstablelm.py:121/falcon.py:148, andcfg.parallel_attn_mlpnow follows the flag instead of being overwritten._bridge_builder.py— addeduse_parallel_residualto_HF_PASSTHROUGH_ATTRS, which is how Falcon'sparallel_attnalready reaches its adapter.loading_from_pretrained.py— same hardcode existed on the HookedTransformer side; mirrored per AGENTS.md §2.Reading the already-resolved
cfg.parallel_attn_mlpinstead looked simpler but is wrong:TransformerBridgeConfig.parallel_attn_mlpdefaults toFalsewhile HF's GPTNeoX default isTrue, so a hand-built config (boot_native, and the existingtest_neox_adapter.py) would silently flip to sequential. Using the HF field name with HF's default keeps the parallel path byte-identical — I confirmed the existing NeoX adapter unit tests pass unchanged.Test coverage
ParallelBlockBridgedocuments its identity in its own docstring (block.py:407,output = resid_pre + attn_out + mlp_out), but nothing asserted it for any of the seven adapters that use it — the same class of gap as #1639, and what let this wiring bug stay invisible.New
tests/integration/model_bridge/test_parallel_residual_identities.pycovers all seven (gptj,codegen,phi,cohere,neox,stablelm,falcon) and exercises the three config-switchable families in both wirings. Fixtures are seeded tinies built from local HF configs, so there is no hub access and both wirings are reachable from one checkpoint shape — 20 tests in ~8s.It is a real regression guard, not a rubber stamp: reverting the
neox.py+_bridge_builder.pychange fails exactlytest_sequential_variant_exposes_resid_mid[neox]andtest_sequential_variant_decomposition[neox], with the other 18 still passing. Reverting theloading_from_pretrained.pychange fails the newconvert_hf_model_configcase.The new file is a sibling to
test_residual_decomposition_identities.py(which explicitly scoped these architectures out); I updated that docstring to cross-reference rather than moving anyone's tests.Type of change
Validation
make test-prclean on macOS arm64 / Python 3.12 / transformers 5.14.1:make unit-testmake docstring-testmake acceptance-testmake integration-testuv run pytest tests/integration/model_bridge/test_parallel_residual_identities.py— 20 passed in 8smake check-formatclean;uv run mypy .— Success, no issues in 385 source filesNoted but not included
transformer_lens/benchmarks/hook_registration.pystill carries a BLOOM carve-out (lines ~246-255 and ~592-599) whose comment — "hook_attn_out and hook_mlp_out capture attn+residual instead of just attn" — was made false by #1640/#1642. It is unreachable today (both call sites inmain_benchmark.py:401-402pass noreference_activations, so the functions return before it), so it is dead code with a misleading comment rather than a live suppression. Left out to keep this PR to one story — happy to send it separately.Checklist: