Repository navigation
fix(bridge): rotate Cohere and Cohere2 with their own RoPE convention - #1873
Merged
jlarson4 merged 1 commit intoOct 9, 2026
Merged
Conversation
CohereRotaryEmbedding returns cos/sin with each frequency repeated twice and Cohere rotates adjacent element pairs, but the attention bridge used the Llama formula, so logits did not match Hugging Face (hyper-accel/tiny-random-cohere: max diff 1.1e-2 and a different argmax before, 7.8e-8 after). Add a rotary_interleaved_cos_sin config flag, set by the Cohere adapter and inherited by Cohere2, and a helper that rotates adjacent pairs with cos/sin as given.
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
CohereForCausalLMandCohere2ForCausalLMdo not match Hugging Face through the bridge, because the bridge rotates Q and K with the wrong RoPE convention. Cohere'sCohereRotaryEmbeddingreturns cos/sin with every frequency repeated twice (torch.repeat_interleave(freqs, 2, dim=-1), so[c0, c0, c1, c1, ...]), and Cohere'sapply_rotary_pos_embuses arotate_halfthat pairs adjacent elements.PositionEmbeddingsAttentionBridgeused the Llama formula for Cohere. The adapter comment says the rotation is "delegated to HF", but the generic attention bridge reconstructs it.rotary_adjacent_pairs(GLM/ERNIE) does not fix it either: those models get the usual half-duplicated cos/sin and the helper interleaves them itself, so with Cohere's already interleaved cos/sin it would interleave twice. I tried that first and it left the attention output wrong.So this adds a separate config flag,
rotary_interleaved_cos_sin(defaultFalse, declared next torotary_adjacent_pairs), and a helper_apply_rotary_pos_emb_interleaved_cos_sinthat rotates adjacent pairs with cos/sin as given. The Cohere adapter sets the flag, andCohere2ArchitectureAdapterinherits it. Other architectures are unchanged.I found it with a parity battery over the supported causal-LM architectures (a tiny random-weight Hugging Face model per architecture wrapped with
build_bridge_from_module, bridge against HF logits). For Cohere the q/k/v projections, cos and sin matched and the rotated q did not, so the rotation is the only difference. The project's own record agrees:supported_models.jsonhashyper-accel/tiny-random-coherebelow threshold withforward_pass_logitsfailing atmax_diff=0.009886.Bridge against an untouched copy of the Hugging Face model (fp32, CPU):
devhyper-accel/tiny-random-cohere, real checkpointtrl-internal-testing/tiny-CohereForCausalLM, real checkpointCohereForCausalLM, wide weightsCohere2ForCausalLM, wide weightsWith the fix, greedy generation of the tiny random Cohere model, with and without the KV cache, is token for token the same as the untouched Hugging Face model. I did not run
verify_modelsand I could not try the gated Cohere checkpoints.One thing I learned on the way, that you may know already:
build_bridge_from_modulereplaces the submodules of the model it is given (itsstate_dictkeys get_original_component), so calling the Hugging Face model afterwards runs the bridge's code. A reference has to be computed before bridging, or on adeepcopy, which is what the new tests do.Fixes # (no issue, found by the parity check above)
Type of change
Tests
In
test_cohere_adapter.py: the adapter setsrotary_interleaved_cos_sin, the default isFalse(on a fresh config, the class-scoped fixture is mutated by the adapter), andTestCohereParitybuilds a tiny randomCohereForCausalLMand compares the bridge with an untouched copy usingassert_tiny_parity.test_cohere2_adapter.pyhas the same forCohere2ForCausalLM. Four of the new tests fail ondev(the two config tests and both parity tests), and all pass with this change. The two files pass, 83 tests.All of
tests/unit(Windows, Python 3.12, torch 2.14.1 CPU, transformers 5.19.0, withtimmandtorchvisioninstalled): 7273 passed, 3 failed. The same 3 fail on unmodifieddev:test_model_structure_doc.py::test_every_qualified_hook_name_across_docs_exists(aUnicodeDecodeErrorreading a file with the Windows default codec) and two intest_sparse_probing.py(test_default_tolerance_accepts_large_scale_activationsandtest_stop_reason_distinguishes_converged_from_capped_fits), which I did not look into.black,isort,pyclnandmypyare clean on the changed files.Checklist:
dev, see above)