Skip to content

[CI] Fix mock type narrowing in LoRA serving test - #59344

Merged
khluu merged 1 commit into
vllm-project:mainfrom
khluu:fix/lora-test-asyncmock-typing
Sep 30, 2026
Merged

khluu merged 1 commit into
vllm-project:mainfrom
khluu:fix/lora-test-asyncmock-typing

Conversation

@khluu

@khluu khluu commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Summary

Fix the main-branch pre-commit failure introduced by #59286 (failed job). OpenAIServingModels.engine_client is typed as EngineClient, so mypy sees add_lora as an async method and rejects assert_not_called(), even though the test creates it with MagicMock(spec=EngineClient).

Assert that add_lora is an AsyncMock before checking its call count. This narrows the type while preserving the regression test's guarantee that rejected adapter names never reach the engine. No production code changes.

Searched open PRs for 59286 in:body, LoRA mypy, and "assert_not_called"; no existing PR addresses this failure.

Validation

  • Reproduced the exact attr-defined error on unmodified main with the repository's mypy 1.20.2 wrapper.
  • .venv/bin/pre-commit run --files tests/entrypoints/serve/lora/test_serving_models.py --hook-stage manual — all applicable hooks passed, including mypy for Python 3.10, 3.11, 3.12, and 3.13.
  • Dependency-isolated smoke using the real EngineClient class definition: its MagicMock produces an AsyncMock for add_lora, and assert_not_called() rejects an unexpected call.
  • git diff --check passed.
  • Full serving pytest suite was not run locally; this checkout has lint dependencies only. Model evaluation is not applicable to this test-only change.

AI assistance: prepared and validated with Codex.

Why #59286 passed before merging

Its passing pre-commit run tested merge commit 4865d3e against base 7aa8372 and finished at 2026-09-29 19:39 UTC. At that point, tests/entrypoints/serve used --follow-imports skip.

At 20:27 UTC, #51043 (5dd693122c) merged and moved this directory to SILENT_GROUPS, enabling follow_imports="silent". #59286 merged at 2026-09-30 01:29 UTC, after that stricter check landed. The earlier green run therefore did not validate their combined state.

Confirmed on the same unfixed test with mypy 1.20.2: --follow-imports skip passes; --follow-imports silent reproduces the exact line-115 attr-defined error. Skipping imports hides the imported EngineClient method type; following imports exposes it.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: khluu <khluu000@gmail.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@AndreasKaratzas

Copy link
Copy Markdown
Member

How is this possible? All pre ommit checks passed in that PR. Weird ...

@khluu
khluu merged commit bec710f into vllm-project:main Sep 30, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants