Repository navigation
🚨 Vision (2d/3d) rotary embeddings - #48105
Merged
Merged
Conversation
vasqu
approved these changes
Sep 3, 2026
vasqu
left a comment
Collaborator
There was a problem hiding this comment.
Smaller comments overall, should be good to merge after that 🫡 huge work and dont forget to share internally as well, this is significant and avoids all the headache around weird rope applications
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: cohere_compass, cosmos3_edge, cosmos3_omni, dinov3_vit, edgetam_video, eomt_dinov3, ernie4_5_vl_moe, exaone4_5, gemma4, glm46v, glm4v, glm4v_moe, glm5_next, glm_image, glm_ocr, idefics |
Contributor
CI recapDashboard: View test results in Grafana |
pawel-olejniczak
added a commit
to pawel-olejniczak/vllm-gaudi
that referenced
this pull request
Sep 9, 2026
…er HPU Pixtral lazily Root cause: transformers 5.17.0 removed PixtralRotaryEmbedding and position_ids_in_meshgrid, so the eager Pixtral import in register_model() aborted plugin registration for every architecture. Upstream: huggingface/transformers#48105 Fix: drop the redundant eager import and keep only the lazy "module:class" registration. Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com>
pawel-olejniczak
added a commit
to pawel-olejniczak/vllm-gaudi
that referenced
this pull request
Sep 10, 2026
…ansformers below 5.17 Root cause: transformers 5.17.0 removed position_ids_in_meshgrid and renamed PixtralRotaryEmbedding to PixtralVisionRotaryEmbedding, both imported at module level by upstream vLLM's vllm/model_executor/models/pixtral.py, so PixtralForConditionalGeneration can no longer be inspected. Upstream: huggingface/transformers#48105 Fix: cap transformers<5.17, the version upstream vLLM tests against, until vLLM adopts the new vision rotary-embedding API. Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com>
iboiko-habana
pushed a commit
to vllm-project/vllm-gaudi
that referenced
this pull request
Sep 11, 2026
…emoved actorder/g_idx, populate region_num_blocks in (+3 more) (#1788) This PR consolidates 5 hourly-CI fixes against vllm@`d2906091bfc579cebefe3d8e8fb9077397ce9882`. ## Bug 1: drop removed actorder/g_idx kwargs from HPU WNA16 scheme - **State machine id**: compressed_tensors_wna16_actorder_no_longer_supported - **Commit**: 9b4b363 ### Root cause vllm#54809 removed GPTQ group/dynamic activation ordering support, deleting the actorder ctor kwarg, has_g_idx attribute, and weight_g_idx/w_gidx_param_name plumbing from CompressedTensorsWNA16 and MPLinearKernel. HPUCompressedTensorsWNA16 in vllm_gaudi/ops/hpu_compressed_tensors.py still passed/read those removed names, so every WNA16 checkpoint load hit TypeError before reaching the kernel. ### Culprit Regression introduced by [PR #54809](vllm-project/vllm#54809). ### Fix drop actorder/has_g_idx/w_gidx_param_name from the HPU WNA16 scheme and kernel, matching upstream's removal; retire the compressed_w4a16 MoE g_idx e2e job since its checkpoint declares actorder=group, which vLLM now rejects on every backend before hardware dispatch. ## Bug 2: populate region_num_blocks in HPU NIXL KV-cache registration - **State machine id**: nixl_pd_build_fa_local_region_num_blocks_indexerror - **Commit**: 0f8b433 ### Root cause vllm#53780 added a per-region region_num_blocks list to NixlBaseConnectorWorker and made _build_fa_local index it (self.region_num_blocks[i]). The HPU register_kv_caches override (hpu_nixl_connector.py) never populated that list, so the first index access raised IndexError on every NIXL PD job. ### Culprit Probable culprit: [PR #53780](vllm-project/vllm#53780) — pinned by symbol archaeology, bisect not run. Candidate range: [compare](vllm-project/vllm@cd64c2d...d290609). ### Fix append the per-region block count in the same loop that already populates block_len_per_layer/block_stride_per_layer, extend the region-count consistency assert, and mirror upstream's Mamba/hybrid-SSM override of region_num_blocks to the physical block count. ## Bug 3: register HPU Pixtral lazily - **State machine id**: opt_causal_lm_modelconfig_inspect_validation_error - **Commit**: c41afcd ### Root cause transformers 5.17.0 removed PixtralRotaryEmbedding and position_ids_in_meshgrid, so the eager Pixtral import in register_model() aborted plugin registration for every architecture. ### Culprit Regression introduced by [PR #48105](huggingface/transformers#48105). ### Fix drop the redundant eager import and keep only the lazy "module:class" registration. ## Bug 4: populate per-region NIXL registration bookkeeping - **State machine id**: nixl_register_local_xfer_region_mem_types_indexerror - **Commit**: 4ec533f ### Root cause vllm#53780 made the NIXL memory type, transfer group and name per-region, and register_local_xfer_handler now reads region_mem_types[0]; the HPU register_kv_caches override (a pre-#44456 copy kept to restore the K/V region split) never populated any of those lists, so every NIXL PD engine died with IndexError at KV cache registration. ### Culprit Regression introduced by [PR #53780](vllm-project/vllm#53780). ### Fix append region_mem_types / region_group_ids / region_names per region in the override's registration loop, derive _mixed_mem_types and _uses_region_group_mapping from them, and publish the local engine's dst_region_* geometry, matching upstream's post-#53780 tail. ## Bug 5: cap transformers below 5.17 - **State machine id**: mistral3_pixtral_rotary_embedding_import_error - **Commit**: 1e9da63 ### Root cause transformers 5.17.0 removed position_ids_in_meshgrid and renamed PixtralRotaryEmbedding to PixtralVisionRotaryEmbedding, both imported at module level by upstream vLLM's vllm/model_executor/models/pixtral.py, so PixtralForConditionalGeneration can no longer be inspected. ### Culprit Regression introduced by [PR #48105](huggingface/transformers#48105). ### Fix cap transformers<5.17, the version upstream vLLM tests against, until vLLM adopts the new vision rotary-embedding API. --------- Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com>
4 tasks
sbucaille
pushed a commit
to sbucaille/transformers
that referenced
this pull request
Sep 16, 2026
* delete * fix repo * . * add video processor docs * seems like this is expected * style * fix repo * i dont understand * ah this was not needed * add docstring back where needed * self reveiw * modular has no way to stop copying docstring... * comments * unpack kwargs * fix tests * oke, at least these are same, let's keep digging * . * second . * and like this * draft * maybe * commit before i break it * i think this is the last one from varity of ropes, now to update all models * vision-only rope qwen * run fix-repo * this will take soo long to fix every model * sooooo long * ! * one by one, hopefully i fix it all by eow * fix the rest * and left-out sam * fix repo most * and now fix repo for special omni models * a bit more of fix repo * new model was merged, ci wont run * suppress TRF, it comes from `main` * wording * . * gemma4, standardize 2D position IDs tmrw * fix repo * testers were working on pure luck, love it * fix repo * failing tests * a bit more * i suppose revert, why i changed it * fix style * lost type annotations * fix Sam copied incorrectly * qwen3 vl fix * move sam positions to buffer * modular convert * what? why CI never complained?! * forgot this * add the decorators where missing * ssome comments * more * axial rope type register separately * forgot * this is way too long * i am making bad decisions, revert * return dtype is fp32 in most models * ooops revert * bit more, rest comes tmrw * i broke smth and i dont know what * need to fix repo again * skip the test for vision rope for now, will add later more * example * Revert "i am making bad decisions, revert" This reverts commit 852a0c8. * axial as type * fix repo * fix repo again * missing * gemma4 * fix * comments * new mrope with 2 sections only, no temporal! * fix repo
1 task done
38 tasks
2 of 5 tasks
2 tasks done
4 of 5 tasks
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.
We need a standard way to compute rope freq in vision models which is usually only 2D or 3D. Atm we either interleave the grids inside rope module's forward or inside each attention layer. I think doing one interleave/permute or whatever needed in a rope module is easier and we keep the rest of modeling similar to LLMs, plus it is better in e2e perf (see table below)
I am trying to enforce an extra
self.recomposition_2d/3don all vision RoPE, this method has to be called on freqs before returning them thus permuting grids in the way model was trained. I ran a small bench onQwen2-VL2B and 7B version. Asked AI to summarize the numbers, and overall we get better e2e perf when we move the heavy work underModelVisionRotaryEmbedding. The trend is similar across model and batch sizes, so there is clearly a small win(btw, gemma4 qk should have been permuted, seems like they gave us an internal research code that simply calls text-rope twice and concats. Under the hood it is 2D rope as in any other ViT 😢 )
cc @IlyasMoutawwakil , should fix #46443
🤖 mlinter review state
45ab6ae83ade9fef