Skip to content

Conversation

mgoin
Copy link
Member

@mgoin mgoin commented Sep 17, 2025

Purpose

Fixes broken Model Executor Test https://buildkite.com/vllm/ci/builds/31128/steps/canvas?sid=019957f9-c884-4158-85bd-309efbb5f140 since #24078

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.

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 updates tests in tests/model_executor/test_model_load_with_params.py to align with a recent tokenizer refactoring. The changes correctly modify how the tokenizer's name and model_max_length are accessed in the test assertions. Specifically, model_tokenizer.tokenizer_id is replaced with model_config.tokenizer, and model_tokenizer.tokenizer.model_max_length is simplified to model_tokenizer.model_max_length. The changes are correct and necessary to fix the failing tests. I have no further recommendations.

@mgoin mgoin added ready ONLY add when PR is ready to merge/full CI is needed ci-failure Issue about an unexpected test failure in CI labels Sep 17, 2025
@mgoin mgoin changed the title [Ci Bugfix] Fix failing test_model_load_with_params tests due to tokenizer refactor [CI Bugfix] Fix failing test_model_load_with_params tests due to tokenizer refactor Sep 17, 2025
@vllm-bot vllm-bot merged commit e3db5eb into vllm-project:main Sep 17, 2025
21 of 32 checks passed
debroy-rh pushed a commit to debroy-rh/vllm that referenced this pull request Sep 19, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
charlifu pushed a commit to ROCm/vllm that referenced this pull request Sep 25, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-failure Issue about an unexpected test failure in CI ready ONLY add when PR is ready to merge/full CI is needed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants