Fix is_contiguous() method call and remove duplicate imports#3789
Fix is_contiguous() method call and remove duplicate imports#3789yurekami wants to merge 2 commits intounslothai:mainfrom
Conversation
- Fix bug in rope_embedding.py where is_contiguous was used without parentheses, causing the method object (always truthy) to be evaluated instead of calling the method. This fixes issue unslothai#3781 where fast rope backpropagation was broken for zero strided/non-contiguous tensors. - Remove duplicate `import torch` in rl.py (lines 20 and 25) - Remove duplicate `import functools` and `import types` in vision.py 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @yurekami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where is_contiguous was used as a property instead of a method, and also removes some duplicate imports. The changes are good, but I have a suggestion to make the bug fix more robust and idiomatic.
In rope_embedding.py, the fix uses Q.clone() if not Q.is_contiguous() else Q. However, Q.clone() does not guarantee a contiguous tensor. A better approach is to use Q.contiguous(), which is the standard way to ensure a tensor is contiguous, creating a copy only when necessary. I've added comments with code suggestions to apply this improvement.
unsloth/kernels/rope_embedding.py
Outdated
| Q_out = Q.clone() if not Q.is_contiguous() else Q | ||
| K_out = K.clone() if not K.is_contiguous() else K |
There was a problem hiding this comment.
While adding the parentheses fixes the immediate bug of not calling is_contiguous, the logic can be improved for correctness and clarity.
Q.clone() does not guarantee that the resulting tensor is contiguous. If Q is not contiguous, Q.clone() will also be non-contiguous by default (preserving memory format).
A more idiomatic and correct way to ensure a tensor is contiguous is to use torch.Tensor.contiguous(). This method returns the tensor itself if it's already contiguous, and creates a contiguous copy otherwise. This achieves the goal of avoiding an unnecessary copy while ensuring the output tensor is always contiguous for the Triton kernel.
I suggest replacing these lines with:
| Q_out = Q.clone() if not Q.is_contiguous() else Q | |
| K_out = K.clone() if not K.is_contiguous() else K | |
| Q_out = Q.contiguous() | |
| K_out = K.contiguous() |
unsloth/kernels/rope_embedding.py
Outdated
| dQ_out = dQ.clone() if not dQ.is_contiguous() else dQ | ||
| dK_out = dK.clone() if not dK.is_contiguous() else dK |
There was a problem hiding this comment.
Similar to my previous comment, using .contiguous() is the more correct and idiomatic way to ensure the tensors are contiguous for the kernel. clone() does not guarantee contiguity.
| dQ_out = dQ.clone() if not dQ.is_contiguous() else dQ | |
| dK_out = dK.clone() if not dK.is_contiguous() else dK | |
| dQ_out = dQ.contiguous() | |
| dK_out = dK.contiguous() |
Address review feedback: .contiguous() is more idiomatic and only creates a copy when necessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
rope_embedding.pywhereis_contiguouswas used without parentheses, causing the method object (always truthy) to be evaluated instead of calling the methodrl.pyandvision.pyBug Fix (Fixes #3781)
The
is_contiguousmethod was used without parentheses on 4 lines inFast_RoPE_Embedding_QK:Since
is_contiguousis a method (not a property), the condition always evaluated toTrue(method objects are truthy), meaning clones were never made for non-contiguous tensors. This caused fast rope backpropagation to fail with zero-strided tensors.Files Changed
unsloth/kernels/rope_embedding.py- Fixed 4 instances (lines 315, 316, 398, 399)unsloth/models/rl.py- Removed duplicateimport torchunsloth/models/vision.py- Removed duplicateimport functoolsandimport typesTest plan
🤖 Generated with Claude Code