Repository navigation
[Bugfix] Don't shutdown Engine on py_generator=True - #6334
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/entrypoints.md. Module owners: @alex-jw-brooks @linyueqian @NickCao @alex-jw-brooks, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot: no human activity for 20 days@alex-jw-brooks this pull request has had no human commit, comment or review since 2026-08-19. Per repository policy it may be closed if it stays inactive. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
6c56485 to
8c93e63
Compare
Omni ReviewBot routing recordAssigned Direct under experiment |
Omni ReviewBot triage noteResolved as of |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
Consuming Omni.generate(..., py_generator=True) no longer shuts down the shared engine when the generator finishes. The old _run_generation_with_generator path ran self.close() in a finally, so a fully evaluated generator left the request queue dead and a second generate raised janus.ShutDown. The fix yields through _run_generation only, keeps exception-path/__del__/explicit shutdown cleanup, and updates unit tests to assert the engine stays alive after generator use while still shutting down on Omni deletion.
Change flow
flowchart TD
A["[EXISTING] Caller list(Omni.generate py_generator=True)"]:::existing --> B["[CHANGED] Omni._run_generation_with_generator<br/>remove finally self.close()"]:::changed
B --> C["[EXISTING] Omni._run_generation / engine queues"]:::existing
C --> D["[EXISTING] OmniBase weakref.finalize / explicit close()"]:::existing
B --> E["[NEW] test_omni_pygenerator_does_not_kill_engine<br/>+ flipped shutdown_called asserts"]:::new
D --> F["[NEW] test_del_shutsdown_engine"]:::new
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Findings
- [P1] PR body missing Test Plan/Result vs Ready entrypoints guard —
.buildkite/cuda/test-ready.yml:23
Frozen PR body has Purpose/repro text but no Test Plan or Test Result naming what was run. Ready CI guard step "Simple · Engine&Entrypoints Test" runspytest -sv tests/entrypoints tests/engine -m 'core_model and cpu'and is the lane that coverstests/entrypoints/test_omni_entrypoints.py(pytestmark = core_model, cpu). Please record that selector (or an equivalenttests/entrypoints/test_omni_entrypoints.py -m 'core_model and cpu'run) plus results; informal "added a couple of tests" is not enough for CI alignment.
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
8c93e63 to
ceacaaf
Compare
Signed-off-by: Alex Brooks <albrooks@redhat.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Purpose
Currently, running generator with
py_generator=Trueshuts down the engine when we evaluate the generator. I.e.,the second generate call will blow up because we unconditionally close + shut down the engine when
py_generator=True.We also hold a weakref to the engine and shut it down through that, so added a couple of tests to ensure calling generate with a generator doesn't shut down the engine