Skip to content

Conversation

@njhill
Copy link
Member

@njhill njhill commented Nov 17, 2025

Addressing this review comment from #28768.

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 renames the ec_producer field to is_ec_producer in vllm/v1/engine/core.py. This change improves code clarity and follows Python's naming conventions for boolean variables.

However, the refactoring appears to be incomplete. I've noticed that vllm/v1/executor/ray_executor.py still uses the old ec_producer name. To ensure consistency across the codebase and prevent potential confusion or bugs in the future, I recommend renaming self.ec_producer to self.is_ec_producer in vllm/v1/executor/ray_executor.py as well. Specifically, the definition at lines 103-106 and its usage at line 404 should be updated.

@WoosukKwon WoosukKwon added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 17, 2025
@WoosukKwon WoosukKwon enabled auto-merge (squash) November 17, 2025 23:36
@WoosukKwon WoosukKwon merged commit da8dadf into vllm-project:main Nov 18, 2025
43 checks passed
@njhill njhill deleted the rename-core-field branch November 18, 2025 17:28
Victor49152 pushed a commit to Victor49152/vllm that referenced this pull request Nov 20, 2025
bhagyashrigai pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Nov 20, 2025
bigPYJ1151 pushed a commit that referenced this pull request Nov 25, 2025
bringlein pushed a commit to bringlein/vllm that referenced this pull request Nov 26, 2025
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
kitaekatt pushed a commit to kitaekatt/vllm that referenced this pull request Dec 1, 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.

2 participants