Skip to content

Enable KV offloading for multi-node agentic sweeps / 为多节点 Agentic 扫描启用 KV 卸载#2141

Merged
cquil11 merged 1 commit into
mainfrom
codex/multinode-agentic-kv-offload
Jul 9, 2026
Merged

Enable KV offloading for multi-node agentic sweeps / 为多节点 Agentic 扫描启用 KV 卸载#2141
cquil11 merged 1 commit into
mainfrom
codex/multinode-agentic-kv-offload

Conversation

@cquil11

@cquil11 cquil11 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Enable multi-node agentic sweep entries to preserve kv-offloading and kv-offload-backend from their benchmark search-space configuration instead of always forcing kv-offloading: none.

This PR is intentionally limited to the shared matrix infrastructure:

  • read KV-offload mode and backend in both full-sweep and test-config generation;
  • emit those fields in multi-node agentic matrix entries;
  • append an offload suffix only for offloaded runs, preserving historical names for none;
  • allow dram in the multi-node Agentic matrix schema and apply the existing KV-offload field-pair validation;
  • add a focused regression test for dram + hicache propagation.

Motivation

The original AgentX multi-node generator explicitly disabled KV offloading even when the YAML search-space supplied kv-offloading: dram and kv-offload-backend: hicache. As a result, multi-node disaggregated Agentic recipes could declare host KV offloading but the generated matrix silently replaced it with none and discarded the backend.

This separates that general capability from the DeepSeek-V4 MI355X recipe PR.

Validation

  • PYTHONPATH=. pytest -q utils/matrix_logic/test_generate_sweep_configs.py — 96 passed
  • python -m py_compile utils/matrix_logic/generate_sweep_configs.py utils/matrix_logic/validation.py
  • git diff --check

中文说明

本 PR 为多节点 Agentic 扫描补充通用的 KV 卸载配置透传能力。此前,多节点 Agentic 矩阵生成逻辑会无条件写入 kv-offloading: none,即使 YAML 搜索空间已经配置了 kv-offloading: dramkv-offload-backend: hicache,生成结果仍会静默丢弃这些配置。

本 PR 的范围严格限定在共享矩阵基础设施:

  • 在完整扫描和测试配置生成路径中读取 KV 卸载模式与后端;
  • 将这些字段写入多节点 Agentic 矩阵项;
  • 仅在启用卸载时为实验名称追加后缀,保持现有 none 配置的历史命名不变;
  • 在多节点 Agentic 矩阵校验中允许 dram,并复用现有的 KV 卸载字段配对校验;
  • 添加一个针对 dram + hicache 透传行为的回归测试。

这样可以把通用的多节点 Agentic KV 卸载能力与 DeepSeek-V4 MI355X 配方 PR 分离开来。

验证结果:矩阵逻辑测试 96 项全部通过,相关 Python 文件编译检查和 git diff --check 均通过。

@cquil11 cquil11 marked this pull request as ready for review July 9, 2026 23:18
@cquil11 cquil11 requested a review from a team July 9, 2026 23:18
@cquil11 cquil11 merged commit 2993627 into main Jul 9, 2026
7 checks passed
@cquil11 cquil11 deleted the codex/multinode-agentic-kv-offload branch July 9, 2026 23:18
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 3m 32s —— View job


Review: LGTM — no blocking issues found

  • Gather context (PR diff, changed files)
  • Review generate_sweep_configs.py changes
  • Review validation.py changes
  • Review test changes
  • Verify downstream workflow plumbing (run-sweep.yml, benchmark-multinode-tmpl.yml)
  • Line count report for generate_sweep_configs.py (posted inline: 1239 → 1259, +20)
  • Post review summary

