Skip to content

fix(kb): make a partitioned card a different machine in the KB key (re-land of #1495) - #1542

Merged
ZhengGong-amd merged 1 commit into
mainfrom
fix/rpoornac/kb-partition-identity-reland
Sep 17, 2026
Merged

ZhengGong-amd merged 1 commit into
mainfrom
fix/rpoornac/kb-partition-identity-reland

Conversation

@rpoornac

Copy link
Copy Markdown
Collaborator

Re-land of #1495 at @ZhengGong-amd's recommendation. #1495 was merged as 31143ab and reverted in #1525 to stabilise the 1.1.1 release, not for a defect: two unrelated PRs were reverted in the same five minutes, #1519's test plan names all three together ("No residue of the three reverted PRs (#1520, #1511, #1495)"), and main's CI was green with the change in it.

The content is the reviewed content. It was restored by reverting the revert onto current main rather than reassembled by hand. Eight of the nine source files are byte-identical to what was merged; parser.py is the exception only because main has since refactored --max-hours into DEFAULT_MAX_HOURS, and relative to current main this branch's only change to that file is still the removal of the --recipe-kb-strict-fingerprint block.

Two things did change, both deliberate:

The gap

The recipe canonical_id is a seven-tuple:

inference:{model}:{hardware}:{framework_name}:{model_type}:{architectures}:{framework_version}:{precision}

Expert parallelism is not a dimension of it, and neither is the compute-partition mode. kb_hardware_slug appended topology only when nodes >= 2, so on a single node it returned the bare GPU type. A run in SPX and a run in CPX produced the same id — and recipe_canonical_id has no partition parameter, so the difference was not merely unrecorded, it was inexpressible.

Nothing downstream caught it either. The warm-start cascade relaxes only conc/isl/osl, and RecipeScope.matches_workload_shape compares a hardcoded (tp, conc, isl, osl). So an exact tier hit at confidence 1.0 could hand the PRELUDE auto-replay a config recorded with eight times the partitions. --warm-replay-min-reproduce-pct is a real backstop and the run does recover, but only after paying for the verify round — and status=drift reports it as a recipe that failed to reproduce rather than as a machine that never matched.

What this changes

The collision is made unrepresentable rather than detectable.

The shape is part of the hardware identity. kb_hardware_slug takes a partition_mode and encodes it, plus expert parallelism, at any node count:

MI300X_cpx                     <- CPX pod
MI300X_dpx                     <- DPX pod
MI300X_ep8                     <- single node, EP=8
MI300X_ws16_tp8_ep8_cpx        <- 2 nodes, EP=8, CPX

The default shape is no shape. SPX and ep <= 1 append nothing, so every key written before this change is byte-identical after it:

MI300X                         <- whole card, dense (the historical key)
MI300X                         <- no mode published at all
MI300X                         <- EP=1 is dense, not a shape

One resolver feeds both sides. resolve_kb_topology returns partition_mode, read from the state's compute_partition["mode"] and falling back to published_shape(), so the KB reader and the writer cannot drift into agreeing only by coincidence.

The fallback tiers carry the shape, so a partitioned pod reaches same-ISA siblings at its own shape and never the whole-card row at any tier:

>>> _hardware_fallback_values("MI300X_cpx")
['mi300x_cpx', 'mi308x_cpx', 'mi325x_cpx']
>>> _hardware_is_compatible("MI300X_cpx", "MI300X")
False

The read-side guard is deleted, along with _shape_mismatch, _GUARDED_SHAPE_KEYS, the strict_shape parameter, and --recipe-kb-strict-fingerprint — a flag declared in the parser and read nowhere.

Why identity, not a read-side check

@ZhengGong-amd blocked the original revision of #1495 on mechanism, and all three defects reproduced:

  1. The comparison read a dict that cannot hold the keys. _with_exact_history builds a fixed ten-key mapping with no tp, ep or partitions, so the check reported a mismatch for dimensions the dict is structurally incapable of carrying: 'tp: row=None pod=8, ep: row=None pod=8, partitions: row=None pod=8'.
  2. Demotion did not deny the replay. Setting warm_tier/warm_conf to seed_only/0.0 leaves the config-donor block reachable, because it is entered on warm_point being truthy. _donor_is_trustworthy compares only conc/isl/osl, so the config replayed anyway — a guard that logged a refusal and handed over the config.
  3. DEFAULT_EP = 1 demoted the whole corpus. Every dense CLI run has state.ep == 1, so workload_shape published ep: 1 on every run and the check reported 'ep: row=None pod=1' against every historical row.

It also closes @meinali-566's point without a separate change: RecipeScope had no way to filter on ep or partitions, and distinct canonical_ids leave nothing to filter.

Blast radius

Against a historical row that recorded no shape at all:

Pod Outcome
dense, no partition env kept — key byte-identical
dense, SPX published kept — byte-identical
dense or MoE with ep=1 kept — ep=1 is not a shape
DPX published not reached (pod key MI300X_dpx)
CPX published not reached (MI300X_cpx)
MoE, EP=8 not reached (MI300X_ep8)

A partitioned pod misses rather than mis-hits. It falls through the normal cascade and, absent a row at its shape, seeds cold — the honest outcome for a corpus that predates the distinction, and it self-heals as sessions republish.

Verification on the new base

  • 264 passed across test_recipe_kb_t0_anchor, test_canonical_id_5tuple, test_multi_node_scripts and test_remote_recipe_v2, including the 14 tests that define the change.
  • Full suite 9 failed, 22614 passed. All nine are the known pre-existing set (test_external_multi_node ×5, test_agentx_repair, test_preflight_auth_override, two load-flaky test_supervisor cases under -n 32); none touch the KB.
  • ruff check and ruff format --check clean.
  • No file overlap with the 23 commits main gained since the revert. KernelForge: add fuzzy KB warm-start fallback across framework versions and GPUs #1512's fuzzy KB warm-start fallback is entirely in src/kernelforge/, a different KB from the inference-optimizer recipe KB this touches.
  • +412/−27 across 10 files: +260 tests, +49 CHANGELOG, +103/−24 business code.

One process note for whoever approves: @meinali-566's sign-off on #1495 was posted as a comment rather than an Approve, so that PR merged with no review on record. Worth using the Approve button this time so the trail exists.

Re-land of #1495, which was merged as 31143ab and reverted in #1525 to
stabilise the 1.1.1 release rather than for any defect: two unrelated
PRs were reverted in the same five minutes, the release PR names all
three together, and main's CI was green with the change in it.

Restored by reverting the revert onto current main, so the content is
the reviewed content. Eight of the nine source files are byte-identical
to what was merged. parser.py differs only because main has since
refactored --max-hours into DEFAULT_MAX_HOURS; relative to current main
this change still only removes the --recipe-kb-strict-fingerprint block.

The changelog entries moved into the new [Unreleased] section that the
1.1.1 cut opened, and the "tracked separately" promise in the Removed
entry now names the issue it refers to, #1507.

Verified on the new base: 264 passed across the four affected suites,
including the 14 tests that define the change, and ruff clean. No file
overlap with the 21 commits main gained, and #1512's fuzzy KB fallback
is in src/kernelforge, a different KB from the recipe KB this touches.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants