[https://nvbugs/6245861][fix] Gate the two ID None-checks on finish_reason in _GEN_PENDING_FINISH_REASONS… - #14908
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors CTX finish-reason handling in the OpenAI disaggregated service by centralizing which finish reasons indicate a pending GEN handoff into a module constant, then updates both the GEN decision logic and response validation to use this constant. Response validation now conditionally enforces disaggregated parameter IDs only during pending GEN transitions, allowing early CTX completions to omit these fields. ChangesDisaggregated Service Finish-Reason Conditional Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This is exactly how I was planning to fix the bug. 💯 |
|
/bot run --disable-fail-fast |
|
PR_Github #52019 [ run ] triggered by Bot. Commit: |
|
PR_Github #52019 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52192 [ run ] triggered by Bot. Commit: |
|
PR_Github #52192 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52297 [ run ] triggered by Bot. Commit: |
|
PR_Github #52297 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52338 [ run ] triggered by Bot. Commit: |
|
PR_Github #52338 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52678 [ run ] triggered by Bot. Commit: |
|
PR_Github #52678 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #52925 [ run ] triggered by Bot. Commit: |
|
PR_Github #52925 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #53056 [ run ] triggered by Bot. Commit: |
|
PR_Github #53056 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
7685d21 to
3316698
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #54163 [ run ] triggered by Bot. Commit: |
3316698 to
2023d8c
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #54275 [ run ] triggered by Bot. Commit: |
|
PR_Github #54163 [ run ] completed with state |
|
PR_Github #54275 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #54394 [ run ] triggered by Bot. Commit: |
|
PR_Github #54394 [ run ] completed with state
|
…ctx_request_id A CTX request that finishes early (finish_reason='stop', e.g. EOS during prefill) never sets up the KV-cache handoff, so ctx_request_id and disagg_request_id stay None. _verify_ctx_response ran before _need_gen and unconditionally required both IDs, turning a completed request into HTTP 500. Gate the ID None-checks on finish_reason in _GEN_PENDING_FINISH_REASONS, mirroring _need_gen which already skips the handoff for such responses. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
2023d8c to
c1ff26d
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #54454 [ run ] triggered by Bot. Commit: |
|
PR_Github #54454 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #54463 [ run ] triggered by Bot. Commit: |
|
PR_Github #54463 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #54581 [ run ] triggered by Bot. Commit: |
|
PR_Github #54581 [ run ] completed with state |
…reason in _GEN_PENDING_FINISH_REASONS`… (NVIDIA#14908) Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
…reason in _GEN_PENDING_FINISH_REASONS`… (NVIDIA#14908) Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
Summary
finish_reason in _GEN_PENDING_FINISH_REASONS(shared constant also used by _need_gen); update the broken unit test to use a GEN-pending finish_reason and add positive tests for completed responses.Test plan
Links
Summary by CodeRabbit
Bug Fixes
Tests