Skip to content

Conversation

@Isotr0py
Copy link
Member

@Isotr0py Isotr0py commented Nov 28, 2025

Purpose

  • Remove Olmo3 and FlexOlmo config copy, because Transformers v4.57 has included these models' config

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Isotr0py <[email protected]>
@Isotr0py Isotr0py added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 28, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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"]

    ...

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

Comment on lines 17 to 20
import torch
from torch import nn
from transformers import FlexOlmoConfig

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Member

@DarkLight1337 DarkLight1337 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants