@claude /pr-review-toolkit:review-pr does this pr tackle the parent issue correctly? Validate correctness, ease of use, separation of concerns, adherence to our @docs/contributing/standards, invariants, behavioral discipline, behavioral tests and non-structural tests, modularity, clean api boundaries and contracts, performance implications, documentation correctness and completeness - including user and developer documentation.

Claude finished @namasl's task in 5m 23s —— View job

PR Review: fix(latency): model prefill-decode interference in aggregated roofline

Tasks:

• Initialize review
• Read changed files and parent issue
• Review correctness vs parent issue Roofline latency model underestimates prefill-decode interference in aggregated mode #949
• Check standards (rules.md, invariants.md, principles.md)
• Review behavioral tests and non-structural tests
• Check API boundaries, contracts, modularity
• Check documentation correctness
• Compile and post final review

Summary

The PR correctly addresses both gaps identified in issue #949 with clean, localized changes. Core implementation and test plan are solid. Three documentation gaps need attention before merge.

Correctness vs Issue #949

Gap 1 — Mixed-batch MFU degradation: ✅ Correctly implemented. The formula penalty = 1 - MixedBatchPenalty × minorityFraction divides totalComputeS by a value in [0.5, 1.0], increasing compute time proportionally to batch imbalance. At MixedBatchPenalty=0 the block is skipped entirely — exact backward compatibility.

Gap 2 — Imperfect compute/memory overlap: ✅ Correctly implemented. max(c, m) + OverlapPenalty × min(c, m) is the exact formula proposed in the issue. At OverlapPenalty=0 it reduces to max(c, m) — exact backward compatibility.

R11 (division guard): ✅ penalty ∈ [0.5, 1.0] — mathematically guaranteed since MixedBatchPenalty ∈ [0,1] and minorityFraction ∈ [0, 0.5]. Maximum degradation is ×2 at 50/50 split with MixedBatchPenalty=1. The comment on line 283–284 correctly explains this.

R18 (CLI flag precedence): ✅ Both flags use cmd.Flags().Changed() guards, preventing silent overwrite of hardware_config.json values when flags aren't explicitly provided.

R4 (construction site audit): ✅ HardwareCalib{} construction sites checked — test helpers and cluster tests use zero-valued struct literals, which correctly zero-initialize the new fields (both defaulting to "no penalty"). No missed construction sites.

Issues Found

Important

1. docs/concepts/roofline.md fields table is incomplete

Lines 99–105 list hardware_config.json fields as:

mixedBatchPenalty and overlapPenalty are not in this table. A user extending the config for a new GPU has no documentation for these optional fields or their semantics. This is the primary user-facing developer reference for the roofline hardware config.

Fix this →

2. docs/guide/latency-models.md "Supported hardware" section doesn't list new fields

The "Supported hardware" section states that the required fields are TFlopsPeak, BwPeakTBs, mfuPrefill, mfuDecode, MemoryGiB. The new optional fields are not listed anywhere here. A user checking this guide to add a new GPU entry has no guidance on the penalty parameters.

Fix this →

3. docs/reference/configuration.md CLI flags table missing new flags

The configuration reference has a table of CLI flags. --mixed-batch-penalty and --overlap-penalty are not listed. They have good --help text in cmd/root.go (lines 981–988) but the reference doc is used for structured discovery.

Fix this →

Suggestions

4. Variable naming: penalty is a divisor, not an additive penalty

In roofline.go:285, the local variable is named penalty but its semantics are inverted — penalty = 0.5 means maximum degradation (2× compute time), while penalty = 1.0 means no degradation. The name reads as if higher = worse, but numerically lower = worse. A name like mfuEfficiencyFactor or computeScaleFactor would be less surprising:

// Before
penalty := 1.0 - hwConfig.MixedBatchPenalty * minorityFraction
totalComputeS /= penalty

// Clearer
mfuEfficiencyFactor := 1.0 - hwConfig.MixedBatchPenalty * minorityFraction
totalComputeS /= mfuEfficiencyFactor

The comment on lines 283–284 partially compensates but the variable name itself will confuse the next reader who isn't expecting a divisor.

5. BC-6 test is structurally coupled to formula internals

TestRooflineStepTime_ZeroPenalties_BackwardCompatible (roofline_test.go) manually reimplements calculateTransformerFlops, calculateMemoryAccessBytes, and the roofline formula to derive expected. Per the BDD/TDD standards: "Would this test still pass if the implementation were completely rewritten but the behavior preserved?" — no, because it hardcodes the two-loop structure and clampToInt64 call.

A simpler behavioral alternative: capture a known-good expected value (golden-style) or assert that zero-penalty results are <= any nonzero-penalty result for the same batch. The backward-compatibility law can be tested without reimplementing the implementation. This is a suggestion, not a blocker — the test does provide strong regression coverage.

Standards Compliance Summary

Test Plan Coverage

BC-1 through BC-7 tests are all present and cover the stated behavioral contracts:

• BC-1 ✅: compute-bound mixed batch increases with penalty
• BC-2 ✅: pure-prefill and pure-decode unaffected (two separate tests)
• BC-3 ✅: larger minority fraction → higher penalty ratio (verified via ratio comparison, not absolute time, to handle different total token counts — correct approach)
• BC-4 ✅: overlap penalty increases step time for any non-empty batch
• BC-5 ✅: monotonicity of overlap penalty
• BC-6 ✅: zero penalties reproduce original formula (structurally coupled, see suggestion 5)
• BC-7 ✅: validation rejects out-of-range and NaN/Inf values for both fields

The testHardwareCalib() helper correctly leaves the new fields zero (Go zero-value), so all 235 pre-existing roofline tests act as BC-6-equivalent coverage implicitly.

Verdict

Ready to merge after addressing documentation gaps 1–3. The implementation is correct, backward-compatible, localized to rooflineStepTime, and well-tested. Issues 1–3 are user-facing documentation that new contributors and GPU calibrators will hit. Issues 4–5 are low-priority suggestions.

@claude /pr-review-toolkit:review-pr does this pr tackle the parent issue correctly? Validate correctness, ease of use, separation of concerns, adherence to our @docs/contributing/standards, invariants, behavioral discipline, behavioral tests and non-structural tests, modularity, clean api boundaries and contracts, performance implications, documentation correctness and completeness - including user and developer documentation.

Claude finished @namasl's task in 1m 17s —— View job

PR Review: fix(latency): model prefill-decode interference in aggregated roofline

Tasks:

• Initialize review
• Read changed files and parent issue
• Review correctness vs parent issue Roofline latency model underestimates prefill-decode interference in aggregated mode #949
• Check standards (rules.md, invariants.md, principles.md)
• Review behavioral tests and non-structural tests
• Check API boundaries, contracts, modularity
• Check documentation correctness and completeness
• Compile and post final review

Summary

The PR correctly addresses both gaps from issue #949 with clean, localized changes. The implementation is sound and backward-compatible. The previous automated review incorrectly flagged three documentation gaps that are all already present in this PR's diff. Three test quality issues need attention — one of which makes a core test vacuous.

Correctness vs Issue #949

Gap 1 — Mixed-batch MFU degradation: ✅ mfuScaleFactor = 1 - MixedBatchPenalty × minorityFraction divides totalComputeS by a value in [0.5, 1.0], increasing compute time proportionally to batch imbalance. Guard condition MixedBatchPenalty > 0 && len(Prefill) > 0 && len(Decode) > 0 ensures the block is fully skipped at default 0 — exact backward compatibility.

Gap 2 — Imperfect compute/memory overlap: ✅ max(c, m) + OverlapPenalty × min(c, m) is exactly the formula proposed in the issue. At OverlapPenalty=0 it reduces to max(c, m) — exact backward compatibility.

R11 (division guard): ✅ mfuScaleFactor ∈ [0.5, 1.0] — mathematically proven (MixedBatchPenalty ∈ [0,1] × minorityFraction ∈ [0, 0.5] ≤ 0.5). Comment on lines 283–284 documents this. Division by totalTokens at line 281 is guarded by the outer condition requiring both PrefillRequests and DecodeRequests to be non-empty.

R18 (CLI flag precedence): ✅ Both flags use cmd.Flags().Changed() guards in resolveLatencyConfig.

R4 (construction site audit): ✅ Only two HardwareCalib{} production sites exist (config.go:89,98), both error returns that correctly zero-initialize. All test sites either use the blackbox backend (penalty never reached) or zero-initialize correctly.

Issues Found

Important — Test Quality

1. BC-3 is vacuous: the assertion is never exercised

TestRooflineStepTime_MixedBatchPenalty_SymmetricMinority (roofline_test.go:1211) is intended to verify that a 50/50 token split produces a higher penalty ratio than a 90/10 split. However, unlike BC-1 which explicitly sets BwPeakTBs = 1e6 to force compute-bound behavior, BC-3 uses testHardwareCalib() with standard H100 specs (BwPeakTBs = 3.35).

MixedBatchPenalty exclusively scales totalComputeS. For a 32-layer/4096-hidden model, weight bandwidth alone (~15 GB / 3.35 TB/s ≈ 4.5 ms) dwarfs compute time for 50–100 token batches (~256 GFLOPS / (989 TFLOPS × 0.55) ≈ 0.47 ms). Both batches are memory-bound, so max(totalComputeS, totalMemoryS) = totalMemoryS regardless of the penalty, and both ratios equal 1.0. The assertion ratio5050 >= ratio9010 becomes 1.0 >= 1.0 — trivially true. Deleting the entire MixedBatchPenalty block from roofline.go would not cause this test to fail.

Fix: add hc.BwPeakTBs = 1e6 and hcNoPenalty.BwPeakTBs = 1e6 in BC-3, matching the established technique from BC-1.

Fix this →

2. Flag parity tests don't cover new flags

Two tests in cmd/simconfig_shared_test.go maintain hardcoded flag lists:

• TestRunCmd_SimConfigFlagsParity (line 76) — missing "mixed-batch-penalty" and "overlap-penalty"
• TestBothCommands_SimConfigFlagsHaveIdenticalDefaults (line 163) — also missing both new flags

While the flags ARE correctly registered for both commands via registerSimConfigFlags (replay.go:303), these parity guards exist specifically to catch future regressions. Omitting the new flags creates a blind spot.

Fix this →

Suggestions

3. No test for MixedBatchPenalty on a memory-bound mixed batch (no-op confirmation)

BC-2 tests pure-prefill and pure-decode batches. It doesn't test the case where a mixed batch is memory-bound — the penalty should still be a no-op in this regime (the penalty applies to totalComputeS, which is not the bottleneck). This would be a useful regression guard in case someone accidentally applies the penalty to totalMemoryS instead.

4. OverlapPenalty = 1.0 boundary case has no behavioral test

At OverlapPenalty = 1.0, the formula reduces to c + m (additive rather than max). BC-7 validates that 1.0 is accepted, but no behavioral test checks the resulting step time equals totalComputeS + totalMemoryS. This is the most extreme user-facing case and a natural invariant to verify.

5. BC-6 is structurally coupled (per BDD/TDD standards)

TestRooflineStepTime_ZeroPenalties_BackwardCompatible reimplements calculateTransformerFlops, calculateMemoryAccessBytes, and clampToInt64 inline. Per the project's refactoring survival test ("Would this test still pass if the implementation were completely rewritten?") — no, it would fail. The behavioral contract ("zero penalties produce identical output to pre-#949") could be tested with a physics invariant instead: for a memory-bound step, result × peakBW ≈ totalBytes; compare against the existing TestRooflineStepTime_SingleCrossover_MemoryBoundDecode approach. This is low priority but noted for completeness.

Minor Documentation Gap

6. roofline.md Section 2 formula is outdated

Sections 2–4 of docs/concepts/roofline.md describe the mathematical formula as max(FLOPs/Peak, Bytes/BW). After this PR, the actual formula is max(c, m) + OverlapPenalty × min(c, m) for non-default configs. The new fields are correctly documented in the table (lines 104–105), but the conceptual formula in the overview section doesn't reflect the change.

Fix this →

Correction to Previous Review

The previous automated review identified three "Important" documentation gaps. All three are already present in this PR:

• docs/concepts/roofline.md lines 104–105: ✅ both new fields listed in the table
• docs/guide/latency-models.md line 70: ✅ "optional interference fields" added
• docs/reference/configuration.md: ✅ both CLI flags added to the table

Standards Compliance

Verdict

Recommend fixing issues 1–2 before merge — BC-3 is currently vacuous (deleting the implementation wouldn't break it), and the parity test omission leaves a regression gap. Issues 3–6 can go to a follow-up.

@claude /pr-review-toolkit:review-pr does this pr tackle the parent issue correctly? Validate correctness, ease of use, separation of concerns, adherence to our @docs/contributing/standards, invariants, behavioral discipline, behavioral tests and non-structural tests, modularity, clean api boundaries and contracts, performance implications, documentation correctness and completeness - including user and developer documentation.

Claude finished @namasl's task in 3m 47s —— View job

PR Review: fix(latency): model prefill-decode interference in aggregated roofline

Tasks:

• Initialize review
• Read changed files and gather context
• Review correctness vs parent issue Roofline latency model underestimates prefill-decode interference in aggregated mode #949
• Check standards (rules.md, invariants.md, principles.md)
• Review behavioral tests and non-structural tests
• Check API boundaries, contracts, modularity
• Check documentation correctness and completeness
• Compile and post final review

Summary

This is the third review pass. The first two automated reviews identified issues that the author has since resolved. I re-read all 11 changed files from scratch against the current HEAD. All important issues are resolved. The PR is ready to merge, with two residual low-priority suggestions and one new observation.

Correctness vs Issue #949

Gap 1 — Mixed-batch MFU degradation: ✅ Formula correct. Guard condition MixedBatchPenalty > 0 && len(PrefillRequests) > 0 && len(DecodeRequests) > 0 ensures exact backward compatibility at default 0. The variable is named mfuScaleFactor (fixed from penalty in previous review). Division guard proven in comment: mfuScaleFactor ∈ [0.5, 1.0] since MixedBatchPenalty ∈ [0,1] and minorityFraction ∈ [0, 0.5].

Gap 2 — Imperfect compute/memory overlap: ✅ Formula max(c,m) + OverlapPenalty × min(c,m) exactly matches the issue proposal. At OverlapPenalty=0 reduces to max(c,m) — exact backward compatibility.

Scope note: The penalties apply only to RooflineLatencyModel (--latency-model roofline). The TrainedRooflineLatencyModel (--latency-model trained-roofline) has its own learned corrections in trained_roofline.go and is not affected by these parameters. This is architecturally correct — the trained model's beta coefficients capture these effects through regression — but worth noting for users who configure hardware_config.json penalties and then switch backends.

Previous Review Issues — Resolution Status

All items flagged as "Important" in the second automated review have been resolved in the latest two commits:

Standards Compliance

Test Coverage Assessment

All BC-1 through BC-7 tests are present and, after the BC-3 fix, non-vacuous:

Pre-existing tests serve as implicit BC-6 coverage: all 235+ prior roofline tests use testHardwareCalib() which zero-initializes both new fields, continuously validating backward compatibility.

Remaining Suggestions (non-blocking)

1. BC-6 is structurally coupled to implementation internals

TestRooflineStepTime_ZeroPenalties_BackwardCompatible reimplements calculateTransformerFlops, calculateMemoryAccessBytes, and clampToInt64 inline to derive expected. Per the BDD/TDD principles ("Would this test still pass if the implementation were completely rewritten?"): no, it would fail if the internal FLOP formula changed, even if the observable behavior (physics) was preserved. The 235+ pre-existing tests already provide this backward-compatibility guarantee implicitly. This is a suggestion, not a blocker — the test provides strong regression coverage.

2. No behavioral test for OverlapPenalty = 1.0 boundary

At OverlapPenalty=1.0 the formula reduces to compute + memory (fully additive). BC-7 validates that 1.0 is accepted, but no test asserts the resulting step time equals totalComputeS + totalMemoryS. This is the most extreme user-configurable case and a natural physics invariant to verify. Not required for this PR's stated goals.

Verdict

Ready to merge. All critical and important issues from the two prior reviews have been resolved. The implementation is correct, backward-compatible, localized to rooflineStepTime and ValidateRooflineConfig, and well-tested. The two remaining suggestions above are follow-up candidates, not blockers.

@claude /pr-review-toolkit:review-pr does this pr tackle the parent issue correctly? Validate correctness, ease of use, separation of concerns, adherence to our @docs/contributing/standards, invariants, behavioral discipline, behavioral tests and non-structural tests, modularity, clean api boundaries and contracts, performance implications, documentation correctness and completeness.

Claude finished @namasl's task in 6m 0s —— View job

PR Review: fix(latency): model prefill-decode interference in aggregated roofline

Tasks:

• Initialize review
• Read changed files from current HEAD
• Review correctness vs parent issue Roofline latency model underestimates prefill-decode interference in aggregated mode #949
• Check standards compliance (rules.md, invariants.md, principles.md)
• Review behavioral tests and non-structural tests
• Check API boundaries, contracts, modularity
• Check documentation correctness and completeness
• Run tests (Go not available in this environment — reviewed by code inspection + CI)
• Compile and post final review

Summary

Fourth review pass. All important issues from prior reviews have been resolved. The implementation is correct, backward-compatible, and well-tested. One new minor observation; two low-priority suggestions from review #3 remain open. Ready to merge.

Correctness vs Issue #949

Gap 1 — Mixed-batch MFU degradation (roofline.go:329-341): ✅

• Formula: mfuScaleFactor = 1.0 - MixedBatchPenalty × minorityFraction; divides totalComputeS.
• Guard: MixedBatchPenalty > 0 && len(PrefillRequests) > 0 && len(DecodeRequests) > 0 — block fully skipped at default 0.
• Range proof in comment: mfuScaleFactor ∈ [0.5, 1.0] since MixedBatchPenalty ∈ [0,1] and minorityFraction ∈ [0, 0.5]. No division-by-zero possible.
• Variable name mfuScaleFactor (not the confusing penalty from earlier drafts) — correct.

Gap 2 — Imperfect compute/memory overlap (roofline.go:361): ✅

• Formula: max(c,m) + OverlapPenalty × min(c,m) — exact match to issue proposal.
• At OverlapPenalty=0: reduces to max(c,m) — exact backward compatibility.

Scope boundary (note for reviewers): Both penalties apply only to --latency-model roofline. The trained-roofline backend learns its own correction coefficients (the beta array) through regression — it correctly does not use these parameters. This is architecturally sound but worth noting for users who configure hardware_config.json penalties and then switch backends.

Resolution Status of Prior Review Issues

Standards Compliance

Hardware config backward compatibility: encoding/json zero-initializes missing fields. Existing hardware_config.json files without mixedBatchPenalty/overlapPenalty will parse correctly with both = 0.0 (no-penalty behavior). ✅

Test Coverage

Pre-existing 235+ roofline tests (using testHardwareCalib() with zero-initialized new fields) serve as continuous BC-6-equivalent regression coverage.

New Observation

CLI validation pathway for out-of-range penalty flags

If a user passes --mixed-batch-penalty 1.5, the Changed() guard sets hwConfig.MixedBatchPenalty = 1.5 without early CLI-side validation. The value is caught downstream: ValidateRooflineConfig inside NewLatencyModel returns an error → NewInstanceSimulator panics with a descriptive message: "NewInstanceSimulator(…): NewLatencyModel: latency model: invalid roofline config: HardwareCalib.MixedBatchPenalty must be in [0, 1] when set, got 1.5".

This is consistent with how other NewInstanceSimulator failures behave. The panic message is clear. An early logrus.Fatalf in resolveLatencyConfig (matching the alpha/beta validation pattern at root.go:333-339) would be more user-friendly, but this is a minor polish item, not a blocker.

Remaining Low-Priority Suggestions (unchanged from review #3)

1. BC-6 is structurally coupled to implementation internals

TestRooflineStepTime_ZeroPenalties_BackwardCompatible (roofline_test.go:1447) reimplements calculateTransformerFlops, calculateMemoryAccessBytes, and clampToInt64 inline. Per BDD/TDD standards ("Would this test still pass if the implementation were completely rewritten?"): no. The 235+ pre-existing tests already provide the backward-compatibility guarantee implicitly. Consider replacing with a simpler physics invariant instead. Not a blocker.

2. No behavioral test for OverlapPenalty = 1.0 boundary

At OverlapPenalty=1.0, the formula reduces to compute + memory (fully additive). BC-7 validates that 1.0 is accepted, but no test asserts that the resulting step time equals totalComputeS + totalMemoryS. This is the most extreme user-configurable case and a natural physics invariant. Not a blocker.

Verdict

Ready to merge. The implementation is correct, backward-compatible, localized to rooflineStepTime and ValidateRooflineConfig, and all critical/important issues from prior reviews have been resolved. The two remaining suggestions above and the new CLI-validation observation are polish items for a follow-up, not blockers.