-
Notifications
You must be signed in to change notification settings - Fork 224
[AMD][MI355X] DSv4 SGLang agentic: v0.5.14 image, router, HiCache #2146
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
seungrokj
wants to merge
8
commits into
main
Choose a base branch
from
amd/agentx_dsv4_sgl
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.
+169
−101
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
fec0cee
[AMD][MI355X] Update DSv4 SGLang agentic launcher to v0.5.14 image wi…
seungrokj 3fe2c5e
[AMD][MI355X] Add dram-utilization 0.80 to DSv4 SGLang agentic sweep …
seungrokj e282bf7
Update perf-changelog with PR link for DSv4 SGLang agentic entry
seungrokj 6acad04
[AMD][MI355X] Remove --context-length and comment out extra sweep con…
seungrokj 7e1c332
[AMD][MI355X] DSv4 SGLang agentic: use compressed attention, 2x max-r…
seungrokj 2ecf4e9
[AMD][MI355X] Fix cudaHostRegister OOM: lower hicache-ratio to 4, adj…
seungrokj f504c26
Merge branch 'main' into amd/agentx_dsv4_sgl
seungrokj fcffec7
[AMD][MI355X] DSv4 SGLang agentic: focus sweep on DPA configs, scale …
seungrokj 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
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.
🟡 The default arm of the
KV_OFFLOAD_BACKENDcase at line 85 prints(expected: native, mooncake, lmcache), but the case above only acceptshicache— the listed values look copy-pasted from the sibling vLLM launchers (which do accept native/mooncake/lmcache) and would themselves fall into the same default arm here. Sweep configs only usehicacheso CI never trips this branch, but anyone debugging by hand will be misdirected. Fix: change the list to(expected: hicache).Extended reasoning...
What the bug is
The
case "${KV_OFFLOAD_BACKEND:-}"block at lines 56-88 ofbenchmarks/single_node/agentic/dsv4_fp4_mi355x_sglang.shhas exactly one accepting arm:hicache). Anything else falls into the default*)arm at lines 84-87, which emits:None of
native,mooncake, orlmcacheare recognized by this case statement — they are all backends supported by other launchers in the repo (nativeforkimik2.5-fp4-mi355x-vllm-agentic,mooncakeforminimaxm3-fp8-mi300x-vllm-agentic,lmcachefor the siblingdsv4-fp4-mi355x-vllm-agentic). The message is a copy-paste from those vLLM-family launchers.Why the message is self-contradictory
A user who sees
unsupported KV_OFFLOAD_BACKEND 'foo'and reads the parenthetical hint will pick one ofnative,mooncake, orlmcache— and hit the exact same default arm and the exact same message. In the case where they pickednative, the output literally readsunsupported KV_OFFLOAD_BACKEND 'native' (expected: native, mooncake, lmcache), which is self-contradictory.Step-by-step proof
KV_OFFLOADING=dram KV_OFFLOAD_BACKEND=native(a natural attempt after seeing another dsv4 launcher uselmcache).agentic_kv_offload_enabledreturns true becauseKV_OFFLOADING=dram.caseat line 56 and evaluates the arms in order. The only arm ishicache).nativedoes not matchhicache), so control falls to*)at line 84.Error: unsupported KV_OFFLOAD_BACKEND 'native' (expected: native, mooncake, lmcache).mooncakeorlmcache. Same path — steps 3-6 repeat.Why existing code doesn't prevent it
The header comment at line 7 states "KV_OFFLOADING=dram requires KV_OFFLOAD_BACKEND=hicache", so the canonical documented value is
hicache. The error text just wasn't updated to match when this launcher was adapted from the vLLM sibling.Impact
Cosmetic misdirection in a diagnostic-only path. The four sweep entries in
configs/amd-master.yamlfor this recipe all passkv-offload-backend: hicache, so CI never hits the default arm. The cost is a minute or two of wasted debug time for someone experimenting with a non-hicachebackend by hand.Fix
Change line 85's parenthetical to
(expected: hicache):On the refutations
Both refutations are duplicate-management: bug_002 and bug_004 target the same defect at the same lines with the same fix, and the refuters explicitly note the finding itself is real — they only refuted one of the two to avoid posting the same comment twice. The synthesis agent merged them into a single report, which resolves that concern.