Repository navigation
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ShangmingCai
left a comment
ShangmingCai
left a comment
There was a problem hiding this comment.
nixl seems to have a notify mechanism and use notif_map = self.agent.get_new_notifs() to recv notif, so these timeout params might not be needed for nixl backend since it can notify and abort the requests when an error happens.
But I am not quite sure, maybe this PR should be reviewed by @ishandhanani
When nixl is used as the transport backend, the kv_indices are still transmitted via zmq, so the prefill side still requires a timeout mechanism to terminate potential infinite waits for kv_indices. The notify mechanism of nixl occurs during the send_aux and send_kvcache stages, which take place after the kv_indices received. |
There was a problem hiding this comment.
Pull request overview
This PR adds a timeout mechanism to the bootstrapping phase of the nixl connector in PD disaggregation mode to prevent indefinite waiting when decode nodes or ZMQ communication fail.
Changes:
- Added configurable bootstrap timeout via
SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUTenvironment variable (default: 300s) - Implemented timeout tracking and handling in the
KVTransferHandle.poll()method to abort timed-out requests - Added initialization timestamp tracking for timeout calculation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.warning_once( | ||
| "Some requests timed out when bootstrapping, " | ||
| "which means prefill instances fail to receive the KV indices from the decode instance of this request. " | ||
| "If a greater mean TTFT is acceptable, you can 'export SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUT=600' (10 minutes) to relax the timeout condition. " | ||
| ) |
There was a problem hiding this comment.
The error message uses logger.warning_once() which may not be appropriate for timeout events that could occur for multiple different requests. Consider using logger.warning() instead, or include the request identifier in the message to ensure each unique timeout is logged.
| if self.init_time is not None: | ||
| now = time.time() | ||
| elapsed = now - self.init_time | ||
| if elapsed >= self.kv_mgr.bootstrap_timeout: | ||
| logger.warning_once( | ||
| "Some requests timed out when bootstrapping, " | ||
| "which means prefill instances fail to receive the KV indices from the decode instance of this request. " | ||
| "If a greater mean TTFT is acceptable, you can 'export SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUT=600' (10 minutes) to relax the timeout condition. " | ||
| ) | ||
| self.kv_mgr.record_failure( | ||
| self.bootstrap_room, | ||
| f"Request {self.bootstrap_room} timed out after {elapsed:.1f}s in KVPoll.Bootstrapping", | ||
| ) | ||
| self.conclude_state = KVPoll.Failed | ||
| return KVPoll.Failed |
There was a problem hiding this comment.
The check if self.init_time is not None is unnecessary since init_time is always initialized to time.time() in __init__ at line 740 and is never set to None. This redundant check adds confusion about when init_time might be None.
| if self.init_time is not None: | |
| now = time.time() | |
| elapsed = now - self.init_time | |
| if elapsed >= self.kv_mgr.bootstrap_timeout: | |
| logger.warning_once( | |
| "Some requests timed out when bootstrapping, " | |
| "which means prefill instances fail to receive the KV indices from the decode instance of this request. " | |
| "If a greater mean TTFT is acceptable, you can 'export SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUT=600' (10 minutes) to relax the timeout condition. " | |
| ) | |
| self.kv_mgr.record_failure( | |
| self.bootstrap_room, | |
| f"Request {self.bootstrap_room} timed out after {elapsed:.1f}s in KVPoll.Bootstrapping", | |
| ) | |
| self.conclude_state = KVPoll.Failed | |
| return KVPoll.Failed | |
| now = time.time() | |
| elapsed = now - self.init_time | |
| if elapsed >= self.kv_mgr.bootstrap_timeout: | |
| logger.warning_once( | |
| "Some requests timed out when bootstrapping, " | |
| "which means prefill instances fail to receive the KV indices from the decode instance of this request. " | |
| "If a greater mean TTFT is acceptable, you can 'export SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUT=600' (10 minutes) to relax the timeout condition. " | |
| ) | |
| self.kv_mgr.record_failure( | |
| self.bootstrap_room, | |
| f"Request {self.bootstrap_room} timed out after {elapsed:.1f}s in KVPoll.Bootstrapping", | |
| ) | |
| self.conclude_state = KVPoll.Failed | |
| return KVPoll.Failed |
|
Thanks @rexrock. Closing this because it has had no updates in 199 days. Reopen it if the work is still relevant. Some directories moved recently, so an older branch may need retargeting: |
Motivation
In the PD separation mode, the nixl connector lacks a timeout mechanism during the bootstrapping phase. This can lead to the requests on the prefill side not being released when issues arise with the decode node or ZMQ communication.
Modifications
Following the implementation of Mooncake, this PR adds a timeout mechanism to the bootstrapping phase of the nixl connector. The timeout duration is configured via SGLANG_DISAGGREGATION_BOOTSTRAP_TIMEOUT, with a default value of 300s.
Accuracy Tests
Benchmarking and Profiling
Checklist
Review Process
/tag-run-ci-label,/rerun-failed-ci,/tag-and-rerun-ci