Upstream sync 7/N: merge cf567cbc71 attention benchmarks (conflict) - #1100
Merged
mgehre-amd merged 2 commits intoAug 13, 2026
Merged
Conversation
…-project#39336) Signed-off-by: Matthew Bonanni <mbonanni@redhat.com>
Conflict-only step of the upstream catch-up. cf567cb is "[Attention] Improve attention benchmarks: configs and profiling (vllm-project#39336)", which lands in benchmarks/attention_benchmarks/ where the fork carries six commits of its own (skip/intermittent/cooldown flags, gfx1151 TRITON_ATTN tuning, YAML configs for pytest). Two conflicts: common.py: upstream adds a median_time field to BenchmarkResult.to_dict(); the fork nulls every timing field when the run was skipped. Keep the fork's "if not self.skip else None" guard and apply it to median_time too. benchmark.py: both sides add imports at the same spot. Keep both. Also fixes create_skipped_result(), which git merged cleanly but which upstream's change breaks: median_time is a required dataclass field with no default, and that fork-only helper does not pass it, so every skipped benchmark would have raised TypeError. Audited all seven BenchmarkResult call sites in the directory; this was the only one missing it. Signed-off-by: Robert Esclapez Garcia <robert.garcia@amd.com>
This was referenced Aug 13, 2026
roberteg16
marked this pull request as ready for review
August 13, 2026 14:16
|
Why do we have create_skipped_result on the fork? Do we still need it? |
Author
We need it as long as long we kept our feature of golden reference testing |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Seventh step of the batched upstream catch-up. Stacked on #1098 (base
rogarcia.merge-upstream-06); retarget togfx11once #1097 and #1098 land.Conflict-only step: exactly one upstream commit.
cf567cbc71"[Attention] Improve attention benchmarks: configs and profiling (vllm-project#39336)"mainIt lands squarely in
benchmarks/attention_benchmarks/, where the fork carries six commits of its own (skip/intermittent/cooldown flags, gfx1151 TRITON_ATTN tuning, YAML configs for pytest).Conflicts and resolutions
common.py— upstream adds amedian_timefield toBenchmarkResult.to_dict(); the fork nulls every timing field when the run was skipped. Kept the fork'sif not self.skip else Noneguard and applied it tomedian_timeas well, so skipped runs stay uniformly null.benchmark.py— both sides add imports at the same line (jsonvsos/shutil/subprocess). Kept both.A second breakage git did not flag
create_skipped_result()is a fork-only helper, and upstream mademedian_timea required dataclass field with no default. The helper does not pass it, so every skipped benchmark would have raisedTypeError— and git merged that file region without a murmur, because the two sides touched different parts of it.Fixed here, and all seven
BenchmarkResult(...)call sites in the directory were audited with an AST pass against the dataclass's required-field set; this was the only one missing an argument.This is the second occurrence of the same pattern in this sync series (see #1097's stale
ops.impl("ggml_*")registrations). Worth carrying forward as a standing check: after each merge, verify that fork-only code still satisfies any API upstream changed.How bad this would have been. Not a rarely-used manual flag:
tests/kernels/attention/benchmark/test_benchmark_attention.pydefines a per-platformSKIP_CASESdict, passes it tobenchmark.pyvia--skip, and the golden validator relies on the skipped case still being present in the output JSON (if actual.get("skip"): continue). So the attention golden-regression suite hits this path on every run that has skip cases defined for the platform — it would have failed outright on gfx1151, not just for someone experimenting with--skipby hand.Test plan
ruff checkandruff format --checkclean on the resolved files. (TheREADME.mdformatting complaint reproduces on upstream's own copy of the file and is not covered by the repo'sruff-formathook.)BenchmarkResult(...)construction against the required-field set.