fix(sglang): support model_path in GDS loading - #605
Conversation
Signed-off-by: DAI0818 <daii-0818@users.noreply.github.com>
WalkthroughGDS loading now resolves the checkpoint path from ChangesGDS model path resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modelexpress_client/python/tests/test_gds_loader.py (1)
265-289: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the remaining path-resolution branches.
This test covers only the case where
modelis absent. Add cases where both fields exist, wheremodelis empty andmodel_pathis set, and where neither field exists. These cases verify precedence, fallback, and failure behavior. ThroughGdsStrategy.load, the missing-path error is wrapped asStrategyFailed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/tests/test_gds_loader.py` around lines 265 - 289, Extend test_gds_uses_sglang_model_path to cover GdsStrategy.load path resolution when both model and model_path are set, when model is empty with model_path set, and when neither field exists. Assert that model takes precedence, model_path is used as the fallback, and the missing-path case raises the wrapped StrategyFailed error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modelexpress_client/python/tests/test_gds_loader.py`:
- Around line 265-289: Extend test_gds_uses_sglang_model_path to cover
GdsStrategy.load path resolution when both model and model_path are set, when
model is empty with model_path set, and when neither field exists. Assert that
model takes precedence, model_path is used as the fallback, and the missing-path
case raises the wrapped StrategyFailed error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea72bed3-4987-46b7-a04d-18363b2d1e57
📒 Files selected for processing (2)
modelexpress_client/python/modelexpress/load_strategy/gds_strategy.pymodelexpress_client/python/tests/test_gds_loader.py
|
PR #605: @zhengluo-nv Could you please review this SGLang/GDS compatibility fix when you have a chance? It allows GDS loading to resolve SGLang's |
1 participant
Summary
modelor SGLangmodel_pathProblem
The shared GDS strategy currently reads
ctx.model_config.model. SGLang exposes the checkpoint asmodel_path, so GDS loading fails immediately with:The strategy then falls back to the default disk loader, preventing SGLang from using GDS.
Testing
Summary by CodeRabbit
Bug Fixes
Tests