Repository navigation
Conversation
2d32735 to
117fd62
Compare
117fd62 to
58b238a
Compare
58b238a to
b3db124
Compare
📝 WalkthroughWalkthroughThe loader now verifies loaded weights through a shared helper, and multiple load paths call it. Several model classes opt into verification or adjust optional-weight handling, while the new unit test covers missing and optional parameters. ChangesWeight loading verification
Sequence Diagram(s)sequenceDiagram
participant L as BaseModelLoader.load_weights_and_postprocess
participant H as _load_and_verify_weights
participant M as model.load_weights
participant U as unloaded_required_params
L->>H: pass model and weights
H->>M: load_weights(weights)
alt model.verify_weights_on_load
H->>U: compute missing required params
U-->>H: missing set
alt missing set is not empty
H-->>L: raise RuntimeError
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
b3db124 to
36a4e9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@python/sglang/srt/model_loader/loader.py`:
- Around line 339-340: The completeness check in loader.py only verifies
model.named_parameters(), so it can miss required tensors that load_weights()
also populates from buffers. Update the verification path around
unloaded_required_params/model completeness to use the same tensor source
contract as load_weights(), including any buffer-backed entries merged into
params_dict by models like gemma4_mm and gemma4_causal. Then add a regression
test covering a required buffer-backed tensor to ensure it is reported as
missing when absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f304e31-50e5-4eb8-a19a-bc332a0d5fba
📥 Commits
Reviewing files that changed from the base of the PR and between 3344b73 and 36a4e9b68b1fdd96ce9ef6e6f544b6063fb8ec52.
📒 Files selected for processing (10)
python/sglang/srt/model_loader/loader.pypython/sglang/srt/model_loader/weight_completeness.pypython/sglang/srt/models/deepseek_ocr.pypython/sglang/srt/models/deepseek_v4.pypython/sglang/srt/models/deepseek_v4_nextn.pypython/sglang/srt/models/gemma4_causal.pypython/sglang/srt/models/gemma4_mm.pypython/sglang/srt/models/gemma4_unified.pypython/sglang/srt/models/mllama4.pytest/registered/unit/model_loader/test_load_and_verify_weights.py
💤 Files with no reviewable changes (1)
- python/sglang/srt/models/gemma4_unified.py
d4b09eb to
fe3aeed
Compare
…t#32540) Co-authored-by: Jiminator <69131491+Jiminator@users.noreply.github.com>
…oE to flashinfer_trtllm (sgl-project#32818)
sgl-project#32834) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…nav with Kimi (sgl-project#32835) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: R0CKSTAR <yeahdongcn@gmail.com>
…raph track buffer (sgl-project#32555) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ention_heads_per_layer) (sgl-project#32625)
06e7e7d to
62d7418
Compare
Upstream Review Target
The upstream PR for this branch is sgl-project#29323.
This fork-local PR mirrors the same branch and is kept only as a fork-side view of the final-state implementation.
Motivation
load_weights(weights)should be the reusable tensor-copy primitive. Full-checkpoint completeness is a loader concern because online/RL partial updates and pinned-cache reloads intentionally load only selected tensors.Changes
verify_weights_on_load.is_optional_weight(name).Verification
git diff --checkpython3 -m py_compileon touched Python filesruff format --checkruff checkon the new helper/test and changed DeepSeek NextN surfaceunloaded_required_paramsSummary by CodeRabbit
New Features
Bug Fixes
Tests
CI States
Latest PR Test (Base): ❌ Run #30517416707
Latest PR Test (Extra): ❌ Run #30517416638