Repository navigation
fix(agentx): role-aware, window-scoped ATOM cache rates with validity / 修正 ATOM 缓存命中率口径并标注有效性 - #3964
chunfangamd wants to merge 5 commits into
Conversation
… / 修正 ATOM 缓存命中率口径并标注有效性 The ATOM agentic result adapter summed prefix_cache_full_tokens over prefill and decode endpoints, reported lmcache_loaded_tokens (transfer volume) as CPU reuse, and set overall equal to GPU. The AIPerf metrics section also spans warmup. - Take endpoint roles from the launcher inputs (SRT_PREFILL/DECODE/AGG_ENDPOINTS). - Count prefill (or unified) endpoints over timeslices inside the profiling window. - CPU = prefix_cache_offload_tokens / full; external aliases CPU; overall = GPU + CPU. - Emit server_metrics.cache_validity so consumers know whether the rates are a complete, same-scope actual observation; refuse invalid or unprovable inputs. Refs: run 36734492117, #3543, #3158
andyluo7
left a comment
andyluo7
left a comment
There was a problem hiding this comment.
Requesting changes for one accounting issue. The endpoint-role accounting, profiling-window slicing, offload-based CPU reuse, and explicit refusal states are otherwise coherent. However, error accounting currently mixes warmup and profiling, so a harmless warmup failure can suppress an otherwise valid pricing observation.
I reviewed head 2db01926476a2dd93640c76e089787287575b45b; exact-head Lint and Tests are green. Deployment should remain blocked until phase-scoped error accounting is fixed and the corrected app-side guard is live.
中文
因一个计数问题请求修改。除此之外，endpoint role 计数、profiling 窗口切分、基于 offload 的 CPU 复用，以及明确的拒绝状态整体上是合理且一致的。但当前错误计数混合了 warmup 和 profiling，因此一次无害的 warmup 失败也会屏蔽原本有效的定价观测。
我审核的是提交 2db01926476a2dd93640c76e089787287575b45b；该提交的 Lint 和 Tests 均已通过。在修复按阶段统计错误的问题、并上线已修正的 app 侧保护逻辑之前，部署应继续保持阻塞。
| return { | ||
| "profiling_requests": len(records), | ||
| "profiling_prompt_tokens": sum(prompt) if len(prompt) == len(records) else None, | ||
| "error_records": request_accounting.get("records_error_dropped"), |
There was a problem hiding this comment.
records_error_dropped is not profiling-scoped: artifacts.py increments it before phase filtering, and atom.py rejects pricing whenever it is nonzero. A warmup-only error can therefore invalidate a profiling window whose admissions and prompt-token counts conserve exactly. Please track profiling errors separately (or increment after phase classification) and add a regression where warmup errors coexist with a fully valid profiling window.
中文
records_error_dropped 并不只统计 profiling 阶段：artifacts.py 在按阶段过滤之前就会递增它，而 atom.py 只要看到该值非零就会拒绝定价。因此，仅发生在 warmup 阶段的错误，也可能让一个 admission 数和 prompt token 数完全守恒的 profiling 窗口失效。请单独统计 profiling 错误（或在阶段分类后再递增），并添加一个回归测试，覆盖 warmup 有错误但 profiling 窗口完全有效的情况。
There was a problem hiding this comment.
Thanks, confirmed and fixed in 0fda3f1e. The branch head is now 0842d829, which also merges current main.
records_error_droppedkeeps its meaning: failed requests in every phase.load_records_with_accountingalso recordsrecords_profiling_error_dropped, which counts failed requests outside warmup. A record without a phase still counts as profiling, following the existing rule.request_scope_from_recordspasses the profiling count aserror_records, so a warmup-only failure no longer blocks pricing. Accounting without the split falls back to the all-phase total.
Regression: test_only_profiling_errors_from_the_loader_block_pricing writes profile_export.jsonl and runs the producer CLI (loader → request scope → ATOM accounting) for six cases: clean, warmup-only error, profiling error, errors in both phases, and missing phase with and without an error. It fails on 2db01926 and passes on 0842d829. The five points of run 36734492117 keep their cache rates and statuses.
中文
感谢指出，问题已确认并在 0fda3f1e 中修复；分支 head 现为 0842d829，其中还合并了当前的 main。
records_error_dropped保持原有含义，统计所有阶段的失败请求。load_records_with_accounting另外记录records_profiling_error_dropped，统计 warmup 之外的失败请求；缺少 phase 的记录按现有规则仍计为 profiling。request_scope_from_records把 profiling 错误数作为error_records传入，因此仅 warmup 失败不再阻止计价；没有这项拆分的旧记账回退为全阶段总数。
回归测试 test_only_profiling_errors_from_the_loader_block_pricing 写入 profile_export.jsonl 并运行 producer CLI（loader → 请求范围 → ATOM 计数），覆盖六种情况：正常、仅 warmup 错误、profiling 错误、两个阶段都有错误，以及缺少 phase 时有错误和无错误。它在 2db01926 上失败，在 0842d829 上通过。run 36734492117 的五个点缓存命中率和状态均不变。
Brings in 22 upstream commits up to 5107ebe. - backends/atom.py: upstream only removed a comment inside the old cache-rate block that this branch replaces; this branch's accounting is kept. - tests/results/agentic/test_process_agentic_result.py: upstream's comment removal is kept. - server_metrics.py and tests/bench/test_agentic_command.py merged cleanly. 合并 origin/main 的 22 个上游提交（至 5107ebe）。atom.py 中上游只删除了本分支已替换的旧缓存命中率代码块里的一条注释，保留本分支的计数逻辑；test_process_agentic_result.py 保留上游删除的注释；server_metrics.py 与 tests/bench/test_agentic_command.py 自动合并。
records_error_dropped counts failed requests in every phase, and the ATOM request-scope check refused pricing whenever it was nonzero. A failed warmup request therefore invalidated a profiling window whose admissions and prompt tokens conserved exactly. - load_records_with_accounting adds records_profiling_error_dropped for errors outside warmup; records without a phase still count as profiling. records_error_dropped stays the all-phase total. - request_scope_from_records passes the profiling count as error_records and falls back to the all-phase total for accounting without the split. - A processor-entry regression covers clean, warmup-only, profiling, both-phase and legacy missing-phase errors. Review: #3964 (andyluo7) records_error_dropped 统计所有阶段的失败请求，而 ATOM 请求范围检查只要它非零就拒绝计价，因此一次 warmup 失败也会让请求数和 prompt token 完全守恒的 profiling 窗口失去计价资格。load_records_with_accounting 新增 records_profiling_error_dropped，只统计 warmup 之外的错误，缺少 phase 的记录仍按 profiling 计；records_error_dropped 保持全阶段总数。request_scope_from_records 以 profiling 错误数作为 error_records，没有该字段的旧记账回退为全阶段总数。新增经处理器入口的回归测试，覆盖正常、仅 warmup 错误、profiling 错误、两个阶段都有错误，以及缺少 phase 的旧记录。
Drops a comment that restated the control flow and a descriptive block above a test fixture, and shortens the decode re-count note to one line. Comment-only. 按 AGENTS.md 的注释规则精简本 PR 自己的注释：删除一条复述控制流的注释和一段测试夹具上方的描述性注释，并把 decode 重复计数的说明压缩为一行。仅改注释。
… (cherry-pick #3964) Carry #3964 (2db0192, 0fda3f1, 0842d82) so this PR's sweep prices the ATOM P/D cache rates with it: count only prefill endpoints, take the CPU rate from atom:prefix_cache_offload_tokens (reported for LMCache and the Mooncake Store alike), and slice counters to the profiling window. Applied as #3964's net diff onto current main; drop it once #3964 merges. 携带 #3964（2db01926、0fda3f1e、0842d829），使本 PR 的 sweep 用它来计算 ATOM P/D 的缓存命中率：只统计 prefill 端点，CPU 命中率取自 atom:prefix_cache_offload_tokens（LMCache 与 Mooncake Store 都会上报）， 并按 profiling 窗口切分计数。以 #3964 相对其基线的净改动应用到当前 main 上； #3964 合入后即可移除。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re-review update for head 0842d82: The producer-side review finding is addressed. Commit 0fda3f1 correctly separates profiling errors from warmup errors, so a warmup-only failure no longer invalidates an otherwise conserved profiling window. Profiling errors and legacy missing-phase errors still block pricing. I found no new producer-side code findings. The focused suites passed at the exact head and on a clean merge with current main; exact-head Tests and Lint are also green. I am keeping the merge blocked because downstream containment has not landed. SemiAnalysisAI/InferenceX-app#1342 was closed unmerged at the same reviewed head, and the app still does not consume server_metrics.cache_validity. Run-specific purges and the theoretical default do not protect Actual mode from invalid new observations. Before merging this PR, please land and deploy an app-side ETL or consumer guard that:
After that guard is deployed, rebase this PR on current main, rerun CI, and I can complete the approval. 中文对提交 0842d82 的复审结论： producer 侧原 review 问题已经解决。0fda3f1e 正确区分 profiling 错误和 warmup 错误，因此仅发生在 warmup 阶段的失败不再让一个请求数和 token 数守恒的 profiling 窗口失效；profiling 错误以及旧格式中缺少 phase 的错误仍会阻止计价。我没有发现新的 producer 侧代码问题。针对性测试在当前 head 以及与最新 main 的干净合并结果上均通过，当前 head 的 Tests 和 Lint 也为绿色。 但下游隔离尚未落地，因此目前仍应阻止合并。SemiAnalysisAI/InferenceX-app#1342 已关闭且未合并，app 仍未消费 server_metrics.cache_validity。按 run 清理和默认使用理论命中率，都不能保护 Actual 模式免受新的无效观测影响。 合并本 PR 前，请先落地并部署 app 侧 ETL 或 consumer guard，要求：
该 guard 部署后，请将本 PR rebase 到最新 main 并重新运行 CI，届时我可以完成 approval。 |
Cross-vendor fairness audit of cache-aware pricing (head
|
| Issue | Fixed? | Notes |
|---|---|---|
| ATOM P/D hit rate under-reported by about half | ✅ | Denominator now counts prefill/unified only; CPU comes from offload; overall = GPU + CPU |
| ATOM warmup included in the rate | ✅ (ATOM only) | |
| Warmup included for vLLM / SGLang / TRT-LLM / dynamo | ❌ | Becomes a new cross-vendor asymmetry once only ATOM is windowed (§3.1) |
Decode-worker inflation for other engines (e.g. vLLM P/D external_kv_transfer) |
❌ | Not addressed (§3.2) |
| Other adapter issues (vLLM fallback double count, TRT-LLM gauge average, SGLang storage tier) | ❌ | Not addressed (§3.3) |
| Non-priceable observations still priced | ATOM only | Only ATOM emits cache_validity. Other engines have no validity signal at all |
| Missing h priced as 0 hits on non-GB300 hardware (B200/B300/GB200/MI355X…) | ❌ | Guard scope is ATOM-family only (§3.4) |
| GB300-only theoretical fallback and GB200 → GB300 donor backfill | ❌ | Unchanged (§3.4) |
| All-or-nothing frontier interpolation (one missing point → whole sweep at 0 hits) | ❌ | Unchanged. After the GB300 fallback, this only hits non-GB300 frontiers |
| Measured-only surfaces (Token Revenue chart, Historical Trends, Fleet Lifecycle, calculator view API) | ATOM only | The theoretical default exists only in the Profit Estimator |
P+D double count in prompt_total (token mix) |
❌ | Explicitly left unchanged |
| Historical / purged runs | ❌ | No re-import. Purged ATOM runs 36734492117 and 35855544139 stay absent |
3. Detailed explanation
3.1 Warmup accounting: confirmed on real exports, and it turns against AMD after this PR
Every adapter except new ATOM reads stats.total through sum_stat/series_stat (common.py:114-145):
vllm.py:44-53sglang.py:40-57trtllm.py:45-59dynamo_vllm.py:67-77
I checked two real single-node ATOM AIPerf exports (agentic, 900 s, c2 and c4). In both, stats.total equals the slice straddling the profiling start plus the in-window slices. For c2, the straddling slice also equals warmup_metrics.total exactly. So stats.total includes warmup. Warmup runs on a cold cache, at about 0.80–0.85 hit rate, which pulls the whole-export rate down:
| run | stats.total (incl. warmup) |
profiling window only | input price factor 1 − 0.9h |
|---|---|---|---|
| c2 | 4,522,240 / 5,091,598 = 0.888 | 2,827,520 / 3,096,562 = 0.913 | 0.2006 → 0.1782 (−11%) |
| c4 | 7,601,152 / 8,446,996 = 0.900 | 4,797,952 / 4,958,435 = 0.968 | 0.1901 → 0.1291 (−32%) |
The other engines use the same AIPerf mechanism, so their rates should carry the same downward bias, which inflates their revenue. After this PR only ATOM is measured on the clean window. The magnitude for the NVIDIA-side engines is not measured here, because I don't have their exports. The direction follows from the mechanism.
Side note: the repo's tests (test_process_agentic_result.py:1321-1364) assume metrics covers profiling only, which does not match these exports. In the c4 export the straddling slice (2,803,200 cached) also does not match warmup_metrics (2,480,384). By this PR's own logic that case is WINDOW_UNRESOLVED, yet a rate is still emitted.
3.2 vLLM P/D: decode workers can inflate external (code-confirmed, scraping unverified)
When a vLLM decode worker pulls KV over NIXL, it counts the whole prompt minus local hits as external_kv_transfer. If decode endpoints are scraped, as the SRT endpoint list does for vllm-disagg, overall ≈ (1 + h) / 2. A true 0.6 would read as 0.8, which under-states revenue. This would affect AMD vllm-disagg and the NVIDIA dynamo-vLLM agentic configs. I did not verify which endpoints the Dynamo frontend path actually scrapes.
3.3 Other adapter issues (plausible, not quantified)
- vLLM fallback. Without
prompt_tokens_by_source, external = hits / queries, where queries are already net of local hits. That is a rate conditional on a local miss. The app then adds it to GPU: 0.6 + 0.5 = 1.1, clamped to 1.0, versus a true 0.8. - TRT-LLM (NVIDIA only). It prefers the
trtllm_kv_cache_hit_rategauge average (trtllm.py:50-57). That is a time average, not token-weighted, and a plain mean across endpoints, so prefill and decode get equal weight. The bias direction is unknown. - SGLang. The storage (L3) tier goes into
overallbut not intogpu/cpu. The app pricesgpu + cpu, so L3 hits are dropped, which inflates revenue.
3.4 App-side asymmetries (code-confirmed)
- GB300-only fallback (
cache-pricing.ts:39,58). A GB300 point without a measurement is priced at the theoretical ceiling (≈0.97). Any other hardware without a measurement is priced at 0 hits. InferenceX-app#980 listed the non-GB300 gaps that stay at 0 hits by design: B200/B300 TRT MiniMax M3 (22/33 rows each), GB200 Kimi K3 (13/44) and one B200 GLM-5.1 row. I did not re-count them against the production DB. - Donor backfill (
run-overrides.ts:475-514, applied at:609-775). Fourteen GB300 points borrow GB200 measured rates by nearest concurrency and display them as GB300's own. No equivalent repair path exists for other hardware. The ATOM runs with missing CPU rates were purged instead (run-overrides.ts:53-54). NVIDIA TRT runs that lost metrics were purged too, so purging itself is even-handed; the repair path is not. - All-or-nothing interpolation (
components/calculator/interpolation.ts:456-458,interpolated-trend-core.ts:241-247). One missing point disables h for the whole frontier, so the whole sweep bills at full input price. After the GB300 fallback, only non-GB300 frontiers can hit this. theoreticalis the default only in the Profit Estimator (profit-estimator.ts:184). The Token Revenue chart, Historical Trends, Fleet Lifecycle and the calculator view API all price measured h, so every item above shows there.theoreticalis not perfectly uniform either: it comes from each run's own request sample, and rows without it keep the measured value.
3.5 New-ATOM refusals price in AMD's favour
When roles, window or counters are unresolved, account_cache returns null for every rate (atom.py:141-249). On MI355X that prices as 0 hits, which over-states revenue. A missing offload counter yields GPU-only (atom.py:230-236), which under-counts reuse. The other engines almost never emit null; they emit a possibly biased number instead. Rates for non-eligible statuses (WINDOW_UNRESOLVED, REQUEST_SCOPE_UNRESOLVED) are still emitted (atom.py:251-253), and nothing downstream reads pricing_eligible.
3.6 Engine-to-hardware mapping makes these vendor-skewed
From the agentic configs:
- ATOM / atom-disagg run only on MI355X;
- TRT-LLM and every
dynamo-*run only on NVIDIA (B200/B300/GB200/GB300/H200); - vLLM and SGLang run on both.
So §3.1 and §3.5 split along vendor lines, and §3.4 applies only to NVIDIA hardware.
3.7 Scope of the proposed guard (InferenceX-app#1342)
That guard applied only when framework.startsWith('atom'). It required complete GPU + CPU/external components and nulled revenue for failing rows. ATOM would then be held to a stricter rule than NVIDIA rows, which keep 0-hit pricing on missing data and the GB300 fallback. A guard keyed on producer-emitted validity, applied to every producer, would avoid that.
Evidence levels.
- Confirmed from code or real exports: §3.1 warmup inclusion and the ATOM magnitudes; §3.4; §3.5; §3.6.
- Code-derived but not reproduced on data: §3.2, §3.3.
- Not measured: the NVIDIA-side warmup magnitude and the current production counts of rows without measurements.
|
Thanks @valarLip for the cross-vendor audit and @andyluo7 for the re-review. Here is what we found on the NVIDIA side, with evidence, and the order we propose. 1. Warmup inside 2. Adapter issues on the NVIDIA side (real exports).
We have local patches with tests for these: a counter-based TRT rate that is published only as interval telemetry and carries validity, plus the vLLM, Dynamo and SGLang items. They are not published yet. One caveat: the vLLM role fix alone moves the GB200 dynamo-vllm row from about +0.013 to about −0.014 until the warmup fix is in, so it should ship together with or after item 1. 3. One validity contract and one app rule. 4. Proposed order.
This PR stays in Draft until step 3 is in. The pricing-policy choices (uniform theoretical fallback or unpriced, donor data, whether interval telemetry can price in Actual mode) are for the maintainers and SemiAnalysis to make. Drafted with Claude Opus 5.5 (Cursor agent) and reviewed by Chun Fang. 中文 感谢 @valarLip 的跨厂商审计和 @andyluo7 的复审。以下是我们在 NVIDIA 侧的发现、证据和建议的顺序。
由 Claude Opus 5.5（Cursor agent）起草，Chun Fang 审阅。 |
|
Generally most InferenceX submissions don't need to add changes to the infx package. On the off chance they do, generally the diff is small. Please prompt your agent to clean your PR to keep your diff to only necessary & minimal changes too. We have an increased code quality bar for changes to the infx package. 中文大多数 InferenceX 提交通常不需要修改 infx 包。即使确实需要，改动通常也很小。请让你的 agent 清理 PR，使改动只包含必要且最小的内容。infx 包的改动有更高的代码质量要求。 |
|
Ready for review: scope, current evidence and known limits This is the canonical evidence comment for this PR; later results will update it rather than add new comments. Chun has approved moving code review forward now; the merge and activation gates remain separate. This replaces our earlier statement on this PR that it would stay in Draft until the app guard is in. @andyluo7's condition is unchanged: the merge waits for the app-side guard (SemiAnalysisAI/InferenceX-app#1377) to land and deploy. Completing the wider model sweep is not a prerequisite for reviewing this change. Code identity. Head Reviewed scope. ATOM result adapter only:
New evidence: live paired runs on MI355X, ATOM, single node. 12 complete pairs: Kimi-K3 4, DeepSeek-V4-Pro 3, GLM-5.2 2, MiniMax-M3 2 and DeepSeek-V4.1-Flash 1, at concurrency 10 to 16. Each pair runs one frozen serving recipe twice: H0 with AgentX 1.0.11, H1 with 1.0.11 plus SemiAnalysisAI/agentx-harness#49. That fix is now released in 1.0.12 and pinned on
Reading.
Limits.
Review request. Please review the ATOM role, window and CPU-tier logic and the validity statuses, in particular how the start gap with the #49 collector should be handled. Deployment is separate. This comment does not request a production import, package release, default-policy switch or historical backfill. Prepared by Claude Opus 5.5 (Cursor agent) for Chun Fang, who approved publishing verified results; the values are computed from the immutable per-run summaries. 中文进入 review：范围、当前证据和已知限制 这是本 PR 的主证据评论，后续结果会更新这条评论，不再另发新评论。 Chun 已批准现在推进代码 review；合并和上线的门槛保持不变。这取代了我们之前在本 PR 上“app 侧 guard 落地前保持 Draft”的说法。@andyluo7 提出的条件不变：合并要等 app 侧 guard（SemiAnalysisAI/InferenceX-app#1377）落地并部署。完成全部模型的测试不是 review 本改动的前提。 代码身份。 head review 范围。 只涉及 ATOM 结果适配器：
新证据：MI355X、ATOM、单节点上的实测成对运行。 共 12 个完整成对：Kimi-K3 4 个、DeepSeek-V4-Pro 3 个、GLM-5.2 和 MiniMax-M3 各 2 个、DeepSeek-V4.1-Flash 1 个，并发 10 到 16。每对用同一个冻结的服务配方跑两次：H0 用 AgentX 1.0.11，H1 用 1.0.11 加 SemiAnalysisAI/agentx-harness#49。该修复现已随 1.0.12 发布，并由 #4318 锁定在
解读。
限制。
review 请求。 请 review ATOM 的角色、窗口和 CPU 层逻辑以及有效性状态，特别是新采集器带来的起始空档应如何处理。 部署另行处理。 本评论不请求生产导入、包发布、默认策略切换或历史回填。 由 Claude Opus 5.5（Cursor agent）为 Chun Fang 准备，Chun 已批准发布经核实的结果；数值根据不可变的逐次运行汇总计算。 |
|
Claude finished @chunfangamd's task in 1m 41s —— View job Review of head
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| gap = ( | ||
| not covering | ||
| or covering[0][0] > start | ||
| or covering[-1][1] < end | ||
| or any(covering[i][0] != covering[i - 1][1] for i in range(1, len(covering))) | ||
| ) |
There was a problem hiding this comment.
🔴 BLOCKING: Because of the covering[0][0] > start check, every new ATOM run that uses the collector pinned on this branch (infx/bench/agentic/requirements.txt → agentx-harness==1.0.12, i.e. #49) gets no cache rate.
Why it matters: With 1.0.12 the first slice of each counter starts slightly after profiling.start_ns (about 0.1 s, per your evidence comment). That always reads as a sampling gap, so account_cache returns WINDOW_UNRESOLVED at line 208, before any rate is computed. Your own 12 H1 pairs are all 12/12 null. main's adapter is within 0–0.011 of the client reference on those same runs. So on merge, single-node ATOM goes from an accurate rate to null, and the app currently prices null as 0 hits on MI355X. This is a regression for new runs, separate from the app-guard gate. It is not only a limitation to handle in a follow-up.
Fix: Split the leading gap from the interior and trailing gaps. Don't refuse on a leading gap. Collect it, still compute the rates, and let request conservation decide eligibility. If counter increments fall before the first scrape, the requests/full vs client comparison already catches them:
parts["leading_gap"] = bool(covering) and covering[0][0] > start
gap = (
not covering
or covering[-1][1] < end
or any(covering[i][0] != covering[i - 1][1] for i in range(1, len(covering)))
)In account_cache, gather leading entries for FULL/CACHED/OFFLOAD instead of returning. After conserved is computed, run if leading and not conserved: boundary.extend(...). This is roughly the local candidate you described. I'd put it in this PR rather than a follow-up, because the pin it targets is already on the branch.
| slices = _slices(series[stem][url]) | ||
| if slices is None: | ||
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: timeslices missing or invalid") | ||
| parts = _window_parts(slices, start, end) | ||
| if parts["gap"]: | ||
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: sampling gap in the window") |
There was a problem hiding this comment.
🟡 WARNING: lmcache_loaded_tokens is documented as diagnostic only (docstring at line 117), but invalid or gappy timeslices on it still go through these return refuse(...) paths. That nulls GPU, CPU and overall for the whole point.
Why it matters: A labeled or lazily created LMCache counter can first appear mid-window (first load after profiling start), or have a missed scrape. Either way its first slice no longer covers start, so a counter that never feeds a rate vetoes a fully valid observation.
Fix: For LOADED, degrade to None instead of refusing:
| slices = _slices(series[stem][url]) | |
| if slices is None: | |
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: timeslices missing or invalid") | |
| parts = _window_parts(slices, start, end) | |
| if parts["gap"]: | |
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: sampling gap in the window") | |
| slices = _slices(series[stem][url]) | |
| parts = _window_parts(slices, start, end) if slices is not None else None | |
| if stem == LOADED and (parts is None or parts["gap"]): | |
| inside = None | |
| break | |
| if slices is None: | |
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: timeslices missing or invalid") | |
| if parts["gap"]: | |
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: sampling gap in the window") |
(totals[stem] = inside below then stores None for LOADED, which the rest of the function already handles.)
| if not isinstance(url, str): | ||
| continue | ||
| if url in by_endpoint: | ||
| duplicates.append(url) | ||
| by_endpoint[url] = series | ||
| return by_endpoint, duplicates |
There was a problem hiding this comment.
🟡 (optional) Conditional: if an ATOM worker exports more than one labelled series per metrics URL, every ATOM run loses all cache hit rates (null, INVALID_COUNTER) where base summed them. _series_by_endpoint treats any second series with the same endpoint_url as a duplicate alias, even under the same metric name. Fix: flag a duplicate only when one endpoint reports the same counter under two alias names. Series sharing one name and URL but differing in labels should be summed per endpoint, timeslice by timeslice, while the alias-duplicate refusal stays.
Why this was flagged
I could not rule out from this checkout that ATOM exports several series per endpoint for one counter, for example per DP rank or engine label. The loop at atom.py:47-52 runs if url in by_endpoint: duplicates.append(url) for every series in entry["series"], so two series of atom:prefix_cache_full_tokens from the same worker count as a duplicate. account_cache then returns early with refuse("INVALID_COUNTER", "duplicate ... series") (atom.py ~line 145), which sets gpu, cpu, external and overall to None. The same happens for the diagnostic lmcache_loaded_tokens and prefix_cache_requests. On the base branch, sum_stat added up every series of each name, so these runs still published cache hit rates. The new test at test_atom_cache_accounting.py:317 only covers the cross-alias case (atom: plus atom_..._total), so nothing guards the same-name multi-series case.
Verification: _series_by_endpoint (atom.py:35-52) keys only on endpoint_url and ignores labels, so two series under atom:prefix_cache_full_tokens from one URL are flagged as duplicates. account_cache then returns refuse("INVALID_COUNTER", ...), and AtomBackend writes null rates. On the base branch, sum_stat (common.py:128-145) summed every series. No test covers same-name series with different labels.
| totals: dict[str, float | None] = {} | ||
| boundary: list[str] = [] | ||
| for stem in _COUNTERS: | ||
| present = [u for u in accounted if u in series[stem]] | ||
| if not present: | ||
| totals[stem] = None | ||
| continue | ||
| if len(present) != len(accounted): | ||
| totals[stem] = None | ||
| continue | ||
| inside = 0.0 | ||
| for url in present: | ||
| slices = _slices(series[stem][url]) | ||
| if slices is None: | ||
| return refuse("WINDOW_UNRESOLVED", f"{stem}@{url}: timeslices missing or invalid") |
There was a problem hiding this comment.
🟡 (optional) A bad diagnostic-only counter can null every ATOM cache rate. A duplicate, malformed or gapped lmcache_loaded_tokens series makes all four rates null, even when cached/full/offload are clean. The window loop runs refuse("WINDOW_UNRESOLVED", ...) for every stem in _COUNTERS, including LOADED and REQUESTS. The duplicate check at atom.py:148-151 refuses on any stem the same way. Fix: refuse only on FULL, CACHED and OFFLOAD problems. A bad LOADED series should only drop its own diagnostic. A bad REQUESTS series should only make requests None, which already yields REQUEST_SCOPE_UNRESOLVED.
Why this was flagged
The duplicate scan at atom.py:148-151 runs over all of _COUNTERS, including lmcache_loaded_tokens and prefix_cache_requests. A second alias series on one endpoint returns refuse("INVALID_COUNTER"). In the window loop at atom.py:191-205, a series with missing or invalid timeslices, or a sampling gap, returns refuse("WINDOW_UNRESOLVED") before any rate is computed. The loop treats LOADED and REQUESTS like FULL/CACHED/OFFLOAD here. It only skips them later, for the straddle checks at atom.py:207. For a P/D or unified ATOM run where only that counter is affected, server_gpu_cache_hit_rate, server_cpu_cache_hit_rate and server_overall_cache_hit_rate all come out None, and so does every nested["cache"] rate. On the base branch the GPU rate was still reported in this case. The docstring calls lmcache_loaded_tokens "transfer volume" that "stays diagnostic only", yet it can veto the rates. A bad requests counter should only fail the conservation check (status REQUEST_SCOPE_UNRESOLVED). Instead it wipes the telemetry rates too.
Verification: if stem in (LOADED, REQUESTS): continue at atom.py:210 runs only after the INVALID_COUNTER and WINDOW_UNRESOLVED refusals, so it skips only the straddle checks. refuse returns result with gpu, cpu and overall still None (lines 121-123). The old code computed cache_hit_rate from cumulative sum_stat with no timeslice dependence, so this way of losing all rates is new in this change.
functionstackx
left a comment
functionstackx
left a comment
There was a problem hiding this comment.
this PR changes over 1.4k LoC, & has a bunch of AI slop comments & AI slop PR description
can u create an new set of PRs in an PR stack with the first PR being the new that fixes AMD Mooncake ATOMesh kvcache counter and then continue creating PRs to stack on top on that while ensuring that diff r kept high quality & self contained & miminal
similar to how after @billishyahao & @cquil11 and I reviewed YukioZzz k3 disagg PR last week, was able to get the PR LoC decreased by 10x into an high quality self contained diff
understood. We need to eliminate any non-critical AI generated comments |
|
thanks @billishyahao to clarify, not just comments, but also LoC, ideally start an github stack with each PR being self contained |
stacked PR series are also more friendly to reviewer |
|
The ATOM replacement is split into two focused PRs:
We removed historical window recovery, warmup subtraction and expanded transfer diagnostics. They are unnecessary for the current counter repair and qualification. Legacy telemetry remains visible but is no longer eligible for measured pricing. Consequently, these two PRs replace the current-data responsibilities of #3964; they do not preserve every historical-recovery feature. Earlier legacy-parity comparisons applied to earlier heads and are superseded by this narrower contract. #4163 builds on #4405 for modern TRT counter pairs and P/D totals. Its generic phase classifier has been removed, and TRT remains interval telemetry with no exact-pricing eligibility. #4164 retains the vLLM/Dynamo/SGLang corrections above it. No additional PR was created for this simplification. Verification on the current source commits:
The final frozen CPU run passed all 12 phases, including 19 actual producer CLI replays. The first attempt had passed #4405 checks but stopped at a historical harness environment conflict; correcting the harness left the product source unchanged. Failed evidence was retained. The official collector recording uses coordinated simulated HTTP services; it does not prove real ATOM or TRT serving. app #1377 is a separate consumer/deployment dependency, with its default policy still legacy. Consumer protection and the agreed policy activation remain prerequisites to merge/deployment. This PR is closed as superseded by #4397 and #4405. Review and the default/measured P90 verification continue on the replacement stack; consumer-protection and deployment gates remain in force. 中文替代分为两层：#4397 修 counter，排除 decode 重复分母、使用 CPU offload，并令 overall=GPU＋CPU；#4405 证明现代 AgentX 1.0.12 profiling 观测与成功请求/token 范围一致，并提供 app 消费的最小资格 core。 本轮删除了当前目标不需要的历史窗口恢复、warmup 扣减和扩展传输诊断。历史 telemetry 保留，但不再获得实测计价资格。因此两层替代的是 #3964 的当前数据修复职责，不宣称保留全部历史恢复功能；以前旧新资格一致的验证属于以前的 head，不能沿用于本轮收窄后的合同。 #4163 继续叠在 #4405 上，仅保留现代 TRT 配对计数和 P/D 总量，删除通用 phase classifier，TRT 仍不可精确实测计价。#4164 保留 vLLM/Dynamo/SGLang 修复。本轮没有新增 PR。模拟 collector 录制不能代替真实 serving；app #1377 的保护与约定策略启用仍是 merge/deployment 前提。#3964 已由 #4397 和 #4405 取代并关闭；后续审阅及 default/measured P90 验证继续在新 stack 上推进，消费端保护与部署门槛不变。 |
Summary
ATOM prefill/decode (P/D) agentic results under-reported prefix-cache hit rates by about half. The ATOM result adapter (
inferencex-e2e/infx/results/agentic/backends/atom.py) had four problems:prefix_cache_full_tokensover prefill and decode endpoints. The decode worker re-counts each prompt on first admission without serving any reuse, so the denominator doubled.lmcache_loaded_tokens(LMCache transfer volume) as CPU reuse. ATOM records CPU reuse inprefix_cache_offload_tokens, which is disjoint from the HBMprefix_cache_cached_tokens.overallequal to GPU instead of GPU + CPU.stats.total, which for this export also spans the warmup phase.Found on run 36734492117 (see #3543; original recipe #3158).
Review follow-up. andyluo7 found that a failed warmup request blocked pricing of an otherwise valid profiling window. This is fixed in
0fda3f1e(see Errors below). The branch also merges currentmain(f3bf3fb1), and0842d829aligns this PR's own comments with the AGENTS.md comment rules (comment-only).Review status (2026-10-20). Ready for review. The merge still waits for the app-side guard to land and deploy, as requested in review. The branch merges current
main(63195a06) without a force-push. Live-run evidence and known limits are in the evidence comment.Changes
SRT_PREFILL_ENDPOINTS/SRT_DECODE_ENDPOINTS, orSRT_AGG_ENDPOINTS). The mapping mirrorsinfx.bench.agentic.replay._server_metrics_urls. Only prefill (or unified) endpoints are counted. Without roles, the unified fallback applies only to a declared non-disagg single endpoint.summary.phase_time_ranges.profiling. There is nostats.totalfallback for priced rates. A slice straddling the window edge must be proven warmup-only bywarmup_metrics; sampling gaps, invalid slices and duplicate aliases are refused.externalis an alias of CPU (ATOM LocalCPUBackend).lmcache_loaded_tokensstays diagnostic only.full > 0,0 <= cached <= full,offload >= 0,cached + offload <= full(all finite). There is no clamping.server_metrics.cache_validity(nullfor non-ATOM adapters). It carriesstatus,pricing_eligible, observed/requested scope, CPU component, roles source, window and request-scope evidence, and reasons.KV_OFFLOADING=none.usage_prompt_tokens, no profiling errors or cancellations); otherwise the rates are telemetry only (REQUEST_SCOPE_UNRESOLVED).load_records_with_accountingaddsrecords_profiling_error_dropped, counting errors outside warmup. A record without a phase still counts as profiling.records_error_droppedstays the all-phase diagnostic.tokens.prompt_totalkeeps its meaning (it still sums P and D prompt counters). Non-cache fields and other adapters are unchanged.test_process_agentic_result.pyalso carries formatter-only (ruff format) changes besides the schema-key update.Validation (head
0842d829685bd5d797588e4f82fe8be129d014aa)CI on the new head. CI run 38522214656 tested the merge commit
afecbac5(0842d829+main7b28963e):Regression for the review finding.
test_only_profiling_errors_from_the_loader_block_pricingwritesprofile_export.jsonland runs the producer CLI (loader → request scope → ATOM accounting). It covers six cases: clean, warmup-only error, profiling error, errors in both phases, and missing phase with and without an error. It fails on2db01926and passes on0842d829.Real producer CLI on the five points of run 36734492117. Unpatched
mainreproduces every published cache field. The patch matches an independent window oracle:VALID_COMPLETEVALID_COMPLETEWINDOW_UNRESOLVEDREQUEST_SCOPE_UNRESOLVEDREQUEST_SCOPE_UNRESOLVEDRe-run on
0842d829, the rates and statuses are unchanged. The only output change is the addedrequest_accounting.records_profiling_error_dropped, which is 0 for these runs.Other new tests. A new adapter test module, plus a launcher →
run.py→ producer CLI test proving that role env reaches the producer.On
73c40bdf(merge of currentmain), Tests and Lint passed.Local checks
test_eval_command.pyexcluded):0842d829: 11 failed / 455 passed;main5107ebea: 11 failed / 408 passed.ModuleNotFoundError: yamlin a subprocess interpreter, and 1 is a permission error writing/nonexistent/logs.infx.Not in this PR
Downstream status
mainpins (fix(agentx): bump harness to 1.0.12 #4318). With it, this PR returns WINDOW_UNRESOLVED, with no rate, for every ATOM run we measured, because of a short start gap before the first in-window timeslice. See the evidence comment.cache_validity; the requested changes are being addressed. This PR merges only after that guard lands and deploys.cache_validityyet. The app's default theoretical cache-hit mode (Default profit estimators to cache roofline / 利润估算器默认使用理论缓存命中率 InferenceX-app#1352) and its run purges do not repair measured rates.AI model disclosure
Claude Opus 5.5 (Anthropic), running as a Cursor IDE agent, investigated the issue, wrote the patch, the review follow-up and the tests, and drafted this description. Chun Fang reviewed each step. Independent reviews of the patch and of the follow-up were done with ChatGPT; its exact model version could not be verified. The 2026-10-20 status update and the live-run evidence comment were prepared by Claude Opus 5.5 (Cursor agent; its delegated agents for data reduction were also Claude Opus 5.5) for Chun Fang, who approved publishing verified results.
中文
修正 ATOM P/D agentic 结果中被低报约一半的前缀缓存命中率：
prefix_cache_offload_tokens，overall = GPU + CPU；server_metrics.cache_validity，标明这组值能否作为完整的实际计价输入；不完整或范围不明的数据只作遥测。根据 andyluo7 的 review，
0fda3f1e让请求范围检查只统计 profiling 阶段的失败请求。新增records_profiling_error_dropped，records_error_dropped保持全阶段诊断含义；新增的回归测试经 producer CLI 覆盖六种情况，在2db01926上失败、在0842d829上通过。分支已合并当前main。新 head 的 CI（测试合并提交
afecbac5）中 Tests 为 2435 passed、1 skipped，Lint 通过。run 36734492117 五个点的命中率和状态不变。本地相关套件有 11 项与main相同的环境失败。2026-10-20 更新： 本 PR 已进入 review；合并仍按 review 要求等 app 侧 guard 落地并部署。分支已通过合并当前
main（63195a06）保持同步，没有 force-push，73c40bdf上 Tests 和 Lint 通过。SemiAnalysisAI/agentx-harness#49 已合并并随 AgentX 1.0.12 发布，main已锁定该版本（#4318）；用该采集器时，本 PR 对我们实测的所有 ATOM 运行都返回 WINDOW_UNRESOLVED、不给命中率，原因是窗口内第一个 timeslice 之前有一段很短的起始空档，详见证据评论。SemiAnalysisAI/InferenceX-app#1377 读取cache_validity，正在处理 review 提出的修改。不包含历史回填，也没有修改计价默认值。InferenceX-app#1342 已被维护者关闭、未合并；下游如何隔离无效的新输出正在与维护者对齐，在商定并验证之前，新 producer 数据不应按实测缓存率计价。