[https://nvbugs/5934461][fix] Cherry-picks 11767 (logits support in disagg) - #11832
Conversation
…ap scheduler (NVIDIA#11795) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
… in disagg (NVIDIA#11767) Signed-off-by: Balaram Buddharaju <169953907+brb-nv@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR introduces first-generation logits propagation through the disaggregated execution pipeline. It adds Changes
Sequence DiagramsequenceDiagram
participant Executor as PyExecutor
participant Result as PyResult
participant Params as DisaggregatedParams
participant Protocol as OpenAI Protocol
Note over Executor,Protocol: First-Gen Logits Propagation Flow
Executor->>Executor: _maybe_prepend_logprobs_and_logits()
activate Executor
Executor->>Result: append generation_logits
activate Result
Result->>Result: store logits chunks
deactivate Result
deactivate Executor
Executor->>Result: get_latest_logits_unexcluded()
activate Result
Result-->>Executor: return unexcluded logits
deactivate Result
Executor->>Params: first_gen_logits assignment
activate Params
Params->>Params: store first_gen_logits
deactivate Params
Params->>Protocol: serialize first_gen_logits
activate Protocol
Protocol->>Protocol: base64 encode + dtype handling
Protocol-->>Params: JSON-safe representation
deactivate Protocol
Protocol->>Params: deserialize first_gen_logits
activate Params
Params->>Params: restore torch.Tensors
deactivate Params
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/executor/result.py (1)
305-312:⚠️ Potential issue | 🔴 CriticalAdd missing
loggerimport
logger.warning()at line 305 references an undefined variable. Theloggermodule is used but never imported, causing aNameErrorat runtime when disaggregated serving encounters a logprobs mismatch.Proposed fix
+from ..logger import logger🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/executor/result.py` around lines 305 - 312, The warning call uses an undefined symbol `logger`, causing a NameError; fix by adding a module-level logger and importing the stdlib logging module (e.g., import logging and initialize logger = logging.getLogger(__name__)) at the top of tensorrt_llm/executor/result.py so the `logger.warning(...)` call (around the logprobs mismatch message) has a defined logger object; place the import/initialization near other imports to match file style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/serve/openai_protocol.py`:
- Around line 1137-1176: The serializer and deserializer currently assume
well-formed inputs; update _serialize_first_gen_logits to validate the top-level
first_gen_logits is a list/sequence and that each element is a torch.Tensor
(raise ValueError with a clear message otherwise), and validate tensor
attributes (e.g., contiguous/cpu convertible) before encoding; similarly update
_deserialize_first_gen_logits to validate the top-level serialized is a list,
each item is a dict and that "data" is a string, "shape" is a list of ints, and
"dtype" is a valid numpy dtype string (raise ValueError for violations), and
check that the decoded bytes length is compatible with the inferred dtype/shape
before constructing the ndarray and torch.Tensor.
In `@tests/integration/defs/disaggregated/test_disaggregated_single_gpu.py`:
- Around line 794-796: The test currently swallows exceptions in the
cancellation block by printing the exception in the except Exception as e
handler and continuing to finally, which can hide failures; update the except
block in the cancellation test to either re-raise the caught exception (raise)
after logging or call pytest.fail(str(e)) so the test fails on errors—modify the
except Exception as e handler that prints "Exception encountered: {e}" to raise
the exception (or invoke pytest.fail) to ensure real regressions surface.
In `@tests/unittest/_torch/executor/test_chunked_logits.py`:
- Around line 179-180: The test constructs an incorrect 4D expected tensor by
calling .unsqueeze(1) before transpose; remove .unsqueeze(1) so the expected
shape matches the 3D output of get_latest_logits_unexcluded(). Update the
assertion lines that build expected from logits_b (e.g., replace
logits_b.unsqueeze(1).transpose(0, 1) with logits_b.transpose(0, 1) or simply
logits_b.transpose as appropriate) and make the same change for the similar
block around lines 196-202 to ensure shapes match before torch.allclose;
reference get_latest_logits_unexcluded, logits_b, and unexcluded when locating
the fixes.
---
Outside diff comments:
In `@tensorrt_llm/executor/result.py`:
- Around line 305-312: The warning call uses an undefined symbol `logger`,
causing a NameError; fix by adding a module-level logger and importing the
stdlib logging module (e.g., import logging and initialize logger =
logging.getLogger(__name__)) at the top of tensorrt_llm/executor/result.py so
the `logger.warning(...)` call (around the logprobs mismatch message) has a
defined logger object; place the import/initialization near other imports to
match file style.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/disaggregated_params.pytensorrt_llm/executor/result.pytensorrt_llm/serve/openai_protocol.pytests/integration/defs/disaggregated/test_disaggregated_single_gpu.pytests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/test-db/l0_h100.ymltests/unittest/_torch/executor/test_chunked_logits.pytests/unittest/disaggregated/test_openai_disagg_service.py
|
/bot run --disable-fail-fast |
|
PR_Github #37367 [ run ] triggered by Bot. Commit: |
|
PR_Github #37367 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #37411 [ run ] triggered by Bot. Commit: |
|
PR_Github #37411 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #37506 [ run ] triggered by Bot. Commit: |
|
PR_Github #37506 [ run ] completed with state |
ea128b0
into
NVIDIA:release/1.3.0rc5.post1
Summary by CodeRabbit
New Features
Tests
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.