Skip to content

docs(patches): give every patch a schema-checked upstream status record - #95

Draft
dorado269 wants to merge 2 commits into
mainfrom
dorado/dev/patch-upstream-status-records
Draft

docs(patches): give every patch a schema-checked upstream status record#95
dorado269 wants to merge 2 commits into
mainfrom
dorado/dev/patch-upstream-status-records

Conversation

@dorado269

Copy link
Copy Markdown
Collaborator

Why

deploy/docker/patch.upstream.status.md was a single hand-maintained table. It could not answer the question that actually decides whether a patch can be deleted — does this patch change bytes in this engine, on this pinned base — and it had no gate, so a patch could be added without a row. patch_mooncake_mamba_unpack.py went unindexed from 2026-08-02 to 2026-08-05.

What

Every patch under deploy/docker/patches/ now has a machine-checked record beside it.

file role
<patch-prefix>.upstream.status.yaml one record per patch (25 of them)
deploy/docker/patch.upstream.status.yaml index, grouped by the library patched
patches/archived/patch.archived.yaml retired patches, one list (6 entries)
patches/_schema/*.json three JSON schemas
scripts/validate-patch-status.py validates + cross-checks against the tree

Validation is strict: additionalProperties: false throughout, dates pattern-checked, commits [0-9a-f]{7,40}, digests ^sha256:[0-9a-f]{64}$, engines and libraries enumerated. Beyond schema shape, the validator also checks that every patch on disk has a record, that the index totals match what is there, and that an archived entry claiming deleted names a commit it can be recovered from — so the set cannot silently drift from the tree.

Wired into .pre-commit-config.yaml, which CI already runs with --all-files. tests/unit/test_patch_status_records.py has 8 tests, 6 of which check the validator actually refuses bad input.

Beyond the fields the old table had, each record carries per-engine effect (op / no-op / not-applied / unverified) with its evidence, an enumerated alive_because.reason, and drop_signal — how you will find out the patch is dead.

Four things writing this down surfaced

  • Four patches can only be marked unverified. Whether patch_vllm_mooncake_blocksize.py, patch_moriio_write.py and the DSv4 MoRIIO patches are load-bearing depends on what the pinned base already carries, and nobody has checked. Each is one grep inside the image.
  • HIPFILE_GIT_REF is unpinned. build_hipfile.sh clones HEAD of ROCm/rocm-systems, so two images built from the same infera commit either side of rocm-systems#7386 (merged 2026-07-16) contain different hipfile code. A reproducibility problem independent of any patch.
  • One patch does not drop when its merged PR lands. patch_hicache_rocm_staged_write_back.py's MERGED PR (sglang#28534) is what introduced the defect; it drops on the open #30350. Its own drop check is a precondition test, not an anchor test, because #30350 never touches our anchor.
  • Six patches cannot announce their own obsolescence — they keep applying cleanly after upstream fixes the problem, so only the pinned version tells you. Now enumerable by drop_signal.

Review notes

  • Nothing was deleted. patches/vllm-dsv4/legacy/patches/archived/vllm-dsv4/ and the superseded .md is kept as patches/archived/patch.upstream.status.superseded.md, both via git mv so history follows. Delete the superseded .md here if you would rather it went.
  • Image behaviour is unchanged. Every Dockerfile patch loop globs *.py or patch_*.py, so no .upstream.status.yaml can be executed. Nothing copies patches/ wholesale, so _schema/ and archived/ never enter an image. The archived legacy patches are no longer copied into the vLLM image at all.
  • Two fields are deliberately nullable, because forcing a value would mean inventing one: image digests (only Dockerfile.vllm pins one — the rest record digest_source: unresolved) and PR authors.
  • The records absorbed the old table's prose and I compressed some of it. If any patch's argument reads thinner than before, patches/archived/patch.upstream.status.superseded.md is in this branch to diff against.

Verification

$ python3 scripts/validate-patch-status.py
patch status OK — 25 record(s), 25 indexed, 6 archived

$ pytest tests/unit/test_patch_status_records.py -q
8 passed

$ ruff format --check && ruff check   # both clean

No image was built, so the four unverified rows stay unverified — they are recorded as such rather than guessed at.

@dorado269
dorado269 marked this pull request as draft August 6, 2026 06:54
@dorado269 dorado269 changed the title docs(patches): give every patch a schema-checked upstream status record [wip]docs(patches): give every patch a schema-checked upstream status record Aug 6, 2026
@dorado269
dorado269 force-pushed the dorado/dev/patch-upstream-status-records branch from f115bbc to cf92b9e Compare August 7, 2026 07:58
@dorado269
dorado269 marked this pull request as ready for review August 7, 2026 07:59
@dorado269 dorado269 changed the title [wip]docs(patches): give every patch a schema-checked upstream status record docs(patches): give every patch a schema-checked upstream status record Aug 7, 2026
The single patch.upstream.status.md table could not answer the question that
decides whether a patch can go: does it change bytes in THIS engine, on THIS
pinned base. It also had no gate, so a patch could be added without a row --
patch_mooncake_mamba_unpack.py went unindexed from 2026-08-02 to 2026-08-05.

Replace it with one <patch-prefix>.upstream.status.yaml beside each patch (25
records), a patch.upstream.status.yaml index grouped by the library patched, and
a single patch.archived.yaml for retired patches. Three JSON schemas validate
them; scripts/validate-patch-status.py also cross-checks the set against the
tree, so a patch without a record, an index whose totals drift, or an archived
entry that cannot be recovered fails the lint gate.

Each record carries the fields the old table lacked: per-engine op/no-op with
its evidence, an enumerated reason the patch is still alive, and drop_signal --
how you find out it is dead. Writing that down surfaced four things:

- Four patches can only be marked `unverified`: whether they are load-bearing
  depends on what the pinned base already carries and nobody has checked.
- HIPFILE_GIT_REF is unpinned, so two images from the same infera commit built
  either side of rocm-systems#7386 (2026-07-16) hold different hipfile code.
- patch_hicache_rocm_staged_write_back.py does not drop when its MERGED PR
  lands -- sglang#28534 is what INTRODUCED the defect; it drops on #30350.
- Six patches keep applying cleanly after upstream fixes them, so only the
  pinned version says they are dead; drop_signal now enumerates which.

Nothing is deleted: the legacy vllm-dsv4 patches move to patches/archived/ via
git mv so history follows, and the old table is kept as
patch.upstream.status.superseded.md.

Image behaviour is unchanged. Every Dockerfile patch loop globs *.py or
patch_*.py, so no .upstream.status.yaml can be executed, and nothing copies
patches/ wholesale, so _schema/ and archived/ never enter an image.

The gate itself had two holes, both from discovery only running patch -> record:

- extra_files was a global set with no check that the claiming record sits
  anywhere near the file it claims, so one line in any record -- in any
  directory -- marked an unrelated new patch as covered and the gate passed.
  A record may now only name files beside it, which is what the schema's own
  description ("further files that are part of the same patch") already said.
- Nothing walked record -> patch, so a record whose patch was renamed or
  deleted was never loaded, never schema-checked, and never reported. A git mv
  of a patch is the realistic version of this.

Three tests cover those, each confirmed failing against the validator without
the fix.

Finally, yamllint over deploy/docker/**.yaml, which caught eleven values losing
text: an unquoted ` #` opens a YAML comment, so a PR number written mid-sentence
silently ends the value, and the result is still valid YAML that still passes
its schema.

  action:      Get #2725 reviewed; it has had no activity ...  ->  "Get"
  reusable_on: An aiter older than #3033, e.g. the earlier ...  ->  "An aiter older than"

Quoting the eleven restores the text; that diff is quotes only, no wording
changed. --strict, because the truncation is reported as a warning and a hook
that exits 0 on warnings would not have caught it. line-length is 160 rather
than the default 80: these records quote upstream issue titles and engine error
strings verbatim, and those stay byte-exact to stay greppable. Scoped to
deploy/docker/ -- deploy/operator/ is a much larger body of existing k8s YAML
and is left alone for now.

Signed-off-by: yihou <yihou@amd.com>
…M-5.2 patches

work.todo.md inventories all seven sglang GLM-5.2 patches against live upstream and
finds exactly three with no PR of anyone's: the ROCm hicache allocator, the mooncake
early-send wait event, and the DSA decode host-sync deadlock. Those are now
sglang#33968, #33970 and #33973, all drafts. pr.done.md records the outcome.

The three records move from `no-upstream-pr` to `upstream-pr-open`, gain an
upstream_prs entry, and have their open_actions rewritten to say what is still owed.
by_status moves 11/5 -> 8/8.

Adapting each patch onto upstream main corrected the records that described them:

- The hicache fix is NOT a one-line override in the shape of the merged MUSA PR
  #23361, as its header claimed. get_device() returns "cuda" on ROCm, so HIP has no
  key of its own; and memory_pool_host.py keys the table twice with a torch.device
  OBJECT, which is not dict-key-equal to "cuda" (measured: different hash), so those
  two pools always take the defaultdict default. Adding only a key misses them.
- The DSA record credits base_spec_worker for supplying extend_seq_lens_cpu. It is
  eagle_worker_common.prepare_for_draft_extend:105. Also found a better upstream
  argument than the record had: the backend already declares needs_cpu_seq_lens =
  False to opt out of the D2H sync, so its eager fallback contradicts itself.
- The mooncake patch's prefill.py edit also fixes mori, which reads the barrier but
  was never handed one on the overlap path. Neither header said so.

Only the 2a half of the DSA diff went upstream. 2b overlaps sglang#32209 and rests on
a concurrency-32 failure whose cause is unidentified, so that patch outlives its PR --
recorded in the index rather than left to be rediscovered.

Every PR is a draft, and the records say why in the field that decides it: the MI355X
and multi-node clusters were unreachable, so none has been re-run against the original
fault on hardware that reproduces it. This session established equivalence and scope
against the proven local fix on MI300X and nothing more. The index's verification note
warns not to read these rows as "fixed upstream".

One figure was corrected after the fact: `.max().item()` was first measured at 44 ms,
but that was a first-call measurement carrying lazy-init cost. Warmed, it is 0.5-3 ms.
Fixed here and in sglang#33973's body. The claim the PR rests on is unchanged and
qualitative -- the call blocks at all, on a branch only some DP ranks take.

work/ is gitignored: the validation scripts and working log are useful to re-run when
the cluster returns, but they are not repo deliverables. Their conclusions are in the
records and in the two reports.

validate-patch-status.py passes and the edited .diff still applies --fuzz=0 to the
pinned base.

Signed-off-by: yihou <yihou@amd.com>
@dorado269
dorado269 marked this pull request as draft August 7, 2026 10:15
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.

1 participant