feat: add load_lora_adapter_from_distributed api - #27268
yushengsu-thu merged 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to load LoRA adapters dynamically from a distributed process group without restarting the server. It adds the load_lora_adapter_from_distributed endpoint and corresponding methods across the engine, HTTP server, scheduler, tokenizer manager, TP worker, and model runner. Feedback on these changes includes fixing a potential AttributeError during LRU eviction where a non-existent loaded_adapters attribute is accessed, replacing a worker-crashing assert statement with a graceful error response, validating input list lengths before zipping them to prevent silent truncation, securing the new HTTP endpoint with the appropriate authentication decorator, and improving type hint consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| unload_result = await self._unload_lora_adapter_locked( | ||
| UnloadLoRAAdapterReqInput(lora_name=lru_lora_name) | ||
| ) | ||
| if not unload_result.success: | ||
| raise ValueError( | ||
| f"Error while unloading LRU LoRA adapter '{lru_lora_name}': " | ||
| f"{unload_result.error_message}" | ||
| ) | ||
| del result.loaded_adapters[lru_lora_name] |
There was a problem hiding this comment.
The result object is an instance of LoRAUpdateOutput (or its alias LoadLoRAAdapterFromDistributedReqOutput), which does not have a loaded_adapters attribute. Accessing result.loaded_adapters will raise an AttributeError and crash the request when LRU eviction is triggered. Since _unload_lora_adapter_locked already handles the unloading process, this line should be removed.
| unload_result = await self._unload_lora_adapter_locked( | |
| UnloadLoRAAdapterReqInput(lora_name=lru_lora_name) | |
| ) | |
| if not unload_result.success: | |
| raise ValueError( | |
| f"Error while unloading LRU LoRA adapter '{lru_lora_name}': " | |
| f"{unload_result.error_message}" | |
| ) | |
| del result.loaded_adapters[lru_lora_name] | |
| unload_result = await self._unload_lora_adapter_locked( | |
| UnloadLoRAAdapterReqInput(lora_name=lru_lora_name) | |
| ) | |
| if not unload_result.success: | |
| raise ValueError( | |
| f"Error while unloading LRU LoRA adapter '{lru_lora_name}': " | |
| f"{unload_result.error_message}" | |
| ) |
| assert group_name in self._model_update_group, ( | ||
| f"Group {group_name} not in {list(self._model_update_group.keys())}. " | ||
| "Please call `init_weights_update_group` first." | ||
| ) |
There was a problem hiding this comment.
Using assert to validate the presence of group_name in self._model_update_group will raise an AssertionError and crash the worker process if the group is missing. It is safer to handle this gracefully by returning a LoRAUpdateOutput(success=False, ...) so the server can report the error back to the client without crashing.
if group_name not in self._model_update_group:
error_msg = (
f"Group {group_name} not in {list(self._model_update_group.keys())}. "
"Please call `init_weights_update_group` first."
)
logger.error(error_msg)
return LoRAUpdateOutput(success=False, error_message=error_msg)| tensors = {} | ||
| handles = [] | ||
| for name, dtype, shape in zip(names, dtypes, shapes): |
There was a problem hiding this comment.
zip(names, dtypes, shapes) will silently truncate if the input lists have mismatched lengths. This can lead to silent failures where some weights are not loaded. We should validate that all three lists have the same length before zipping them.
if not (len(names) == len(dtypes) == len(shapes)):
raise ValueError("names, dtypes, and shapes must have the same length")
tensors = {}
handles = []
for name, dtype, shape in zip(names, dtypes, shapes):| @app.api_route("/load_lora_adapter_from_distributed", methods=["POST"]) | ||
| async def load_lora_adapter_from_distributed( |
There was a problem hiding this comment.
The new endpoint /load_lora_adapter_from_distributed is missing the @auth_level(AuthLevel.ADMIN_OPTIONAL) decorator. State-modifying administrative endpoints should be protected to prevent unauthorized access, consistent with /unload_lora_adapter.
| @app.api_route("/load_lora_adapter_from_distributed", methods=["POST"]) | |
| async def load_lora_adapter_from_distributed( | |
| @app.api_route("/load_lora_adapter_from_distributed", methods=["POST"]) | |
| @auth_level(AuthLevel.ADMIN_OPTIONAL) | |
| async def load_lora_adapter_from_distributed( |
| config_dict: Dict, | ||
| names: list[str], | ||
| dtypes: list[str], | ||
| shapes: list[list[int]], | ||
| group_name: str = "weight_update_group", | ||
| pinned: bool = False, | ||
| added_tokens_config: Optional[Dict] = None, |
There was a problem hiding this comment.
Type hints for config_dict and added_tokens_config use Dict without generic parameters, and mix PEP 585 list with typing.Dict. It is better to use Dict[str, Any] for better type safety and consistency with io_struct.py.
| config_dict: Dict, | |
| names: list[str], | |
| dtypes: list[str], | |
| shapes: list[list[int]], | |
| group_name: str = "weight_update_group", | |
| pinned: bool = False, | |
| added_tokens_config: Optional[Dict] = None, | |
| config_dict: Dict[str, Any], | |
| names: list[str], | |
| dtypes: list[str], | |
| shapes: list[list[int]], | |
| group_name: str = "weight_update_group", | |
| pinned: bool = False, | |
| added_tokens_config: Optional[Dict[str, Any]] = None, |
|
Hi, We have a concrete use case (LoRA weight-sync for RL) and have the colocated/IPC path working; we're very interested in the distributed adapter API this PR adds. Is there anything blocking merge, and would review or testing help? |
yushengsu-thu
left a comment
There was a problem hiding this comment.
verify and test done
Adapted new LoadLoRAAdapterFromDistributedReqInput to v0.5.15's BaseReq(kw_only=True) msgspec Struct style.
Adapted new LoadLoRAAdapterFromDistributedReqInput to v0.5.15's BaseReq(kw_only=True) msgspec Struct style.
…psert (#27268, #31759, #30913) Squashes the two follow-ups into the API commit: #31759 fixes this commit's HTTP handler (the LoRAUpdateOutput was returned unserialized) and #30913 rewrites its registry-registration block to support in-place upsert, so neither applies on its own. Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai> Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…psert (#27268, #31759, #30913) Squashes the two follow-ups into the API commit: #31759 fixes this commit's HTTP handler (the LoRAUpdateOutput was returned unserialized) and #30913 rewrites its registry-registration block to support in-place upsert, so neither applies on its own. Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai> Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…psert (#27268, #31759, #30913) Squashes the two follow-ups into the API commit: #31759 fixes this commit's HTTP handler (the LoRAUpdateOutput was returned unserialized) and #30913 rewrites its registry-registration block to support in-place upsert, so neither applies on its own. Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai> Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…psert (#27268, #31759, #30913) Squashes the two follow-ups into the API commit: #31759 fixes this commit's HTTP handler (the LoRAUpdateOutput was returned unserialized) and #30913 rewrites its registry-registration block to support in-place upsert, so neither applies on its own. Co-authored-by: Ethan (Yusheng) Su <yushengsu@radixark.ai> Co-authored-by: Mathew Han <49226490+mathewjhan@users.noreply.github.com>
…psert (sgl-project#27268, sgl-project#31759, sgl-project#30913) (cherry picked from commit f8a11ee)
Motivation
To support distributed training for lora in miles framework
currently load_lora_adapter_from_tensor uses IPC to sync weight, however IPC can not be used in multi node env.
We need a similar api as update_weight_from_distributed, so that sglang side can receive lora weights through NCCL/ethernet.
The weight update flow is similar to update_weight_from_distributed:
miles side PR: radixark/miles#988
Modifications
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ci