Skip to content

Conversation

skyloevil
Copy link
Contributor

@skyloevil skyloevil commented Aug 3, 2025

Replace generic Exception with TypeError for all mutating operations in ConstantList class. This provides more precise error semantics as these operations attempt to modify an immutable data structure, which is fundamentally a type compatibility issue.

Changes:

  • append, extend, insert, pop, remove, clear methods
  • setitem and delitem magic methods

This improves error handling precision and follows Python best practices for exception types.

Copy link

github-actions bot commented Aug 3, 2025

👋 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.

🚀

Replace generic Exception with TypeError for all mutating operations in
ConstantList class. This provides more precise error semantics as these
operations attempt to modify an immutable data structure, which is
fundamentally a type compatibility issue.

Changes:
- append, extend, insert, pop, remove, clear methods
- __setitem__ and __delitem__ magic methods

This improves error handling precision and follows Python best practices
for exception types.

Signed-off-by: zitian.zhao <[email protected]>
@mergify mergify bot added the v1 label Aug 3, 2025
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 correctly refactors the ConstantList class to raise TypeError instead of a generic Exception for mutating operations. This is a good improvement as it provides more specific and standard error semantics for an immutable collection. I've added a couple of high-severity comments to address incorrect method signatures for insert and pop, which, if corrected, will ensure the class behaves consistently with Python's built-in list and raises the intended errors.

vllm/v1/utils.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The insert method signature is incorrect. It should match the list.insert(index, object) signature to correctly override the behavior and provide the intended TypeError. The current signature insert(self, item) takes only one argument besides self, which will cause a TypeError about the number of arguments if a user tries to call it like list.insert(0, value), preventing the intended "Cannot insert..." error from being raised.

Suggested change
def insert(self, item):
def insert(self, index, item):

vllm/v1/utils.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The pop method signature is incorrect. It should match list.pop([index]), which takes an optional index argument. The current signature pop(self, item) requires one argument, which is inconsistent with list.pop() that can be called with zero or one argument (the index). This will lead to unexpected TypeError exceptions related to arguments, rather than the intended one about immutability.

Suggested change
def pop(self, item):
def pop(self, index=-1):

@skyloevil skyloevil force-pushed the optimize-constantlist-exceptions branch from f035195 to 5f09cad Compare August 3, 2025 17:21
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) August 4, 2025 02:45
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 4, 2025
@vllm-bot vllm-bot merged commit 49bcd89 into vllm-project:main Aug 4, 2025
45 of 49 checks passed
npanpaliya pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Aug 6, 2025
jinzhen-lin pushed a commit to jinzhen-lin/vllm that referenced this pull request Aug 9, 2025
noamgat pushed a commit to noamgat/vllm that referenced this pull request Aug 9, 2025
paulpak58 pushed a commit to paulpak58/vllm that referenced this pull request Aug 13, 2025
diegocastanibm pushed a commit to diegocastanibm/vllm that referenced this pull request Aug 15, 2025
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants