fix rope_theta -> rope_parameters['rope_theta'] #3651
Conversation
Summary of ChangesHello @mmathew23, 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 addresses a breaking change introduced in Transformers v5, where the Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to add backward compatibility for a change in transformers where rope_theta was moved into a RopeParameters object. The changes correctly identify the need to handle both cases. However, I've found a couple of areas where the implementation could be made more robust. Specifically, both changes introduce bare except: blocks, which can hide bugs and make debugging difficult. In unsloth/models/llama.py, the fallback logic for rope_theta has a latent bug that could lead to a KeyError. I've provided suggestions to catch more specific exceptions and to fix the potential bug, which will make the code more robust and maintainable.
| except: | ||
| base = getattr(config, "rope_parameters", {}) | ||
| base = base["rope_theta"] |
There was a problem hiding this comment.
Using a bare except: is generally discouraged as it can catch unexpected errors (like SystemExit or KeyboardInterrupt) and hide bugs. It's better to catch specific exceptions. In this case, you're expecting an AttributeError.
Additionally, the current implementation has a latent bug: if config.rope_theta doesn't exist and config.rope_parameters also doesn't exist, getattr will return an empty dictionary, and the subsequent access base["rope_theta"] will raise a KeyError. This can be confusing to debug.
A cleaner and safer approach is to catch AttributeError specifically and then directly access config.rope_parameters["rope_theta"]. This will provide a more informative error (AttributeError or KeyError) if neither rope_theta nor rope_parameters are found as expected.
| except: | |
| base = getattr(config, "rope_parameters", {}) | |
| base = base["rope_theta"] | |
| except AttributeError: | |
| base = config.rope_parameters["rope_theta"] |
| if "RopeParameters" in config: | ||
| try: | ||
| exec(f"from {config_filepath} import RopeParameters", globals()) | ||
| except: |
There was a problem hiding this comment.
Using a bare except: is risky because it catches all exceptions, including system-exiting ones like SystemExit or KeyboardInterrupt, and can hide bugs by silently continuing the loop. It's better to catch a more specific exception. If you expect import-related issues, except ImportError: would be appropriate. If other errors from exec are possible, except Exception: is a safer alternative to a bare except.
| except: | |
| except Exception: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try: | ||
| base = config.rope_theta | ||
| except: | ||
| base = getattr(config, "rope_parameters", {}) | ||
| base = base["rope_theta"] |
There was a problem hiding this comment.
With transformers v5 configs where rope_theta was moved into a RopeParameters object, LlamaRotaryEmbedding.__init__ falls back to base = getattr(config, "rope_parameters", {}) and immediately indexes base["rope_theta"]. In v5 rope_parameters is a dataclass object, not a dict, so when configs no longer expose config.rope_theta this path raises 'RopeParameters' object is not subscriptable, preventing model initialization in those environments. The fallback needs to read the attribute rather than subscript the object.
Useful? React with 👍 / 👎.
2 participants
Transformers v5 refactored rope theta. It now sits inside a RopeParameters object. This PR both fixes the issue for the transformers v5 refactor and keep it backwards compatible.
@danielhanchen the llama.py code has a comment
# [TODO] Hack to pass in config - need to remove later. Is that still relevant?