The KV-offload propagation is correct and symmetric across the full-sweep and test-config paths: kv-offload-backend is only emitted when set, _validate_kv_offload_fields enforces the pairing (dram requires a backend, none forbids one), the none case preserves historical exp-names, and run-sweep.yml already forwards both fields to the multi-node template. One non-blocking observation for the follow-up recipe PR: multi-node entries carry no total-cpu-dram-gb (the generator hardcodes it to 0 and run-sweep.yml doesn't pass it to benchmark-multinode-tmpl.yml despite the template declaring the input), so a multi-node dram run will see an empty TOTAL_CPU_DRAM_GB and the recipe's launch script must size the host cache itself.

Note: I couldn't execute the test suite in this environment (pytest not in allowed tools), but CI and the PR description report all 96 matrix-logic tests passing, including the new dram + hicache regression test.

spec_decoding = bmk.get(Fields.SPEC_DECODING.value, "none")
kv_offloading = "none"
kv_offload_backend = None
kv_offloading = bmk.get(Fields.KV_OFFLOADING.value, "none")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📊 Line Count Report

  • Total Lines: 1259
  • Base Lines: 1239
  • Change: 📈 +20 lines

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused change with good regression coverage; the inline nit is trivial to work around.

Extended reasoning...

Overview

This PR fixes a silent-dropping behavior in the multi-node agentic matrix generator: previously, both generate_full_sweep and generate_test_config_sweep hardcoded kv-offloading: 'none' and discarded any backend, even when the YAML search-space set kv-offloading: dram + kv-offload-backend: hicache. The change reads those fields from bmk, propagates them into the emitted multi-node agentic entries, appends an offload-name suffix only for non-none runs (preserving historical exp-names), widens the MultiNodeAgenticMatrixEntry schema to accept dram and kv-offload-backend, and reuses the existing _validate_kv_offload_fields pair-check. A focused regression test verifies dram + hicache round-trips through generate_test_config_sweep.

Security risks

None — the change is limited to matrix-configuration schema/validation and generator plumbing. No auth, crypto, permissions, or external input surface is touched.

Level of scrutiny

Low. This is shared benchmark-matrix infrastructure (offline config generation for CI sweeps), not production-critical serving code. The change is small, mechanical (single field pair passed through instead of hardcoded), and mirrors the equivalent single-node path that already existed. Behavior for existing kv-offloading: none recipes is preserved by the conditional suffix and the None-guarded backend emission.

Other factors

  • Test coverage: new test_multinode_agentic_preserves_kv_offload_fields exercises the exact new capability (dram+hicache, exp-name suffix, backend propagation), and the existing 96-test suite reportedly passes.
  • The one bug finding surfaced by the hunting system is a nit about AgenticCodingConfig.validate_dram_offload_capacity requiring dram-utilization for multi-node entries that don't actually consume it. The workaround (add any value in (0, 1]) is trivial and produces functionally correct output, and the finder itself flagged it as nit and suggested it could be addressed in the follow-up DSV4-MI355X recipe PR.
  • Change is intentionally decoupled from the DeepSeek-V4 MI355X recipe PR, keeping blast radius small.

decode: WorkerConfig
conc: list[int]
kv_offloading: Literal["none"] = Field(alias=Fields.KV_OFFLOADING.value)
kv_offloading: Literal["none", "dram"] = Field(alias=Fields.KV_OFFLOADING.value)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Multi-node agentic configs with kv-offloading: dram + kv-offload-backend: hicache (the exact capability this PR enables) are rejected by AgenticCodingConfig.validate_dram_offload_capacity in utils/matrix_logic/validation.py unless the user also sets dram-utilization, but dram-utilization is unused for multi-node entries (both generate_full_sweep and generate_test_config_sweep hardcode total_cpu_dram_gb=0 for multi-node, so agentic_dram_offload_gb — the only consumer of dram-utilization — is never invoked). Users adopting this PR's new feature will hit ValueError: kv-offloading='dram' requires dram-utilization with runner hardware metadata at master-config validation and must add a bogus dram-utilization value that has no effect. Fix: in validate_dram_offload_capacity, gate the dram_utilization requirement on single-node entries only (e.g. skip entries where entry.tp is None).

Extended reasoning...

What the bug is

AgenticCodingConfig.validate_dram_offload_capacity in utils/matrix_logic/validation.py requires dram-utilization to be set whenever any search-space entry has kv-offloading: 'dram', regardless of whether the entry is single-node or multi-node. But for multi-node entries, dram-utilization is completely unused — it never affects the generated matrix.

Why the field is unused for multi-node

The only consumer of dram-utilization is agentic_dram_offload_gb in generate_sweep_configs.py. Both generate_full_sweep and generate_test_config_sweep hardcode:

total_cpu_dram_gb = (
    0
    if is_multinode
    else agentic_dram_offload_gb(agentic_config, bmk, runner, runner_data)
)

So for multi-node, agentic_dram_offload_gb is never called, dram-utilization is never read, and total-cpu-dram-gb is not part of MultiNodeAgenticMatrixEntry at all.

Why the PR's regression test doesn't catch it

The new test test_multinode_agentic_preserves_kv_offload_fields calls generate_test_config_sweep directly with a Python dict, bypassing load_config_files / validate_master_config entirely, so the config never sees the AgenticCodingConfig model.

Step-by-step proof

  1. Author writes a real YAML config using the capability the PR is introducing:
dsv4-agentic-hicache:
  image: sglang-rocm
  model: deepseek-ai/DeepSeek-V4-Pro
  model-prefix: dsv4
  precision: fp4
  framework: sglang-disagg
  runner: cluster:mi355x-amds
  multinode: true
  disagg: true
  scenarios:
    agentic-coding:
      - search-space:
          - conc-list: [16]
            kv-offloading: dram
            kv-offload-backend: hicache
            prefill: { num-worker: 1, tp: 8, ep: 1, dp-attn: false }
            decode:  { num-worker: 1, tp: 8, ep: 1, dp-attn: false }
  1. load_config_files calls validate_master_config, which instantiates MultiNodeMasterConfigEntryMultiNodeScenariosAgenticCodingConfig.

  2. In AgenticCodingConfig.validate_dram_offload_capacity (validation.py ~lines 700–720), the entry has kv_offloading == 'dram' and self.dram_utilization is None → raises ValueError('kv-offloading="dram" requires dram-utilization with runner hardware metadata').

  3. Author adds dram-utilization: 0.80 to make validation pass. The value flows through AgenticCodingConfig and reaches generate_full_sweep / generate_test_config_sweep, but because the entry is multi-node, agentic_dram_offload_gb is skipped and total_cpu_dram_gb is hardcoded to 0. The bogus value has no effect on the generated matrix.

Impact

Real bug on the primary code path this PR is meant to unlock, but the workaround (add any value in (0, 1]) is trivial and produces functionally correct output. Marking as nit so the author can decide whether to fix here or in the follow-up DSV4-MI355X recipe PR.

Suggested fix

In AgenticCodingConfig.validate_dram_offload_capacity, gate the dram_utilization requirement on single-node entries only. AgenticCodingSearchSpaceEntry already distinguishes them: single-node has tp set, multi-node has prefill/decode:

for entry in self.search_space:
    if entry.kv_offloading != "dram":
        continue
    if entry.tp is None:  # multi-node — dram-utilization is unused
        continue
    if self.dram_utilization is None:
        raise ValueError(...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant