[BugFix][Typing] Fix Imprecise Type Annotations #15208

WrRan · 2025-03-20T10:57:45Z

as title said.

github-actions · 2025-03-20T10:57:54Z

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

WrRan · 2025-03-20T11:12:55Z

vllm/v1/engine/core_client.py

ctx must be set, or line-235 will fail because ctx is None.

I checked the codebase. The BackgroundResources is only used by

vllm/vllm/v1/engine/core_client.py

Line 293 in e3f813c

self.resources = BackgroundResources(ctx=sync_ctx)

So it is ok to remove the defaults.

njhill

Thanks for the fixes @WrRan

njhill · 2025-03-20T18:19:20Z

vllm/v1/engine/core_client.py

I'm not sure this comment is necessary. Our pre-commit checks will catch 3.9 incompatibilities.

Did we land code non-compatible with python 3.9 first, then turned on pre-commit checks? Just curious.

@houseroad I don't think so - the code here is compatible with 3.9. The comment was a hypothetical explanation about why we can't add TypeAlias here yet.

njhill · 2025-03-20T18:20:52Z

@WrRan you need to sign-off your commits, see https://github.com/vllm-project/vllm/pull/15208/checks?check_run_id=39103177991. And while you're updating perhaps remove that comment per my suggestion?

houseroad

Looks good.

WrRan · 2025-03-21T04:15:19Z

@WrRan you need to sign-off your commits, see https://github.com/vllm-project/vllm/pull/15208/checks?check_run_id=39103177991. And while you're updating perhaps remove that comment per my suggestion?

Got and ready.

njhill · 2025-03-21T04:26:08Z

Thanks @WrRan. Could you force-push your commits again and include a signoff for the DCO (as described here)? It's fine to squash them into one.

WrRan · 2025-03-21T07:51:08Z

Got. And now commit history is clear. Happy. 😄 @njhill

WrRan · 2025-03-21T07:58:22Z

vllm/v1/serial_utils.py

@njhill Have a look. Any includes None, so Optional is unnecessary.

Do we have a stricter bound for this? fwiw Any is equivalent as not adding type, as it doesn't give any contextual usage of the parameter (but for the sake of passing mypy)

I would prefer to keep the Optional here since None has a different meaning to any other value

aarnphm · 2025-03-21T10:10:43Z

vllm/v1/engine/core_client.py

because this is a dataclass, you can gated the default value under dataclasses.field

ctx: zmq.Context = dataclasses.field(default=None)

Same for L216 and L217

or you can apply the change to L216 and L217 instead of ctx, given that it seems ctx must not be None.

because this is a dataclass, you can gated the default value under dataclasses.field

ctx: zmq.Context = dataclasses.field(default=None)

Same for L216 and L217

Not good. mypy complains.

aarnphm

LGTM. can you rename the title according to https://docs.vllm.ai/en/latest/contributing/overview.html#pr-title-and-classification

njhill

@WrRan could you revert that other optional change per my new comment.

Otherwise the changes look good to me, you just need to fix the DCO thing before we can merge it. Thanks again

njhill · 2025-03-21T15:21:52Z

vllm/v1/serial_utils.py

I would prefer to keep the Optional here since None has a different meaning to any other value

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

WrRan · 2025-03-21T16:20:46Z

@njhill Got, and reverted. And I have known what is sign-off. 😅

Another question:

vllm/vllm/v1/engine/__init__.py

Lines 110 to 119 in cfbb8c9

    
           class UtilityOutput( 
        
                   msgspec.Struct, 
        
                   array_like=True,  # type: ignore[call-arg] 
        
                   gc=False):  # type: ignore[call-arg] 
        
               call_id: int 
        
               # Non-None implies the call failed, result should be None. 
        
               failure_message: Optional[str] = None 
        
               result: Any = None

The UtilityOutput can be omit_defaults=True as EngineCoreOutputs to reduce serialization costs. Right? Or anything miss?

Additionally, I have been carefully reading (or more accurately, learning) vllm v1. The current architecture is elegant. THANKS FOR YOUR WORKS AND OPEN-SOURCE SPIRIT. ❤️ ❤️ ❤️

WrRan · 2025-03-21T16:22:17Z

@aarnphm Got, and I will name PR accordingly.

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com> Signed-off-by: Louis Ulmer <ulmerlouis@gmail.com>

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com> Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>

WrRan requested review from WoosukKwon, alexm-redhat, comaniac, njhill, robertgshaw2-redhat and ywang96 as code owners March 20, 2025 10:57

mergify bot added the v1 label Mar 20, 2025

WrRan commented Mar 20, 2025

View reviewed changes

njhill approved these changes Mar 20, 2025

View reviewed changes

houseroad approved these changes Mar 20, 2025

View reviewed changes

WrRan force-pushed the patch-7 branch 2 times, most recently from 1fdf30d to 28b5c34 Compare March 21, 2025 07:50

WrRan commented Mar 21, 2025

View reviewed changes

WrRan force-pushed the patch-7 branch from f3127e2 to cc2499c Compare March 21, 2025 09:44

aarnphm reviewed Mar 21, 2025

View reviewed changes

aarnphm approved these changes Mar 21, 2025

View reviewed changes

njhill reviewed Mar 21, 2025

View reviewed changes

vllm/v1/serial_utils.py Outdated

Copy link

Member

njhill Mar 21, 2025

Choose a reason for hiding this comment

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

I would prefer to keep the Optional here since None has a different meaning to any other value

njhill changed the title ~~Fix Imprecise Type Annotations~~ [BugFix][Typing] Fix Imprecise Type Annotations Mar 21, 2025

fix type annotations

6d50875

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

WrRan force-pushed the patch-7 branch from cc2499c to 6d50875 Compare March 21, 2025 16:09

njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 22, 2025

vllm-bot merged commit dd861b9 into vllm-project:main Mar 22, 2025
35 of 39 checks passed

erictang000 pushed a commit to erictang000/vllm that referenced this pull request Mar 25, 2025

[BugFix][Typing] Fix Imprecise Type Annotations (vllm-project#15208)

a2f128f

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025

[BugFix][Typing] Fix Imprecise Type Annotations (vllm-project#15208)

ccd7ac3

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com> Signed-off-by: Louis Ulmer <ulmerlouis@gmail.com>

ckhordiasma mentioned this pull request Apr 17, 2025

[do not merge] pr test for nm changes into 2.20 red-hat-data-services/vllm#107

Closed

lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025

[BugFix][Typing] Fix Imprecise Type Annotations (vllm-project#15208)

9444472

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025

[BugFix][Typing] Fix Imprecise Type Annotations (vllm-project#15208)

b161d36

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com>

RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025

[BugFix][Typing] Fix Imprecise Type Annotations (vllm-project#15208)

d3230bf

Signed-off-by: Wang Ran (汪然) <wrran@outlook.com> Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>

Uh oh!

[BugFix][Typing] Fix Imprecise Type Annotations #15208

[BugFix][Typing] Fix Imprecise Type Annotations #15208

Uh oh!

Conversation

WrRan commented Mar 20, 2025

Uh oh!

github-actions bot commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill commented Mar 20, 2025

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

WrRan commented Mar 21, 2025

Uh oh!

njhill commented Mar 21, 2025

Uh oh!

WrRan commented Mar 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarnphm left a comment

Choose a reason for hiding this comment

Uh oh!

njhill left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WrRan commented Mar 21, 2025

Uh oh!

WrRan commented Mar 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

njhill left a comment •

edited

Loading