Hook extensions for specialized bridges - #1673
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
Each adapters touched in this PR mapped a component to a bridge class that cannot express it, so hooks users expect either did not exist or resolved to something that never fired. None of it changes numerics, the swaps keep the math with HF and only restores observability.
gemma3nandgemma4attention were bareGeneralizedComponents whose entire surface ishook_in/hook_out. Nohook_pattern, nohook_attn_scores, nohook_result;hook_q/k/v/zandW_Q..b_Odid not resolve; and q/k/v fired flat[batch, seq, d_model]rather than the per-head shape every gemma1/2/3 sibling gives. Now usesAttentionBridge(maintain_native_attention=True), the same delegation 20+ adapters use, so HF still runs the attention. The optionalk/v/k_norm/v_normflags are preserved: KV-shared layers (E2B/E4B) and K==V layers legitimately lack those modules, and the alias set prunes itself accordingly.gemma3n's MLP takes the one-lineself._gated_mlp()thatgemma4received in Fix Gemma 4 MLP hook aliases #1650.SymbolicBridge–opt,xglm, and the seven that inherit BART's_mlp()(bart,marian,pegasus,blenderbot,mbart,m2m100,led). That class exposes nohook_pre/hook_post, and on OPT/XGLM the block'shook_mlp_outpointed at aHookPointthat never fired.MLPBridge(name=None)fixes both, copying BERT, which maps the containerlessfc1/fc2layout correctly. Safe because component setup keys promotion onname is Nonerather than on the bridge type, so theblocks.{i}.mlp.in/outweight paths are unchanged andMLPBridge.forwardisn't invoked.hook_mlp_inis deliberately not overridden on OPT/XGLM: they are pre-norm, so itsHookedTransformermeaning is the pre-ln2residual the block already supplies. BART overrides it legitimately because it is post-LN.proj_1and GraniteMoeHybrid'sshared_mlp.input_lineareach concatenate gate and up, so under a plainMLPBridgehook_prewas the2*d_mlppre-GLU tensor andhook_pre_lineardid not exist, the neuron basis was unreachable. Both now useJointGateUpMLPBridgewith an architecture-specific splitter, guarded byrequire_readable_weightlike Phi-3's. The split direction is pinned by value in both tests, with a negative control, because shapes cannot catch a gate/up swap. Both halves are[d_mlp, d_model].qkv_projis fused, sohook_q/hook_k/hook_vcould never resolve. They are stripped the same way raven strips itsWqkvequivalents.hook_zstays, since it resolves throughout_proj. OpenELM is correspondingly removed from_KNOWN_DEAD_ALIASES— that marker isxfail(strict=True), so leaving it would XPASS and fail the suite.MoERouterBridge(name="router", logits_index=-1). The index is load-bearing and measured: HF returns(top_k_index, top_k_weights, router_logits), so the default index 0 hooks an int64 index tensor rather than the logits.MLPBridgefor parity with RWKV-4 looked like it would be correct, butMLPBridgepre-fires the wrapped module'shook_in, which would suppress theinprojection's ownhook_inand on RWKV-7 that hook carries the post-token-shift key input. The swap would have traded a correct hook for two alias names. Instead the two aliases are declared directly on the delegated component, sohook_pre/hook_postexist with nothing moved.isinstance(mlp, SymbolicBridge),mlp.submodules["in"].name == "proj_1",shared_mlp.submodules == {"in", "out"}). They are rewritten to assert the hooks a user actually gets rather than the class.cfg.act_fn's "relu" default because the module capture probedactivation_fn/act_fn(OpenELM storesself.act) and the config conversion probed four names that don't include OpenELM'sactivation_fn_name. The probe list now also covers GraniteMoeHybrid'sself.activation.process_weightsflipped the MLP onto the functional branch, which computes plainact(gate)*upwithout HF's_gaussian_topk. The gemma3n MLP now keeps delegating under processed weights, which is correct because distribution writes the processed tensors into the wrapped Linears, and the hooks the swap wanted still fire from inside HF's forward.[batch*seq, d]– OPTDecoderLayer reshapes to 2D around ln2/fc1/fc2. A stamped unflatten conversion now presents[batch, seq, d]to hooks and reverts edits back to 2D, verified read side (shapes) and write side (a position-indexed edit lands positionally; the test fake mirrors modeling_opt's 2D residual so a broken revert crashes it as it would real OPT). Inert when no hooks are attached.W_K/W_Vwould silently mis-factorize on minority layers; a width check now raises naming the per-layer geometry. Zero false positives across the adapter suite.MLPBridge(name=None)direct calls raise – component setup binds the parent decoder layer as the containerless MLP's original_component, soblock.mlp(x)silently executed the whole layer (attention included) with a plausible output shape; the old SymbolicBridge raised here, and now MLPBridge does too.maintain_native_attention,hook_patternandhook_attn_scoresboth fire HF's post-softmax weights and writes to them don't reach the output. The shared test fake now uses real o_proj widths sohook_z's per-head shape is actually asserted; class-constant alias restatements narrowed to isinstance + the repo-wide resolution suite.ffn_with_glu=False(unreachable on published checkpoints) is refused at boot instead of crashing in a mat-mul at first forward.fused_attr=,skip_initso boot no longer burns kaiming inits or RNG draws) replaces the two new per-adapter copies;fused_qkv=Trueon AttentionBridge replaces the raven/OpenELM hand-rolled alias strips;_build_moe_bridge()on the granite base replaces the duplicated router mapping.OpenELM re-verification surfaced three more pre-existing breaks, each fixed rather than skipped:
__init__with dataclass machinery that delivers every non-base field via__post_init__(**extras); apple's frozen 4.x remote code defines it argless, soAutoConfig.from_pretraineditself raised. Newautoconfig_with_remote_post_init_compat(used by boot and verify_models' estimate) retries once after wrapping the remote class's__post_init__to run the base handler first — base-first is load-bearing, since the class's own fields only get set by the base handler under 5.x. Generic: fires only on that exact TypeError, for any 4.x-era remote-code repo.DynamicCache.from_legacy_cache/.to_legacy_cache, both removed in 5.x.prepare_loadingnow swaps the module-level name for a subclass restoring them (5.x'sddp_cache_datainit is the legacy tuple format).num_query_headsper layer, so the new width guard (correctly) refuses the cfg-scalar factorization. The centering statistic is head-agnostic, so the benchmark falls back to the raw 2D projection; the probe also had to stop usinghasattr(attn, "W_O"), which invokes the property and only swallowsAttributeError.All seven
apple/OpenELM-*checkpoints (270M/270M-I/450M-I/1.1B/1.1B-I/3B/3B-I) verified with the SiLU fix in place, replacing the invalidated February entries.Type of change
Checklist: