-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[Chore]: Remove Olmo3 and FlexOlmo config copy #29677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Isotr0py <[email protected]>
Signed-off-by: Isotr0py <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly removes the local copies of Olmo3Config and FlexOlmoConfig, using the versions from the transformers library instead. This is a good cleanup. However, there is a critical issue with the removal of the local Olmo3Config. The local version contained an important architecture override that is now missing, which will break loading for olmo3 models. I've added a specific comment with a suggested fix.
| import torch | ||
| from torch import nn | ||
| from transformers import Olmo2Config | ||
| from transformers import Olmo2Config, Olmo3Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's correct to import Olmo3Config from transformers, this change has an unintended side effect. The removed local vllm.transformers_utils.configs.Olmo3Config contained a crucial architecture override for olmo3 models, mapping them to Olmo2ForCausalLM which is implemented in this file. The new transformers.Olmo3Config defaults to the Olmo3ForCausalLM architecture, which is not registered in vLLM, and will cause model loading to fail for olmo3 models.
To fix this, you should re-introduce the architecture override. A good place for this would be in vllm/transformers_utils/config.py within the get_config function, after parsing the configuration:
# in vllm/transformers_utils/config.py
def get_config(...):
...
config_dict, config = config_parser.parse(...)
# This model uses Olmo3ForCausalLM in transformers but Olmo2ForCausalLM
# in vLLM.
if config.model_type == "olmo3":
config.architectures = ["Olmo2ForCausalLM"]
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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".
| import torch | ||
| from torch import nn | ||
| from transformers import FlexOlmoConfig | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing transformers version bump for new Flex/Olmo configs
The new imports of FlexOlmoConfig from transformers drop our vendored config copies, but the repo still advertises compatibility with transformers >=4.56.0 (see requirements/common.txt). FlexOlmoConfig and Olmo3Config were only added in upstream transformers 4.57, so in any environment that installs 4.56.x—still allowed by our requirements—flex_olmo.py (and olmo2.py) will crash with ImportError at import time. The lower bound should be raised or a fallback retained to avoid breaking supported installs.
Useful? React with 👍 / 👎.
DarkLight1337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirements/common.txt still allows v4.56 so we cannot force this change yet.
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.