Skip to content

Conversation

@hukongyi
Copy link

@hukongyi hukongyi commented Dec 4, 2025

What this PR does / why we need it?

This PR fixes a pydantic.ValidationError in the unit tests caused by recent changes in the upstream vLLM project.

Upstream vLLM PR #29859 removed the default value for is_encoder_decoder in SchedulerConfig, making it a required argument during initialization. As a result, the existing unit test tests/ut/core/test_schedule_config.py was failing.

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

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 aims to fix a pydantic.ValidationError in the unit tests by adding the required is_encoder_decoder argument to the SchedulerConfig initialization, following an upstream change. The change correctly addresses the issue in the setUp method. However, I've identified another instance of SchedulerConfig initialization within the same test file that is also missing this required argument, which will likely cause another test to fail. My review includes a comment pointing out this omission to ensure all tests pass.

max_model_len=8192,
is_multimodal_model=False,
send_delta_data=False,
is_encoder_decoder=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This correctly adds the missing is_encoder_decoder argument. However, there is another instantiation of SchedulerConfig in this file that also needs to be updated. On line 90, SchedulerConfig is called without is_encoder_decoder, which will likely cause the test_valid_config_with_multimodal test to fail with a pydantic.ValidationError.

Please update lines 90-91 from:

SchedulerConfig(is_multimodal_model=True,
                max_num_batched_tokens=8192), {})

to:

SchedulerConfig(is_multimodal_model=True,
                max_num_batched_tokens=8192,
                is_encoder_decoder=False), {})

@hukongyi hukongyi closed this Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant