Optimize GLM-5.2-MXFP4 SGLang Agentic Performance on MI355X - #2900
Optimize GLM-5.2-MXFP4 SGLang Agentic Performance on MI355X#2900jiejingzhangamd wants to merge 7 commits into
Conversation
…ng-opt-0908 Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # perf-changelog.yaml
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…lang-opt-0908 Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # perf-changelog.yaml
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 52835e9. Configure here.
| - "Update the SGLang ROCm image from lmsysorg/sglang-rocm:v0.5.16-rocm720-mi35x-20260728 to lmsysorg/sglang-rocm:v0.5.19-rocm720-mi35x-20260907." | ||
| - "Pick up our recent SGLang main-branch optimizations for GLM-5.2-MXFP4 serving." | ||
| - "Restore HiCache write_through as the default write policy to optimize GLM-5.2-MXFP4 output interactivity and per-GPU throughput in the MI355X AgentX configuration." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2900 |
There was a problem hiding this comment.
Changelog entry not appended at tail
Medium Severity
The new glm5.2-fp4-mi355x-sglang-agentic-mtp changelog block was inserted before later main entries and a historical separator line was dropped. perf-changelog.yaml is append-only: new entries belong at the physical end, and existing bytes including separator whitespace stay unchanged. A YAML parse does not catch this; validate_raw_change and the reuse merge helper require a byte-identical historical prefix.
Reviewed by Cursor Bugbot for commit 52835e9. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on the perf-changelog.yaml trailing-line removal, I checked two other AGENTS.md conventions for this diff: the --use-chat-template requirement for *_mtp.sh scripts calling run_benchmark_serving (this script is an agentic-coding benchmark that doesn't call run_benchmark_serving directly, so it doesn't apply), and the model.container == image rule for image bumps (this is a single-node config with no container key, so that multi-node rule doesn't apply either).
Extended reasoning...
This diff is a small, mechanical-looking change (image version bump plus a one-line default-policy revert) but it carries a reported finding: the removal of the pre-existing trailing whitespace-only line at the end of perf-changelog.yaml, which conflicts with AGENTS.md's explicit "append-only, byte-sensitive" invariant for that file. Since a reported finding exists, I did not restate it but instead checked adjacent conventions that could plausibly also be violated by this change — the --use-chat-template flag requirement and the model.container/image consistency rule — and confirmed both are inapplicable to these particular files (no run_benchmark_serving call in this agentic script; no container key in this single-node config entry). Given the outstanding inline finding, a human should still review before merge.
| @@ -6957,15 +6967,3 @@ | |||
| - "Tune the 8k/1k serving recipe with --mamba-full-memory-ratio 0.37 and --linear-attn-prefill-backend flashinfer; raise max-prefill-tokens and chunked-prefill-size from 16384 to 32768 and mem-fraction-static from 0.8 to 0.86." | |||
| - "Expand the TP8 and TP4/EP1 sweep coverage through concurrency 640." | |||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2866 | |||
There was a problem hiding this comment.
🟡 (optional) The diff deletes the file's pre-existing trailing whitespace-only line (old file ended "...pull/2866\n \n", new ends "...pull/2866\n"), violating AGENTS.md's rule that perf-changelog.yaml is append-only/byte-sensitive: preserve all existing bytes and separator whitespace, append only at the tail. Fix: re-add the removed trailing line so no existing bytes are altered; only append the new entry above it, never edit or truncate the tail.
Extended reasoning...
Base file (at 4caeb31) ends with the last pr-link line followed by a line containing two spaces and a newline (confirmed via od -c). HEAD's perf-changelog.yaml drops that trailing line entirely (git diff shows a bare deletion with no matching addition at file end). AGENTS.md line 31 states the file is append-only and byte-sensitive and existing bytes/separator whitespace must be preserved, appends only at the tail — this diff instead mutates the tail by removing a byte sequence, which any tooling relying on byte-stable diffs/checksums against this file would flag or choke on, and sets precedent that trailing separator content can be silently dropped in future PRs.
Verification: normal (violates a documented non-negotiable invariant this diff introduces). Base perf-changelog.yaml at 4caeb31 ends with ...pull/2866\n \n — od -c on the base tail shows 2866 \n \n (a trailing line of two spaces plus newline after the final pr-link). HEAD's file ends ...pull/2866\n — od -c on the current tail shows 2866 \n with the whitespace-only line gone. The diff's last…
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34286209780 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
5 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=34312259596 |
3 participants
Summary
write_throughas the default HiCache write policy.mainand preserve both concurrent performance-changelog entries.Supersedes #2887.
Benchmark Results
Test configuration: MI355X, TP4/EP4, MTP, HiCache DRAM offload, 3600-second AgentX profiling.
Concurrency 10 provides the best balance of output interactivity and per-GPU throughput.
Validation
perf-changelog.yamlparses successfully after mergingmain.Made with Cursor