-
Notifications
You must be signed in to change notification settings - Fork 224
[Agentx][sglang] config update #2145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Oasis-Git
wants to merge
9
commits into
main
Choose a base branch
from
agentx-sglang-update
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+122
−26
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
e196e0a
dsv4 sglang agentic: MegaMoE DEP + TP-only low-latency recipes (B300)
Oasis-Git c423995
dsv4 sglang agentic B200: low-latency mem-fraction + DEP conc-split r…
Oasis-Git 783261d
dsv4 sglang agentic: trim conc points (B200 DEP8 no-offload; B300 tp8…
Oasis-Git 716d955
perf-changelog: trigger dsv4 sglang agentic B200/B300 MegaMoE + low-l…
Oasis-Git 20cb504
Merge remote-tracking branch 'origin/main' into agentx-sgl-update
Oasis-Git 2cbb3d0
dsv4 sglang agentic B300: set DEP4 mem-fraction to 0.93 (0.835 leaves…
Oasis-Git dda80a0
dsv4 sglang agentic B200: trim DEP8/hicache conc points (nvidia-master)
Oasis-Git 32dc320
dsv4 sglang agentic B300: bump DEP8 mem-fraction 0.835 -> 0.85
Oasis-Git 2449b42
dsv4 sglang agentic B200: DEP high-conc mem-fraction 0.835->0.85 + co…
Oasis-Git File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 B200 non-DP branch now sets MEM_FRACTION_STATIC=0.90 unconditionally, but the accompanying comment justifies this as "the low-latency TP-only path (conc <= 16) runs GPU-only with no HiCache spill" — the gate is only
DP_ATTENTION=false, with no CONC or KV_OFFLOADING check. The pairedconfigs/nvidia-master.yamlstill contains the non-DP HiCache row{ tp: 8, kv-offloading: dram, kv-offload-backend: hicache, conc-list: [8, 16, 32] }, so that whole row (including conc=32, above the stated <=16 window) will now run at 0.90 while HiCache spills, contradicting the retained "leave the same HBM headroom … so a nearly full GPU KV cache does not OOM while HiCache is spilling" comment three lines up. Either mirror the B300 gate (elif { [ "$TP" = "8" ] || [ "$TP" = "4" ]; } && [ "${CONC:-999}" -le 16 ]; then … MEM_FRACTION_STATIC=0.90atdsv4_fp4_b300_sglang.sh:156), gate onKV_OFFLOADING=none, or drop the non-DP HiCache row from the config.Extended reasoning...
What the bug is
In
benchmarks/single_node/agentic/dsv4_fp4_b200_sglang.sh(post-PR lines ~149–161), theMEM_FRACTION_STATICselector now reads:The new comment scopes the 0.90 raise to "the low-latency TP-only path (conc <= 16) runs GPU-only with no HiCache spill". But the actual gate is only
DP_ATTENTION=false— it does not checkCONCand does not checkKV_OFFLOADING. Every non-DP invocation gets 0.90, including HiCache-enabled and conc>16 cases.Why the retained comment matters
The comment immediately above the new one is the previous author's explicit invariant:
That is exactly why 0.88 was chosen. Raising to 0.90 for the non-DP + HiCache case removes ~2% of 192 GB ≈ ~3.8 GB of HBM headroom the previous author intentionally reserved for the DSv4 indexer while HiCache spills.
Step-by-step proof from the paired config
The PR also modifies
configs/nvidia-master.yaml(line ~14088) but keeps a non-DP HiCache row:- { tp: 8, kv-offloading: dram, kv-offload-backend: hicache, conc-list: [8, 16, 32] }Trace the runner for
conc=32from that row:DP_ATTENTION=false,KV_OFFLOADING=dram,KV_OFFLOAD_BACKEND=hicache,CONC=32,TP=8.DP_ATTENTION=truebranch → false; falls into the else (line ~160):MEM_FRACTION_STATIC=0.90.require_agentic_kv_offload_backend hicacheis true →--enable-hierarchical-cacheis passed, HiCache is active and will spill.MEM_FRACTION_STATIC=0.90, which is exactly the case the retained comment says must run at 0.88. Additionally conc=32 exceeds the "conc <= 16" bound the new comment claims. Same failure mode for conc=8 and conc=16 in that row (still HiCache-enabled).Pre-PR that same run used 0.88; post-PR it uses 0.90.
Cross-check against the sibling script
The companion
benchmarks/single_node/agentic/dsv4_fp4_b300_sglang.sh(line ~156) does this correctly in the same PR:So the author knew the correct pattern — gate 0.90 on the low-latency window and keep 0.88 for the general non-DP path — it's just missing on B200.
Fix
Any one of these resolves the code/comment/config three-way inconsistency:
{ TP=8 || TP=4 } && CONC <= 16(or equivalently gate onKV_OFFLOADING=none); keep 0.88 for non-DP + HiCache.{ tp: 8, kv-offloading: dram, kv-offload-backend: hicache, conc-list: [8, 16, 32] }fromconfigs/nvidia-master.yaml, so the 0.90 non-DP path never coincides with HiCache spill.Severity note
Marking nit because the ~3.8 GB delta may or may not trigger an actual OOM at long-context HiCache spill in practice — full-sweep-fail-fast validation will catch it if it does. But the code violates its own documented invariant, and the B300 mirror in the same PR shows the intended shape; the fix is a two-line gate change.