Bug/numeric parity fixes - #1659
Merged
Merged
Conversation
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.
Description
MptMLPadds the residual inside its forward, soblocks.N.hook_mlp_outexposedresid_postinstead of the additive contribution. This is a reoccurrence of the [Bug Report] BLOOMTransformerBridgemapshook_attn_out/hook_mlp_outto residual-added states, breaking HookedTransformer residual identities #1639 BLOOM issue in another arch. The alias now targets the down-projection output (same pattern as the BLOOM Fix BLOOM residual branch hook semantics #1640 fix), and MPT joins the residual-decomposition-identity integration fixture.MLPBridgehook-fire bugs, symmetric on both edges:forwardunconditionally re-firedout.hook_outwith the module's final output (stamping the residual-added state over the projection output on residual-inside MLPs, double-applying interventions on ordinary container MLPs), and pre-firedin.hook_inon a tensor the replacedinprojection then hooked again inside the wrapped forward. Both are now single-fire: the out-side re-fire is a fallback gated on whether the projection actually ran (fired flag onLinearBridge), and the in-side pre-fire suppresses the inner duplicate for one call.GatedMLPBridge's processed-weights functional path had the mirror-image gap — it bypasses the wrapped projections and never firedout.hook_outat all; it now fires it with the down-projection output. The fired/suppress flags are a declared class-level protocol onGeneralizedComponent, implemented identically byLinearBridgeandConv1DBridge(GPT-2-family Conv1D projections included), withobject.__setattr__writes to stay off thenn.Modulesetattr path.MLAAttentionBridgehardcodedqk_head_dim^-0.5, dropping the yarnmscale²factor HF folds intoself.scaling— DeepSeek-V3/R1 and GLM4-MoE-Lite configs score ~1.87× too small ([Bug Report]TransformerBridgeactivations on gpt-oss-20b disagree with the HF model it wraps #1618-class silent divergence). The bridge now scores with the wrapped module'sscaling, falling back tosoftmax_scalefor trust_remote_code DeepSeek-V2 modules that use that name for the same quantity.MPTALiBiAttentionBridgehardcoded the same base scale, ignoring the module's resolvedsoftmax_scale, and gatedclip_qkvonis not Nonewhere HF gates on truthiness — a checkpoint spelling "disabled" as0.0clamped Q/K/V to zeros. Both now match HF; the Q/K/V clamp itself is extracted to a sharedclamp_qkvutility used by both bridges, and the reconstructed path refuses (loudly) the untested clip-plus-post-reshape-norm combination no arch currently has.key_multiplierinside its attention forward; the reconstructed path dropped it.PositionEmbeddingsAttentionBridgenow applies it post-projection/pre-RoPE, matching HF order.clip_qkvwas never applied under hooks: the reconstruction had no clamp, and the old OLMoprepare_modelpatch additionally disabled the native module's clamp — so nothing clamped anywhere. The reconstruction now clamps Q/K/V out-of-place, after the pre-reshape qk-norm (OLMoE norms first, then clamps) and before RoPE; the patch is deleted from both adapters.HookedTransformerConfig.clip_qkvfield, plumbed from the HF config in the OLMo-1B/7B and OLMoE branches, applied inabstract_attentionbetween qk-norm and rotary/cache-append. OLMo checkpoints are attention-bias-free, so weight processing does not shift the clamp threshold.OlmoAttention,OlmoeAttention(pins the norm-before-clamp order),FalconH1Attention, andDeepseekV3Attentionmodules — all eager with an explicit 4D causal mask, so parity is tight enough (~1e-5) that each restored term breaks it when reverted; a backward test that registers real full backward hooks on the projections, which an in-placeclamp_fails (the property the deleted patch protected); an HT test proving the clamp sits between projection and scores; and MLP-bridge hook-semantics tests assertingin.hook_in/out.hook_outfire exactly once with the projection's own tensor, that additive interventions apply exactly once, that residual-inside MLPs keep the projection output onout.hook_out, and that the gated processed path fires it. Each parity test also carries a negative control proving the guarded term is active. All test setup runs undertorch.random.fork_rng, leaving global RNG state untouched.Type of change
Checklist: