[nvbug/5393888][nvbug/5393042] Always use py_seq_slot
#6147
Conversation
Buggy flow prior to fix, for given request X:
0. state:
* scheduler is MAX_UTILIZATION = pausing is enabled
* overlap scheduler is enabled = previous batch is processed *after* current batch is scheduled
1. [`self._pause_requests(scheduled_batch.paused_requests)`](https://github.com/NVIDIA/TensorRT-LLM/blob/a7184869001d28ca70a738e9862ea91cb147da8c/tensorrt_llm/_torch/pyexecutor/py_executor.py#L1110) is called for request X: `seq_slot` is deleted [**by cpp**](https://github.com/NVIDIA/TensorRT-LLM/blob/d71c6fe5267f4b61c51cc39d4594cdcb417f0703/cpp/include/tensorrt_llm/batch_manager/llmRequest.h#L830) when pause is called.
2. **After Pausing X**, [`self._update_requests(self.previous_batch.sample_state)`](https://github.com/NVIDIA/TensorRT-LLM/blob/d71c6fe5267f4b61c51cc39d4594cdcb417f0703/tensorrt_llm/_torch/pyexecutor/py_executor.py#L1138) is called, which also holds X in `self.previous_batch.sample_state.scheduled_requests.all_requests()`.
3. `update_requests` reads the seq_slot off of X, but it has been erased in 1.
cpp deletes the seq_slot for its uses, [as an implementation detail within the scheduler](https://github.com/NVIDIA/TensorRT-LLM/blob/d71c6fe5267f4b61c51cc39d4594cdcb417f0703/cpp/tensorrt_llm/batch_manager/assignReqSeqSlots.cpp#L33)
`py_seq_slot` is not deleted when the request is paused, so we opt to use it. For consistency, we also transition to use this across the whole codebase where `seq_slot` was used before.
Signed-off-by: Netanel Haber <[email protected]>
WalkthroughThe changes systematically update internal references from Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
netanel-haber
changed the title
py_seq_slotpy_seq_slot
|
/bot run |
|
PR_Github #12215 [ run ] triggered by Bot |
|
PR_Github #12215 [ run ] completed with state |
…ot-is-erased-by-cpp-when-request-is-paused
|
/bot run |
|
PR_Github #12277 [ run ] triggered by Bot |
|
PR_Github #12277 [ run ] completed with state |
Yeah, I agree it isn't ideal but I'm okay with this because, as mentioned in the description - the C++ code resetting the seq_slot is an implementation detail of inner scheduling details, not a fundamental property of the seq_slot mechanism. |
…ot-is-erased-by-cpp-when-request-is-paused
|
/bot run |
|
PR_Github #12319 [ run ] triggered by Bot |
|
PR_Github #12319 [ run ] completed with state |
Signed-off-by: Netanel Haber <[email protected]>
Signed-off-by: Netanel Haber <[email protected]>
Signed-off-by: Netanel Haber <[email protected]> Signed-off-by: Shreyas Misra <[email protected]>
Buggy flow prior to fix, for given request X:
State:
Flow:
self._pause_requests(scheduled_batch.paused_requests)is called for request X:seq_slotis deleted by cpp when pause is called.self._update_requests(self.previous_batch.sample_state)is called, which also holds X inself.previous_batch.sample_state.scheduled_requests.all_requests().update_requestsreads the seq_slot off of X, but it has been erased in 2.cpp deletes the seq_slot for its uses, as an implementation detail within the scheduler.
py_seq_slotis not deleted when the request is paused, so we opt to use it. For consistency, we also transition to use this across the whole codebase whereseq_slotwas used before.Summary by CodeRabbit