Pin layer_types so the tiny Cohere2 model covers both attention types - #6963
Open
albertvillanova wants to merge 1 commit into
Open
Pin layer_types so the tiny Cohere2 model covers both attention types#6963albertvillanova wants to merge 1 commit into
albertvillanova wants to merge 1 commit into
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
This PR pins
layer_typesin the tiny Cohere2 generation script so that the model has one layer of each attention type.Related to:
Motivation
The tiny model has 2 layers, and Cohere2's period of 4 makes the derived pattern yield two sliding layers and no full-attention layer.
That matters more than it does for the other hybrid models, because Cohere2 applies RoPE only on sliding layers. From the transformers docs:
So the tiny model never exercised the NoPE global-attention path, which is the defining feature of the Command R7B architecture. Traced on the current model,
apply_rotary_pos_embruns on 2 of 2 layers; with the pinned pattern it runs on 1 of 2.Unlike #6962, this emits no log line, so it is purely a test-coverage gap.
Solution
Pin
layer_typesexplicitly, as the Gemma4 and DiffusionGemma scripts already do, and as the Nemotron 3 scripts do for block types ("one of each block type"). The order is local first, matching the reference'sorder_of_interleaved_layers="local_attn_first".With 2 layers the reference 3:1 ratio cannot be represented at all, so the choice is not between a faithful and an unfaithful pattern, but between covering one attention type and covering both.
The Hub model has been regenerated in this PR:
Verified in this CI run (https://github.com/huggingface/trl/actions/runs/33179106731) with the revision pinned through
MODEL_REVISIONS:transformers4.56.2 floor, with the test totals unchanged (2369 passed, 149 skipped, 7 xfailed).This was the last tiny model with an uncovered layer type. Sweeping all of them, only Gemma3, Olmo3 (both handled in #6962) and Cohere2 were missing one; the other 28 already cover every type their architecture can produce.
Changes
layer_typesto["sliding_attention", "full_attention"]in the tiny Cohere2 generation scriptNote
Low Risk
Test-only tiny-model generation script change with no runtime library impact until the Hub artifact is updated.
Overview
The tiny Cohere2 causal-LM generator now explicitly sets
layer_typesto["sliding_attention", "full_attention"]instead of letting the config derive the pattern from two layers—which previously produced only sliding layers and never ran the full-attention / NoPE path (RoPE on sliding only).This aligns with other tiny scripts (e.g. Gemma4, DiffusionGemma) so CI and smoke tests exercise both attention types; the Hub tiny model must be regenerated separately for tests to pick up the change.
Reviewed by Cursor Bugbot for commit 8e52213. Bugbot is set up for automated code reviews on this repo. Configure here.