feat(sdk): self-contained result rows + closed-loop latency refinement (report + opt-in SLA)

[tianhaox]

What

Three pieces; default results unchanged (the third is opt-in):

1. Result rows become self-contained. run_agg records the operating-point step timings it already computes into the summary row (ColumnsAgg: mix_step_ms, genonly_step_ms, prefill_step_ms, num_mix_steps, num_genonly_steps). Disagg rows record (p)prefill_step_ms — the raw solo context latency of one prefill worker, captured before the autoscale TTFT pre-correction.

2. A post-processing refinement tier (sdk/closed_loop_ttft.py):

out = refine_closed_loop_latency(df)   # df in, df out — consumes only the row's own columns
# adds ttft_refined / tpot_refined / throughput_refined; legacy columns untouched

Under the hood: a deterministic replay of the scheduler's own budget arithmetic at pass granularity — the fused continuous-batching calendar for agg rows, the P/D tandem (round-robin router, whole-prompt prefill batches, decode-attach handoff) for disagg rows. No RNG, no fitted constants, ~ms per row. The three refined columns are one consistent timeline (they satisfy the closed-loop identity C/X = TTFT + (osl−1)·TPOT), so consume them as a set.

3. Opt-in refined-SLA enforcement in the sweep (Task(refined_sla=True), default off): sweep_agg/sweep_disagg compare the TTFT/TPOT targets against the refined steady closed-loop values instead of the fixed-factor-corrected ones. Agg candidate points gate on the solo chunked-prefill lower bound and are then priced; disagg drops the 1.8 prefill gate to the solo bound (a true necessary condition) and knee-tunes the top rate-matched combinations per decode-parallel category — a binary search for the largest compliant concurrency (never above the decode ladder's memory-validated point), with the decode step iterated to its occupancy-consistent value on the decode ladder (Little's law fixed point). Priced rows are repriced from the tandem, retiring the flat rate-match derate for refined rows. Post-processing helpers filter_closed_loop_sla / pick_under_closed_loop_sla cover the same semantics outside the pipeline. Pricing is memoized; overhead on an 8-GPU Qwen3-32B sweep: agg +~2s, disagg ~2min.

Usage & cost

Default: everything off. Without touching anything, the only change is bookkeeping — result rows carry the extra step-timing columns; sweep results, SLA filtering, picking and runtimes are byte-identical to main (locked by the parity/unit suites).

Tier 1 — post-processing (call it when you want it):

out  = refine_closed_loop_latency(df)                    # adds *_refined columns
ok   = filter_closed_loop_sla(df, ttft_ms=500)           # post-filter on refined values
best = pick_under_closed_loop_sla(df, ttft_ms=500)       # re-pick per deployment

Cost: ~ms per row (pure-Python deterministic replay; no DB or engine calls).

Tier 2 — in-sweep enforcement: Task(refined_sla=True) — or refined_sla: true in an experiment YAML (maps through Task.from_yaml). Deliberately NOT applied to pick_autoscale (the dynamo-planner profiling flow: its 1.8 is autoscaling headroom, a different semantic) or AFD frames (pass through unpriced pending their own validation). Measured on an 8-GPU Qwen3-32B sweep (isl4096/osl256, ttft 2000ms / tpot 30ms, H20 perf data, warm DB):

All warm-process numbers (cold adds a one-time ~3-6s perf-DB load either way); pricing is memoized process-wide, so repeated sweeps over the same operating points pay it once:

