Repository navigation
fix(models): Drop the position-indexed token type lookup in RoPE encoders - #48407
Conversation
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
Super nice catch, just a small nit on the test design
| config_and_inputs = self.model_tester.prepare_config_and_inputs() | ||
| self.model_tester.create_and_check_for_token_classification(*config_and_inputs) | ||
|
|
||
| def test_forward_beyond_max_position_embeddings(self): |
There was a problem hiding this comment.
Can we do the same structure as test_for_token_classification? You can also take a look at mamba which has a few regression tests atp.
Other than that, let's add a small comment/docstring to reference this PR and the reason behind this test
There was a problem hiding this comment.
Done, thanks a lot :)
|
[For maintainers] Suggested jobs to run (before merge) run-slow: jina_embeddings_v3, nomic_bert |
CI recapDashboard: View test results in Grafana |
|
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. |
…ders (huggingface#48407) * fix: Drop the position-indexed token type lookup in RoPE encoders * fix: Changes
What does this PR do?
→
nomic_bertandjina_embeddings_v3are the only two RoPE encoders that carry BERT's buffered token_type_ids gather. The buffer is all zeros, so the gather is a no-op, but indexing it withposition_idsstill constrains those IDs tomax_position_embeddings, which RoPE has no reason to obey. Both original implementations build the zeros directly (nomic, jina) and nomic sets max_position_embeddings = 0 whenever rotary is on, so the cap tmk isn't upstream behaviour either.→ In practice this caps
nomic-embed-text-v1.5at 2048 tokens onmain(RuntimeError: index 2048 is out of bounds for dimension 1 with size 2048) despiten_positions: 8192; it reaches 8192 here. Un-skipstest_model_rope_scaling_from_configfor nomic, which passes now :)cc: @vasqu
Before submitting