[Bugfix] Fix mypy errors for StructuredOutputsParams by using stdlib dataclass#34693
[Bugfix] Fix mypy errors for StructuredOutputsParams by using stdlib dataclass#34693hyeongyun0916 wants to merge 5 commits intovllm-project:mainfrom
Conversation
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request correctly resolves mypy errors for StructuredOutputsParams by switching its decorator from pydantic.dataclasses.dataclass to the standard library's dataclasses.dataclass. Since StructuredOutputsParams does not leverage any Pydantic-specific functionality, this change effectively fixes type-checking issues encountered with dataclasses.replace() without impacting runtime behavior. The modification is well-justified and properly implemented.
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
njhill
left a comment
There was a problem hiding this comment.
Thanks @hyeongyun0916 this had also been annoying for me, though I could swear it started much more recently than #22772.
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
There was a problem hiding this comment.
Thanks @hyeongyun0916 for pushing the changes. I did not see your PR when I started adding a fix in #34739, sorry about that.
I took another approach. Like you I didn't see any obvious reasons for changing to Pydantic but I didn't want to alter it in case there maybe serialization somewhere and also not knowing the background for the change. Also, it meant overriding the replace() method which I though was overkill.
As Pydantic dataclasses do not satisfy mypy's type system with replace() even though they are fully compatible at runtime, I decided to follow recommended practice which is to ignore that specific error type-var. My reasoning is that its a "known mypy/pydantic interop gap" and has no side effects.
No worries at all! I totally agree with your approach. I’ve actually been looking into fixing the CI on my end, and as you mentioned, it was becoming unnecessarily complex. Given it's a known interop gap between mypy and Pydantic, using ignore seems like the most practical and cleanest way to handle it. Thanks for the heads up! |
Thanks @hyeongyun0916 for the feedback. How would you like to progress this and get the fix in? Do you want to update this PR and I pick up the changes in #34739 or close this PR in favour of #34739? |
|
FYI we do have a pydantic compatible Lines 105 to 113 in 1391378 If we want to keep the pydantic validation, I'd recommend using this instead of |
I'm closing this PR in favor of that one. 👍 |
Purpose
Fix 4 mypy errors that occur when running
pre-commit run --all-fileslocally.The
mypy-localhook (--follow-imports skip) checks all files together, causing mypy to see the actualStructuredOutputsParamsdefinition insampling_params.py. Since it usespydantic.dataclasses.dataclassinstead of the stdlibdataclasses.dataclass, mypy does not recognize it as a valid dataclass — which breaksdataclasses.replace()calls and constructor keyword resolution.StructuredOutputsParamshad pydantic's@dataclassapplied, butdataclasses.replace()only passes mypy's type check with stdlib dataclasses. Since no pydantic-specific features (validators, etc.) are used, switching to stdlibdataclasshas no impact on runtime behavior.This was introduced in #22772. During that refactor, the
dataclassimport was switched from stdlib to pydantic, butStructuredOutputsParamsdoes not use any pydantic-specific features (validators, etc.). If this was an intentional change, please let me know.Fix: Change
from pydantic.dataclasses import dataclassback tofrom dataclasses import dataclass.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.