sweep	default	refined_sla=True
agg, single SLA point	~0.5s	+<1s
agg, pareto tpot sweep	~1s	+~3.6s first run, +0.2s cached
disagg, single SLA point	~0.4s	~3.2s
disagg, pareto tpot sweep	~2.9s	~58s
single-point estimate (run_single_agg/run_single_disagg)	ms	+ms (headline repriced to the closed-loop values; run_single_disagg's default output previously reported the raw solo TTFT with no correction at all)

The disagg pareto cost is the concurrency knee search: each latency-target pair prices its own rate-matched candidates through the tandem (~1.8k pricings at 4–235ms each; operating-point results are memoized). Single-SLA-point tasks — the common "give me a deployment for this target" call — stay at seconds.

Why

The heuristic TTFT corrections on result rows are clamped/fitted and miss the real shape of closed-loop queueing — which is not even monotone in concurrency (replacement prefill chunks stretch passes; past the knee, larger decode batches eat the cycle and TTFT recedes). A recursion reproduces this because both effects emerge from iterating the budget arithmetic; a multiplier cannot. Derivation and scope notes: docs/design/closed_loop_refinement.md.

Validation (vs the full queueing-model evaluator, identical timing; Qwen3-32B, H20, isl4096/osl256)

arm	result
agg estimator, exact timing callables, C=2..64	TTFT/TPOT/throughput within 0.2% (bit-identical ≤C=8)
agg estimator fed only recorded row scalars	within ~3% over the same sweep
disagg tandem vs disagg evaluator (1P1D/2P1D/2P2D, serial & batched prefill, shallow→deep queueing)	bit-identical TTFT & throughput at all nine points

Validation against real serving

Same estimators, nine closed-loop operating points against trtllm-serve 1.3.0rc20 (Qwen3-32B tp4, H20, chunked prefill ON, isl 1024/4096/8192 x C 4/16/48; predictions computed from the perf DB before the measurements ran):

isl/osl	C	throughput est vs real (d%)	TTFT p99 d%	TPOT d%
1024/128	4	2.68 vs 2.57 (+4.4%)	+2.4%	-13.5%
1024/128	16	5.07 vs 4.81 (+5.3%)	+8.3%	-10.8%
1024/128	48	6.16 vs 3.97 (+55%)	+3.8%	-49%
4096/256	4	0.95 vs 0.91 (+4.5%)	-0.3%	-2.6%
4096/256	16	1.39 vs 1.39 (+0.1%)	+29.5%	-4.5%
4096/256	48	1.61 vs 1.29 (+24.3%)	-0.3%	-18.5%
8192/128	4	0.69 vs 0.70 (-0.6%)	+2.4%	+2.7%
8192/128	16	0.78 vs 0.81 (-4.2%)	+22.6%	+5.7%
8192/128	48	0.81 vs 0.82 (-0.5%)	+30.6%	+7.5%

Reading: throughput within 5.3% on 7/9 points; the two misses (1024 C48, 4096/256 C48) trace to perf-DB timing in deep-churn regimes (real mixed passes cost up to ~2x the DB pricing; the same gap appears if you price those steps standalone), not to the recursion — refined TTFT errs on the conservative side there. The measured closed-loop TTFT correction factor (steady TTFT / solo no-queue TTFT) spans 2.8-14.4x across this small grid and is non-monotone in concurrency — which is why the refinement computes it from the operating point instead of applying any fixed multiplier.

8xH20 A/B arbitration: legacy pick vs refined pick

For the same task (8 GPUs, isl4096/osl256, TTFT<=2000ms, TPOT<=30ms) the legacy sweep picks 1P(tp4)+1D(tp4)@C5; refined_sla=True picks 3P(tp2)+1D(tp2)@c16. Both were deployed on 8xH20 (TRT-LLM native disagg, ctx unchunked bs1, gen bs64; predictions frozen before measuring; steady-state protocol: 20 generations/slot, first 5 and last 1 discarded — the synchronized first round costs 1.5-5x the steady TTFT and multi-worker round-robin needs ~5 generations to relax).

Arm A — the legacy pick's topology at three concurrencies the legacy model prices identically (solo x1.8 = 1077.5ms):

C	real TTFT	refined pred	legacy pred
5	629	737 (+17%)	1077.5 (+71%)
9	2828	3132 (+11%)	1077.5 (-62%)
13	5065	5526 (+9%)	1077.5 (-79%)

Arm B — the refined pick's topology:

C	real TTFT	refined pred	real TPOT (pred)	real X (pred)
16 (pick)	1201 <= SLA	1596 (+33%, conservative)	17.17 (17.62)	2.867 (2.713)
19	2239 — violates, as predicted	2731 (+22%)	17.21	2.863

Real-vs-real: the refined pick serves 91.8 vs 57.1 tok/s/gpu (+61%) under the same TTFT SLA — the legacy 1.8x gate had pre-killed the tp2-prefill family (solo x1.8 = 2070 > 2000) whose true closed-loop TTFT at the knee is 1201ms. X(C16)=X(C19) confirms the knee plateau: concurrency past the knee buys zero throughput and doubles TTFT.

Rust/Python parity

No parity-governed path is touched (operations/**, perf_database.py, perf_interp/**, engine.py, rust_engine_step.py all unchanged). The recorded scalars are the FFI-returned values — identical whichever engine step computed them — and the estimators live in the Python orchestration layer, which stays Python by the dedup plan. test_rate_match_parity extended coverage passes.

Tests

1004 sdk unit tests green (13 new: estimator semantics incl. the closed-loop identity, P/D imbalance signatures, chunked-off admission, DataFrame refine with NaN tolerance, rate-match parity).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added closed-loop latency estimation for aggregate and disaggregated serving configurations.
  • Added refined TTFT, TPOT, and throughput metrics based on recorded operating-point measurements.
  • Added modeling for batching, queueing, prefix caching, chunked prefill, worker scaling, and handoff delays.
  • Results now include detailed prefill, generation, mixed-step timing metrics, and step counts.

• Bug Fixes

  • Preserved raw prefill-step latency alongside corrected TTFT values.

• Documentation

  • Added guidance on closed-loop refinement, validation, and supported scenarios.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The change records aggregate and disaggregated timing metadata, adds deterministic fused and disaggregated latency estimators, and refines result DataFrames with recalculated TTFT, TPOT, and throughput values.

Changes

Closed-loop latency refinement

Layer / File(s)	Summary
Timing metadata contracts and recording aic-core/src/aiconfigurator_core/sdk/{common.py,backends/base_backend.py}, src/aiconfigurator/sdk/{inference_session.py,picking.py,sweep.py}	Result schemas and producers now retain aggregate step timings, scheduling counts, and raw disaggregated prefill-step latency.
Fused and disaggregated latency estimators src/aiconfigurator/sdk/closed_loop_ttft.py	Adds deterministic estimators for fused continuous batching and disaggregated prefill/decode execution.
DataFrame refinement and documented behavior src/aiconfigurator/sdk/closed_loop_ttft.py, docs/design/closed_loop_refinement.md	Adds row-wise estimator selection, refined TTFT, TPOT, and throughput columns, and optional refined SLA filtering. Invalid rows receive NaN.
Estimator and refinement tests tests/unit/sdk/test_closed_loop_ttft.py	Tests queueing, prefix caching, chunked prefill, tandem execution, validation, metric consistency, column preservation, SLA filtering, and invalid-row handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Poem

Timing flows through every row,
Queues and workers trace their way.
TTFT, TPOT, throughput glow,
NaNs guard the invalid day.
Fused and tandem paths align.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is detailed and on-topic, but it omits the required reviewer-start and related-issues sections.	Use the template headings and add the missing reviewer starting point and related issue information.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the self-contained result rows and opt-in closed-loop latency refinement.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (5)

src/aiconfigurator/sdk/closed_loop_ttft.py (4)

351-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the swallowed exception before writing NaN.

The handler catches KeyError, ValueError, TypeError, and RuntimeError and writes NaN. A missing column, a convergence failure, and a genuine bug all produce the same silent result. Add a debug log so the cause is recoverable.

🔍 Proposed fix

-        except (KeyError, ValueError, TypeError, RuntimeError):
+        except (KeyError, ValueError, TypeError, RuntimeError) as exc:
+            logger.debug("closed-loop refinement skipped a row: %s", exc)
             ttfts.append(float("nan"))
             tpots.append(float("nan"))
             xs.append(float("nan"))

Add the module logger near the imports:

import logging

logger = logging.getLogger(__name__)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/aiconfigurator/sdk/closed_loop_ttft.py` around lines 351 - 354, Add a
module-level logger near the imports, then update the exception handler in the
TTFT processing flow to log the caught exception at debug level before appending
NaN values to ttfts, tpots, and xs. Preserve the existing caught exception types
and NaN fallback behavior.

---

313-316: 🚀 Performance & Scalability | 🔵 Trivial

Consider the cost of refining large sweep DataFrames.

Each row runs a full pass-by-pass simulation. estimate_closed_loop_latency allows up to 200 * (warmup_generations + window_generations) * osl passes, which is 409,600 passes for osl=256 with the defaults. A sweep DataFrame with thousands of rows can take a long time.

Document the expected per-row cost, or expose warmup_generations / window_generations on refine_closed_loop_latency so callers can trade accuracy for speed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/aiconfigurator/sdk/closed_loop_ttft.py` around lines 313 - 316, The
refine_closed_loop_latency loop currently performs an expensive full simulation
for every DataFrame row. Expose warmup_generations and window_generations
through refine_closed_loop_latency and pass them to
estimate_closed_loop_latency, preserving existing defaults so callers can trade
accuracy for runtime.

---

135-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Prefer identity-based removal over list.remove.

slots.remove(s) compares list elements by value. Slots are plain lists, so two slots with identical contents compare equal. The current code is safe only because a finished slot never equals an unfinished one. That invariant is easy to break in a later change.

Rebuild the list by identity instead.

🛡️ Proposed refactor

         finished = [s for s in completers + decoders if s[1] >= osl]
+        if finished:
+            finished_ids = {id(s) for s in finished}
+            slots = [s for s in slots if id(s) not in finished_ids]
         for s in finished:
             completions += 1
             if completions == warmup:
                 steady_start = now
             if completions > warmup:
                 steady_completions += 1
                 gaps.extend(s[5])
                 if s[5]:
                     tpots.append(sum(s[5]) / len(s[5]))
-            slots.remove(s)
             pending.append((now + turnaround_ms, new_slot(now)))

The same concern applies to q.remove(r) at line 253 and run.remove(r) at line 280.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/aiconfigurator/sdk/closed_loop_ttft.py` around lines 135 - 146, Replace
value-based removals with identity-based filtering for the finished slot in the
shown completion loop, and apply the same change to q.remove(r) and
run.remove(r). Rebuild each list while excluding only the exact object instance
being removed, preserving all other equal-valued entries.

---

151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared quantile helper.

The project ignores Ruff E731, so replacing the lambda is not required for lint compliance. The two q implementations are identical; move them to one module-level _quantile helper to prevent drift. A one-click change is not suitable because the refactor changes scope and updates both estimators.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/aiconfigurator/sdk/closed_loop_ttft.py` at line 151, Extract the
duplicated q logic from both estimators into one module-level _quantile helper
in closed_loop_ttft.py, then update each estimator to call it. Preserve the
current empty-input behavior and quantile index calculation exactly, removing
the local q definitions to prevent future drift.

tests/unit/sdk/test_closed_loop_ttft.py (1)

90-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add two cases that guard the production paths.

Both fixtures are hand-built dicts. Two realistic paths are untested.

1. _build_disagg_summary_dict and _rate_match_dict write (p)prefill_step_ms with a .get() default, so a row can carry None. No test covers that row reaching refine_closed_loop_latency.
2. The fixtures do not reference common.ColumnsAgg or common.ColumnsDisagg. If a column is renamed in aic-core/src/aiconfigurator_core/sdk/common.py, these tests still pass while every real row degrades to NaN.

🧪 Proposed additional test

    def test_disagg_row_without_prefill_step_ms_is_nan(self):
        df = pd.DataFrame(
            [
                {
                    "concurrency": 8,
                    "isl": 4096,
                    "osl": 64,
                    "(p)workers": 1,
                    "(d)workers": 1,
                    "(p)bs": 1,
                    "(d)bs": 64,
                    "(p)prefill_step_ms": None,
                    "tpot": 21.0,
                    "ttft": 3600.0,
                }
            ]
        )
        out = refine_closed_loop_latency(df)
        assert pd.isna(out["ttft_refined"].iloc[0])
        assert out["ttft"].iloc[0] == 3600.0

For point 2, assert that the required column names are present in common.ColumnsAgg and common.ColumnsDisagg.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/sdk/test_closed_loop_ttft.py` around lines 90 - 150, Add coverage
in TestRefineDataFrame for a disaggregated row whose “(p)prefill_step_ms” value
is None, asserting refinement returns NaN while preserving legacy ttft. Also
validate that all column names used by these fixtures are defined in
common.ColumnsAgg and common.ColumnsDisagg, so renames cannot silently
invalidate the tests.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@aic-core/src/aiconfigurator_core/sdk/common.py`:
- Around line 783-789: Split the inline comment after "power_w" from the
following operating-point step-timings comment so each comment starts on its own
line, preserving the existing field order and wording.

In `@docs/design/closed_loop_refinement.md`:
- Around line 48-59: Update the Validation section to cite reproducible evidence
for the reported agreement figures, including the nine-point disaggregated
results. Reference the script, notebook, or test that generates these
comparisons; if no such artifact exists, label the claims as one-off
measurements and include the measurement date and full configuration.

In `@src/aiconfigurator/sdk/closed_loop_ttft.py`:
- Around line 56-59: Update the slot-layout comment above new_slot to describe
the six fields actually returned: remaining_prefill, generated, arrival_ms,
first_token_ms, last_token_ms, and the gaps list at index 5; remove the
nonexistent gap_sum, gap_count, and gaps_list_or_None entries.

In `@src/aiconfigurator/sdk/inference_session.py`:
- Around line 272-274: Update the prefill_dict initialization in the inference
session flow to subtract prefill_dict.get("encoder_latency", 0.0) from ttft
before storing prefill_step_ms, while leaving the subsequent ttft autoscaling
correction unchanged.

In `@src/aiconfigurator/sdk/picking.py`:
- Line 122: Use a float NaN fallback for the prefill step field in both
rate-matching builders: update _build_disagg_summary_dict in
src/aiconfigurator/sdk/picking.py#L122-L122 and _rate_match_dict in
src/aiconfigurator/sdk/sweep.py#L163-L163 to convert prefill_step_ms with a
float("nan") default, preserving numeric column dtype when the field is absent.

---

Nitpick comments:
In `@src/aiconfigurator/sdk/closed_loop_ttft.py`:
- Around line 351-354: Add a module-level logger near the imports, then update
the exception handler in the TTFT processing flow to log the caught exception at
debug level before appending NaN values to ttfts, tpots, and xs. Preserve the
existing caught exception types and NaN fallback behavior.
- Around line 313-316: The refine_closed_loop_latency loop currently performs an
expensive full simulation for every DataFrame row. Expose warmup_generations and
window_generations through refine_closed_loop_latency and pass them to
estimate_closed_loop_latency, preserving existing defaults so callers can trade
accuracy for runtime.
- Around line 135-146: Replace value-based removals with identity-based
filtering for the finished slot in the shown completion loop, and apply the same
change to q.remove(r) and run.remove(r). Rebuild each list while excluding only
the exact object instance being removed, preserving all other equal-valued
entries.
- Line 151: Extract the duplicated q logic from both estimators into one
module-level _quantile helper in closed_loop_ttft.py, then update each estimator
to call it. Preserve the current empty-input behavior and quantile index
calculation exactly, removing the local q definitions to prevent future drift.

In `@tests/unit/sdk/test_closed_loop_ttft.py`:
- Around line 90-150: Add coverage in TestRefineDataFrame for a disaggregated
row whose “(p)prefill_step_ms” value is None, asserting refinement returns NaN
while preserving legacy ttft. Also validate that all column names used by these
fixtures are defined in common.ColumnsAgg and common.ColumnsDisagg, so renames
cannot silently invalidate the tests.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c5942aa3-9438-41bf-b01a-c8738c3186ba

📥 Commits

Reviewing files that changed from the base of the PR and between 8fc57cf and b5efea4.

📒 Files selected for processing (8)

• aic-core/src/aiconfigurator_core/sdk/backends/base_backend.py
• aic-core/src/aiconfigurator_core/sdk/common.py
• docs/design/closed_loop_refinement.md
• src/aiconfigurator/sdk/closed_loop_ttft.py
• src/aiconfigurator/sdk/inference_session.py
• src/aiconfigurator/sdk/picking.py
• src/aiconfigurator/sdk/sweep.py
• tests/unit/sdk/test_closed_loop_ttft.py

📜 Review details

⏰ Context from checks skipped due to timeout. (10)

• GitHub Check: Collect snapshot (new)
• GitHub Check: Collect snapshot (old)
• GitHub Check: Build wheels (macosx_arm64)
• GitHub Check: Build wheels (manylinux_2_28_x86_64)
• GitHub Check: Build wheels (manylinux_2_28_aarch64)
• GitHub Check: Build and Test (unit)
• GitHub Check: Build and Test (e2e)
• GitHub Check: Rust/Python engine-step parity
• GitHub Check: aic-core public API contract
• GitHub Check: Cargo Deny

🧰 Additional context used

📓 Path-based instructions (5)

**/*

⚙️ CodeRabbit configuration file

**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.

• Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
• If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.

Files:

• aic-core/src/aiconfigurator_core/sdk/backends/base_backend.py
• src/aiconfigurator/sdk/sweep.py
• src/aiconfigurator/sdk/inference_session.py
• docs/design/closed_loop_refinement.md
• src/aiconfigurator/sdk/picking.py
• aic-core/src/aiconfigurator_core/sdk/common.py
• tests/unit/sdk/test_closed_loop_ttft.py
• src/aiconfigurator/sdk/closed_loop_ttft.py

aic-core/src/aiconfigurator_core/sdk/**

⚙️ CodeRabbit configuration file

aic-core/src/aiconfigurator_core/sdk/**: - Verify core SDK API changes remain compatible with legacy aiconfigurator.sdk imports, generator inputs, profiler data flow, and documented examples.

• Flag upper-layer dependencies or silent schema drift introduced into the minimal core distribution.

Files:

• aic-core/src/aiconfigurator_core/sdk/backends/base_backend.py
• aic-core/src/aiconfigurator_core/sdk/common.py

src/aiconfigurator/sdk/**

⚙️ CodeRabbit configuration file

src/aiconfigurator/sdk/**: - Verify SDK API changes remain compatible with generator inputs, profiler data flow, and documented examples.

• Flag silent schema or field-name drift between SDK models and generator/module bridge code.

Files:

• src/aiconfigurator/sdk/sweep.py
• src/aiconfigurator/sdk/inference_session.py
• src/aiconfigurator/sdk/picking.py
• src/aiconfigurator/sdk/closed_loop_ttft.py

docs/**

⚙️ CodeRabbit configuration file

docs/**: - Check that docs match changed CLI, SDK, generator, backend, and support-matrix behavior.

• Flag docs that describe unsupported runtimes, stale command names, or behavior not covered by tests or support-matrix evidence.

Files:

• docs/design/closed_loop_refinement.md

tests/**

⚙️ CodeRabbit configuration file

tests/**: - Check that tests cover the changed behavior rather than only the happy path.

• Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.

Files:

• tests/unit/sdk/test_closed_loop_ttft.py

🧠 Learnings (2)

📚 Learning: 2026-08-03T13:45:40.375Z

Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 1460
File: collector/case_generator.py:2228-2254
Timestamp: 2026-08-03T13:45:40.375Z
Learning: For DeepSeek-V4 CSA top-k DELTA calibration in ai-dynamo/aiconfigurator, apply calibration data only when the runtime native num_heads exactly matches the calibration bucket (currently 64 or 128). Do not borrow calibration across head-count buckets: Flash and Pro DELTA values can diverge by up to 37% at long-context shapes. This exact-match rule applies to both Python and Rust consumers, including collector/case_generator.py.

Applied to files:

• aic-core/src/aiconfigurator_core/sdk/backends/base_backend.py
• src/aiconfigurator/sdk/sweep.py
• src/aiconfigurator/sdk/inference_session.py
• src/aiconfigurator/sdk/picking.py
• aic-core/src/aiconfigurator_core/sdk/common.py
• tests/unit/sdk/test_closed_loop_ttft.py
• src/aiconfigurator/sdk/closed_loop_ttft.py

📚 Learning: 2026-05-01T00:39:37.334Z

Learnt from: simone-chen
Repo: ai-dynamo/aiconfigurator PR: 956
File: src/aiconfigurator/sdk/perf_database.py:4144-4151
Timestamp: 2026-05-01T00:39:37.334Z
Learning: In src/aiconfigurator/sdk/**/*.py, preserve upstream metadata for PerformanceResult.source: since PerformanceResult defaults source to "silicon", callers should not force-set result.source (e.g., PerfDatabase._query_silicon_or_hybrid should rely on the default and keep any existing source information rather than overwriting it). Only set source explicitly when you truly intend to change it.

Applied to files:

• src/aiconfigurator/sdk/sweep.py
• src/aiconfigurator/sdk/inference_session.py
• src/aiconfigurator/sdk/picking.py
• src/aiconfigurator/sdk/closed_loop_ttft.py

🪛 GitHub Actions: Lint and Format / 1_Lint and Format (Ruff).txt

aic-core/src/aiconfigurator_core/sdk/common.py

[error] 783-783: Ruff E501: line too long (121 > 120). The 'ruff check .' command failed with exit code 1.

🪛 GitHub Actions: Lint and Format / Lint and Format (Ruff)

aic-core/src/aiconfigurator_core/sdk/common.py

[error] 783-783: Ruff E501: Line too long (121 > 120). The 'power_w' line exceeds the maximum allowed length.

🔇 Additional comments (6)

aic-core/src/aiconfigurator_core/sdk/backends/base_backend.py (1)

1644-1652: LGTM!

src/aiconfigurator/sdk/inference_session.py (1)

733-736: LGTM!

src/aiconfigurator/sdk/picking.py (1)

444-446: LGTM!

src/aiconfigurator/sdk/sweep.py (1)

686-689: LGTM!

tests/unit/sdk/test_closed_loop_ttft.py (1)

25-88: LGTM!

aic-core/src/aiconfigurator_core/sdk/common.py (1)

818-821: 🗄️ Data Integrity & Integration

Keep (p)prefill_step_ms in its current position.

All ColumnsDisagg producers pass dictionaries, so the inserted field does not shift values for positional consumers.

			> Likely an incorrect or invalid review comment.

[coderabbitai]

🧹 Nitpick comments (1)

docs/design/closed_loop_refinement.md (1)

74-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify that the p99 comparison is estimator-only.

refine_closed_loop_latency adds ttft_refined, tpot_refined, and throughput_refined. It does not add a refined p99 column. The estimator computes p99 values, but the refinement function discards them. As written, this paragraph can imply that the PR produces refined TTFT p99, which conflicts with Lines 101-102.

Suggested change

- throughput within 5.3% on 7 of 9 points and TTFT p99 within a few percent at low/moderate
+ throughput within 5.3% on 7 of 9 points and the estimator's TTFT p99 within a few percent at low/moderate

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/design/closed_loop_refinement.md` around lines 74 - 80, Update the
serving-comparison paragraph to clarify that the TTFT p99 comparison reflects
estimator-computed values only, not a refined p99 output from
refine_closed_loop_latency. Avoid implying that the refinement adds a refined
TTFT p99 column, and keep the existing throughput and latency findings
unchanged.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@docs/design/closed_loop_refinement.md`:
- Around line 74-80: Update the serving-comparison paragraph to clarify that the
TTFT p99 comparison reflects estimator-computed values only, not a refined p99
output from refine_closed_loop_latency. Avoid implying that the refinement adds
a refined TTFT p99 column, and keep the existing throughput and latency findings
unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d81b5c00-9718-411d-bc40-77d923ebd3c5

📥 Commits

Reviewing files that changed from the base of the PR and between 2409137 and b394587.

📒 Files selected for processing (1)

• docs/design/closed_loop_refinement.md

📜 Review details

⏰ Context from checks skipped due to timeout. (10)

• GitHub Check: Collect snapshot (new)
• GitHub Check: Collect snapshot (old)
• GitHub Check: aic-core public API contract
• GitHub Check: Rust/Python engine-step parity
• GitHub Check: Build and Test (unit)
• GitHub Check: Build and Test (e2e)
• GitHub Check: Build wheels (macosx_arm64)
• GitHub Check: Cargo Deny
• GitHub Check: Build wheels (manylinux_2_28_aarch64)
• GitHub Check: Build wheels (manylinux_2_28_x86_64)

🧰 Additional context used

📓 Path-based instructions (2)

**/*

⚙️ CodeRabbit configuration file

**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.

• Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
• If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.

Files:

• docs/design/closed_loop_refinement.md

docs/**

⚙️ CodeRabbit configuration file

docs/**: - Check that docs match changed CLI, SDK, generator, backend, and support-matrix behavior.

• Flag docs that describe unsupported runtimes, stale command names, or behavior not covered by tests or support-matrix evidence.

Files:

• docs/design/closed_loop_refinement.md

🔇 Additional comments (3)

docs/design/closed_loop_refinement.md (3)

61-94: Add reproducible evidence for the validation claims.

These lines add exact DES, real-serving, and error-mode measurements. The supplied context does not include a script, notebook, test, or dated artifact that reproduces them. Add the repository artifact and its configuration, or label the numbers as one-off measurements. The same evidence gap was raised for Lines 48-59 in the previous review, so this is a duplicate concern. A one-click change is not safe because the correct wording depends on the available validation artifact.

As per path instructions, documentation must match behavior and support-matrix or test evidence.

Source: Path instructions

---

96-100: LGTM!

---

104-113: 🗄️ Data Integrity & Integration

Confirm the SLA target semantics before documenting this wiring.

refine_closed_loop_latency provides steady mean ttft_refined and tpot_refined only. It does not provide p50, p99, or transient values. The statement in Lines 111-112 supports mean-based SLA targets, but the phrase “an SLA on served latency” is broader. Confirm the existing SLA contract, then narrow the wording or add percentile outputs in the follow-up. No one-click change is safe because the correct metric depends on the SLA contract.

Source: Path instructions

[jasonqinzhou]

REQUEST_CHANGES · 5.8/10 · high confidence

What this PR does

This PR makes aggregate and disaggregated result rows carry their operating-point timing inputs, adds deterministic closed-loop replay for refined TTFT, TPOT, and throughput, and optionally uses those values for sweep SLA enforcement and single-point headlines.

The implementation is unusually thoughtful about keeping the default path unchanged: the timing columns are additive, the replay is deterministic and memoized, the design notes distinguish the post-processing and in-sweep tiers, and the author backed the model with semantic tests plus queueing-evaluator and real-serving comparisons.

Why this score

The current head is 5.8/10 with REQUEST_CHANGES. The text-only single-replica path is well structured and its submitted tests and CI are green, but two confirmed P1 contract breaks make the opt-in results unsafe for supported configurations: multimodal rows are repriced without encoder latency or visual context tokens, and aggregate refined throughput loses the pp/attention-DP deployment scale. Two narrower P2 gaps also remain: osl=1 produces a NaN TPOT that rejects valid refined-SLA rows, and aggregate speculative-decoding progress is ignored by the replay.

[jasonqinzhou]

[P1] Do not apply the text-only replay to multimodal rows. BaseBackend.run_agg prices prefill/decode at text_isl + img_ctx_tokens but stores only the text isl in the result row, while prefill_step_ms intentionally excludes the encoder phase; the disagg row likewise stores raw context time after subtracting encoder_latency. Both branches here then ignore encoder_latency, and the agg branch also reconstructs the pass count from the text-only length. A focused probe shows that changing a row from encoder_latency=0 to 500 leaves the refined triple identical, and reprice_closed_loop_row overwrites the legacy TTFT with that lower value. With refined_sla=True this can admit a VLM deployment whose actual TTFT exceeds the target. Please either record/model the visual-token and encoder stages for both row shapes, or return NaN and preserve the legacy verdict for multimodal rows until that replay is supported; add aggregate and disaggregated VLM regressions.

[jasonqinzhou]

[P1] Preserve deployment-wide aggregate throughput scaling. run_agg models one local batch b and then scales both concurrency and output throughput by pp * attention_dp_size. Passing only row["bs"] here correctly replays one local scheduler for latency, but the returned throughput_rps is then published by refine_closed_loop_latency / reprice_closed_loop_row as deployment-wide throughput without restoring that scale. For a synthetic row with bs=4 and concurrency=8, the closed-loop identity holds for 4 / X but fails by 2x for the row's advertised 8 / X; run_single_agg(refined_sla=True) correspondingly halves seq/s and tokens/s. Please scale the replay throughput by concurrency / bs (and account for that factor in or after the cache) while leaving per-replica latency unchanged, with a PP/attention-DP regression.

[jasonqinzhou]

[P2] Define no-decode TPOT for osl=1 instead of returning NaN. osl=1 is a supported no-decode workload with existing backend coverage, but neither estimator records an inter-token gap, so both return tpot_mean=NaN. The row still has a finite refined TTFT, which makes _sweep_one_parallel_agg treat it as priced and then reject it because NaN <= tpot_target is false; reprice_closed_loop_row also turns the otherwise valid request latency into NaN because NaN * 0 remains NaN. A focused probe reproduced both behaviors. Please give the no-decode case an explicit finite TPOT/request-latency convention (for example TPOT 0), mirror it in the tandem estimator, and add aggregate/disaggregated osl=1 tests through the refined-SLA and reprice paths.

[jasonqinzhou]

[P2] Honor aggregate speculative-decoding progress in the replay. When nextn_accepted > 0, run_agg reduces decode_iterations and num_genonly_steps using decode_tokens_per_iteration; genonly_step_ms remains the cost of one verification iteration. This replay always advances one output token per genonly_step_ms and neither reads the recorded step counts nor carries decode_tokens_per_iteration, so an MTP row with fewer scheduler steps prices identically to a non-speculative row with the same scalars. That overstates refined TPOT/latency and can discard valid refined_sla points or corrupt the single-point headline. Please record the accepted-token progress in the self-contained row and advance the replay consistently (or fail open for such rows), with a SpeculativeDecodingProfile regression.

[Spycsh]

Does “closed-loop” here mean an infinite request stream with a fixed concurrency/BS, where whenever one request finishes, a new request immediately arrives to keep the number of active requests constant? If so, the arrival times are derived from the simulated completion times rather than being explicit inputs—is that the intended workload model?

[tianhaox]

yes. the closed loop refers to the concurrency based workload where a concurrency N is kept. The open loop is more like request rate based. there's no fixed budget of the slots for the concurrent requests.