Switch GLM-5.2 MI355X HiCache write policy to write_through - #3008
Switch GLM-5.2 MI355X HiCache write policy to write_through#3008jiejingzhangamd wants to merge 2 commits into
Conversation
Ratio is relative to the device KV pool, so 1.5 resolved to 169.73 GB/rank on this cluster, or 679 GB pinned across the 4 TP ranks. 1.0 brings that to ~113 GB/rank (~453 GB). Allocation logic is unchanged; eval and sweep still share the same ratio-based sizing.
Keeps the TP host-pool ratio at 1.0 from the sibling eval PR. Eval and sweep still share the same sizing; only the write path changes.
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
![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 b169be3. Configure here.
| # write_through_selective skips DRAM writes for non-reusable KV blocks, | ||
| # reducing host-bus traffic without affecting the cache hit rate. | ||
| HICACHE_WRITE_POLICY="${HICACHE_WRITE_POLICY:-write_through_selective}" | ||
| HICACHE_WRITE_POLICY="${HICACHE_WRITE_POLICY:-write_through}" |
There was a problem hiding this comment.
Recipe change omits changelog entry
Medium Severity
The new HiCache defaults (HICACHE_RATIO 1.0 and HICACHE_WRITE_POLICY write_through) change published AgentX results for glm5.2-fp4-mi355x-sglang-agentic-mtp, but this PR does not append a perf-changelog.yaml entry. Changelog-driven sweeps will not select the recipe, so the new curve is not recorded.
Reviewed by Cursor Bugbot for commit b169be3. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked whether the new write_through default (set once, outside the TP/DP-attention if/else) unintentionally changes the DP-attention arm's write policy too — it does, but that's unchanged from before this diff (write_policy was already applied to both arms), so it's not a new issue introduced here.
Extended reasoning...
The diff is a small, config-only change to a single benchmark script, lowering the TP-arm HICACHE_RATIO default and switching HICACHE_WRITE_POLICY's default to write_through, consistent with the PR description. I verified the actual diff matches the described change and re-checked the previously flagged candidate about HICACHE_WRITE_POLICY being set outside the TP/DP-attention if/else: that placement is pre-existing (the removed write_through_selective line was in the same position, applying to both arms already), so this is not a newly introduced bug from this PR, just an existing pattern. The confirmed inline finding about the missing perf-changelog.yaml entry stands as the substantive issue and is already posted inline, so I'm not restating it here.
| # write_through_selective skips DRAM writes for non-reusable KV blocks, | ||
| # reducing host-bus traffic without affecting the cache hit rate. | ||
| HICACHE_WRITE_POLICY="${HICACHE_WRITE_POLICY:-write_through_selective}" | ||
| HICACHE_WRITE_POLICY="${HICACHE_WRITE_POLICY:-write_through}" |
There was a problem hiding this comment.
🔴 This diff changes default HICACHE_RATIO (1.5->1.0) and HICACHE_WRITE_POLICY (write_through_selective->write_through) for the TP arm, both of which affect benchmark performance, but adds no perf-changelog.yaml entry, violating AGENTS.md's non-negotiable invariant that every performance-affecting change requires one. Fix: append a new perf-changelog.yaml entry (byte-preserving, tail-only) describing this default change for the affected config-keys before merging. [also at: perf-changelog.yaml:1 - Two perf-affecting default changes (HICACHE_RATIO 1.5->1.0, HICACHE_WRITE_POLICY…]
Extended reasoning...
AGENTS.md line 31: "Every change that can affect benchmark performance ... requires a new perf-changelog.yaml entry. The file is append-only and byte-sensitive." The diff lowers HICACHE_RATIO from 1.5 to 1.0 (less host DRAM pinned, lower cache hit rate under high concurrency) and switches HICACHE_WRITE_POLICY from write_through_selective to write_through (every KV write now hits the host bus instead of skipping non-reusable blocks), both explicitly performance-tuning changes per the PR description itself. git diff --name-only shows only the .sh file changed; perf-changelog.yaml (confirmed present at repo root, last entries for pr-link .../2994) was not touched. Without an entry, downstream tooling/consumers that track perf-affecting changes via this changelog lose visibility into this benchmark's default behavior change.
Verification: normal (raised by review-scope AGENTS.md non-negotiable invariant). The diff changes performance-affecting HiCache defaults for the TP arm — HICACHE_RATIO 1.5→1.0 and HICACHE_WRITE_POLICY write_through_selective→write_through (glm5.2_fp4_mi355x_sglang_mtp.sh lines ~88 and ~92) — yet git diff --stat shows exactly one changed file (the .sh), with no perf-changelog.yaml entry appended.…
1 participant
What
Default
--hicache-write-policywrite_through_selective→write_through. Built on #3004 so the TP host-pool ratio stays 1.0. Eval and sweep still share the same ratio-based sizing.Why
#3004 evals-only already passed at ratio 1.0 +
write_through_selective: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/34566241629This PR is the write-policy A/B: same pool size,
write_through(every KV write hits host). That is the remaining HiCache delta vs puttingwrite_throughback on #2900.Test plan
test-config --config-files configs/amd-master.yaml --config-keys glm5.2-fp4-mi355x-sglang-agentic-mtp --evals-onlywrite_policy=write_throughand ratio 1.0 (~113 GB/rank)Made with Cursor