perf: bound throughput by corrected ttft#1353
Conversation
Add a prefill-side TTFT capacity bound to disaggregated rate matching in addition to the existing prefill/decode throughput capacities. This uses corrected TTFT to account for first-token queueing, avoiding overprediction when prefill concurrency is the effective bottleneck. Signed-off-by: Spycsh <sihan.chen@intel.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesRate matching
Accuracy regression workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/aiconfigurator/sdk/picking.py`:
- Around line 73-90: Apply the same prefill TTFT capacity bound in the sweep
path’s _rate_match_dict calculation, so its seq_s and request_rate use the
minimum of prefill, decode, and TTFT-limited capacities. Prefer reusing shared
capacity arithmetic if appropriate, while preserving compatibility with
generator inputs, profiler data flow, and documented examples.
🪄 Autofix (Beta)
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: ccb16f49-26c9-4f06-8cf9-5f39da90e113
📒 Files selected for processing (1)
src/aiconfigurator/sdk/picking.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the project’s
uv-managed environment and run linting withruff check .andruff format --check ..
Files:
src/aiconfigurator/sdk/picking.py
**/*
⚙️ 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:
src/aiconfigurator/sdk/picking.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/picking.py
🧠 Learnings (1)
📚 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/picking.py
🔇 Additional comments (1)
src/aiconfigurator/sdk/picking.py (1)
73-90: 📐 Maintainability & Code QualityNo action needed.
Signed-off-by: Spycsh <sihan.chen@intel.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/accuracy-regression-testing.yml:
- Around line 3-15: Update the pull_request trigger in the workflow’s on
configuration to prevent body or base-branch edits from rerunning the
regression; either restrict edited events to title changes using the appropriate
event filter or remove edited if title updates should not retrigger it, while
preserving the other intended pull request events.
- Around line 58-68: Harden the old-ref handling in the workflow by passing the
selected reference through the step’s env rather than interpolating it into the
generated shell script, then quote the shell variable when it is used by the git
worktree command. Update the OLD_REF selection and downstream worktree
invocation while preserving the pull_request base SHA and workflow_dispatch
fallback behavior.
🪄 Autofix (Beta)
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: 4d2eba65-55ba-4849-a84d-248a1374bf87
📒 Files selected for processing (1)
.github/workflows/accuracy-regression-testing.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Collect snapshot (new)
- GitHub Check: copyright-checks
- GitHub Check: Build and Test (e2e)
- GitHub Check: Build and Test (unit)
- GitHub Check: Cargo Deny
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: AIC Accuracy Regression Testing
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: - Review workflow edits for least-privilege permissions, safe trigger scopes, secret exposure, fork behavior, and release/cherry-pick side effects.
- Verify matrix jobs, artifact uploads, and scheduled support-matrix workflows still produce debuggable evidence.
Files:
.github/workflows/accuracy-regression-testing.yml
🔇 Additional comments (2)
.github/workflows/accuracy-regression-testing.yml (2)
129-142: 🗄️ Data Integrity & IntegrationAlign retention for the linked regression artifact.
The PR handoff stores
artifact-url, but this primary upload uses the repository default retention while the handoff is retained for 30 days. Artifact URLs are only valid while the referenced artifact exists. (github.com) Verify the repository/org default is at least 30 days, or set it explicitly here:with: name: accuracy-regression-testing-results + retention-days: 30As per path instructions: “Verify matrix jobs, artifact uploads, and scheduled support-matrix workflows still produce debuggable evidence.”
Source: Path instructions
161-182: LGTM!
| # Nightly tracked metric plus opt-in PR signal for perf changes. The PR gate is | ||
| # prediction-regression-gate.yml (docs/design/prediction_regression_gate_design.md | ||
| # §2.6/§5). The value of a scheduled run is the new-side prediction-vs-silicon | ||
| # MAPE in the uploaded artifacts; the old-vs-new comparison is only meaningful | ||
| # on manual dispatches with an explicit old-ref. Longer term this converges | ||
| # onto tools/accuracy_tracking/run_silicon.py and the two-venv machinery here | ||
| # retires. | ||
| # §2.6/§5); PR-time accuracy runs are limited to titles prefixed with "perf:". | ||
| # Longer term this converges onto tools/accuracy_tracking/run_silicon.py and the | ||
| # two-venv machinery here retires. | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: | ||
| - opened | ||
| - edited | ||
| - synchronize | ||
| - reopened |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow around the mentioned lines and the surrounding job condition.
nl -ba .github/workflows/accuracy-regression-testing.yml | sed -n '1,140p'Repository: ai-dynamo/aiconfigurator
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow and the job condition around the referenced lines.
awk 'NR>=1 && NR<=160 { printf "%4d %s\n", NR, $0 }' .github/workflows/accuracy-regression-testing.ymlRepository: ai-dynamo/aiconfigurator
Length of output: 7909
Avoid rerunning the regression on non-title PR edits
pull_request.edited also fires for body and base-branch changes, but this guard only checks the current title. Any edit on a perf: PR can kick off another long regression run. Restrict edited to title changes, or drop it if title updates should not retrigger the workflow.
Suggested change
- if: github.event_name != 'pull_request' || startsWith(github.event.pull_request.title, 'perf:')
+ if: >-
+ github.event_name != 'pull_request' ||
+ (
+ startsWith(github.event.pull_request.title, 'perf:') &&
+ (
+ github.event.action != 'edited' ||
+ github.event.changes.title.from != null
+ )
+ )🤖 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 @.github/workflows/accuracy-regression-testing.yml around lines 3 - 15,
Update the pull_request trigger in the workflow’s on configuration to prevent
body or base-branch edits from rerunning the regression; either restrict edited
events to title changes using the appropriate event filter or remove edited if
title updates should not retrigger it, while preserving the other intended pull
request events.
Source: Path instructions
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| # Empty on schedule and pull_request events (inputs only exist for | ||
| # workflow_dispatch); fall back to main so nightly runs resolve a valid | ||
| # ref. | ||
| OLD_REF_INPUT: ${{ inputs.old-ref }} | ||
| run: | | ||
| OLD_REF="${OLD_REF_INPUT:-main}" | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| OLD_REF="${BASE_SHA}" | ||
| else | ||
| OLD_REF="${OLD_REF_INPUT:-main}" | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the workflow file and inspect relevant sections
git ls-files .github/workflows/accuracy-regression-testing.yml
wc -l .github/workflows/accuracy-regression-testing.yml
cat -n .github/workflows/accuracy-regression-testing.yml | sed -n '1,220p'
# Search for old-ref usage in the workflow
rg -n "old-ref|old_ref|OLD_REF|worktree add|GITHUB_OUTPUT" .github/workflows/accuracy-regression-testing.ymlRepository: ai-dynamo/aiconfigurator
Length of output: 10273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific area around the reported lines and the worktree step
sed -n '50,90p' .github/workflows/accuracy-regression-testing.yml | cat -nRepository: ai-dynamo/aiconfigurator
Length of output: 2285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the downstream step that consumes the generated old_ref output, if present
rg -n -A4 -B4 "steps\.old-ref\.outputs\.old_ref|worktree add|old_ref" .github/workflows/accuracy-regression-testing.ymlRepository: ai-dynamo/aiconfigurator
Length of output: 1782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print only the relevant occurrences with surrounding context
rg -n -C 3 "old_ref|OLD_REF|old-ref|GITHUB_OUTPUT|worktree add" .github/workflows/accuracy-regression-testing.ymlRepository: ai-dynamo/aiconfigurator
Length of output: 2736
Prevent old-ref script injection.
steps.old-ref.outputs.old_ref is inserted directly into the git worktree command. Pass it through env and quote the shell variable at the sink so a crafted workflow_dispatch value can’t alter the generated runner script.
Suggested change
- name: Create old revision worktree
+ env:
+ OLD_REF: ${{ steps.old-ref.outputs.old_ref }}
run: |
- git worktree add --detach ../aiconfigurator-old "${{ steps.old-ref.outputs.old_ref }}"
+ git worktree add --detach ../aiconfigurator-old "$OLD_REF"🤖 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 @.github/workflows/accuracy-regression-testing.yml around lines 58 - 68,
Harden the old-ref handling in the workflow by passing the selected reference
through the step’s env rather than interpolating it into the generated shell
script, then quote the shell variable when it is used by the git worktree
command. Update the OLD_REF selection and downstream worktree invocation while
preserving the pull_request base SHA and workflow_dispatch fallback behavior.
Source: Path instructions
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/accuracy-regression-comment.yml (3)
18-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDrop the apparently-unused
pull-requests: writepermission.Only
github.rest.issues.listComments/createComment/updateCommentare used in this file — these are Issues-API endpoints requiringissues: write, notpull-requests: write. Unless another consumer ofGITHUB_TOKENin this job needs it, this scope should be dropped for least privilege.🔒️ Proposed fix
permissions: actions: read contents: write issues: write - pull-requests: write🤖 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 @.github/workflows/accuracy-regression-comment.yml around lines 18 - 22, Remove the unused pull-requests: write permission from the workflow permissions block, keeping actions: read, contents: write, and issues: write unchanged.Source: Path instructions
25-29: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence on checkout; scope the token only where it's used.
The checkout step doesn't set
persist-credentials: false, so the write-scopedGITHUB_TOKEN(contents:write,issues:write,pull-requests:write) is written to disk for the whole job.actions/checkout@v5still stores the credential directly in the checked-out repo's .git/config — the safer$RUNNER_TEMPstorage produces lower-severity findings when v6.0.0 or higher of actions/checkout is used, reflecting a change in v6.0.0's credential persistence behavior, which this workflow doesn't yet benefit from. This workflow doesn't upload the workspace as an artifact, but the token is also readable by any later step in this job — and this job later does need push access for the "Publish comparison plot" step, so scope it just-in-time instead of for the whole job.🔒️ Proposed fix
- name: Checkout default branch uses: actions/checkout@v5 with: fetch-depth: 0 lfs: true + persist-credentials: falseThen, right before the
git pushin "Publish comparison plot" (around line 124), inject the token just for that push:git -C "${ARTIFACT_DIR}" remote set-url origin "https://x-access-token:${GH_TOKEN}`@github.com/`${GITHUB_REPOSITORY}.git"with
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}added to that step'senv:block.🤖 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 @.github/workflows/accuracy-regression-comment.yml around lines 25 - 29, Update the checkout step using actions/checkout@v5 to set persist-credentials to false. In the “Publish comparison plot” step, add GH_TOKEN from secrets.GITHUB_TOKEN to the step environment and set the repository remote URL immediately before git push using that token, so write credentials are available only for the push.Source: Linters/SAST tools
99-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo retry on concurrent pushes to the shared artifacts branch.
git push origin "${ARTIFACT_BRANCH}"can fail with a non-fast-forward error if two workflow runs (e.g., from different PRs) publish plots around the same time — there's no fetch/rebase/retry loop. The step'scontinue-on-error: trueprevents job failure, but the plot silently won't be published for the losing run. Consider a small retry loop (fetch + rebase + push, a few attempts) since each run writes to a uniquepr-${PR_NUMBER}/run-${TRIGGERING_RUN_ID}path, so conflicts are purely at the git-ref level and safely resolvable.🤖 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 @.github/workflows/accuracy-regression-comment.yml around lines 99 - 125, Update the artifact publication flow around the git push to handle concurrent updates to ARTIFACT_BRANCH: add a bounded retry loop that fetches the branch, rebases the local artifact worktree commit, and retries the push after non-fast-forward failures. Preserve the unique ARTIFACT_PATH layout and existing no-change behavior, and report failure only after all retry attempts are exhausted.
🤖 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 @.github/workflows/accuracy-regression-comment.yml:
- Around line 18-22: Remove the unused pull-requests: write permission from the
workflow permissions block, keeping actions: read, contents: write, and issues:
write unchanged.
- Around line 25-29: Update the checkout step using actions/checkout@v5 to set
persist-credentials to false. In the “Publish comparison plot” step, add
GH_TOKEN from secrets.GITHUB_TOKEN to the step environment and set the
repository remote URL immediately before git push using that token, so write
credentials are available only for the push.
- Around line 99-125: Update the artifact publication flow around the git push
to handle concurrent updates to ARTIFACT_BRANCH: add a bounded retry loop that
fetches the branch, rebases the local artifact worktree commit, and retries the
push after non-fast-forward failures. Preserve the unique ARTIFACT_PATH layout
and existing no-change behavior, and report failure only after all retry
attempts are exhausted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d430186-92a3-4e7e-bec7-9507868b8f26
📒 Files selected for processing (1)
.github/workflows/accuracy-regression-comment.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Collect snapshot (old)
- GitHub Check: AIC Accuracy Regression Testing
- GitHub Check: Cargo Deny
- GitHub Check: Build and Test (unit)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Rust/Python engine-step parity
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: - Review workflow edits for least-privilege permissions, safe trigger scopes, secret exposure, fork behavior, and release/cherry-pick side effects.
- Verify matrix jobs, artifact uploads, and scheduled support-matrix workflows still produce debuggable evidence.
Files:
.github/workflows/accuracy-regression-comment.yml
🪛 zizmor (1.26.1)
.github/workflows/accuracy-regression-comment.yml
[warning] 25-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 9-12: use of fundamentally insecure workflow trigger (dangerous-triggers): workflow_run is almost always used insecurely
(dangerous-triggers)
[info] 226-226: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (3)
.github/workflows/accuracy-regression-comment.yml (3)
9-16: 🔒 Security & Privacy
workflow_runmitigations look sound — worth a final sanity check.zizmor's
dangerous-triggersrule fires generically on anyworkflow_run, but this file already applies the standard mitigations: it checks out the default branch only (never the PR ref/code), and the job gates ongithub.event.workflow_run.event == 'pull_request'. Worth double-checking that the upstream "Accuracy Regression Testing" workflow (not shown here) never lets PR-controlled code influencepr-number.txt/artifact-url.txtbeyond the trustedgithub.event.pull_request.numbervalue already confirmed in its "Prepare PR comment inputs" step.Sources: Path instructions, Linters/SAST tools
31-39: Solid defensive design overall.Good use of
steps.download.outcome/if: always()gating so a missing artifact degrades gracefully, the symlink check (lines 49-52) before trusting artifact contents, PNG magic-byte/size validation before publishing the plot, CSV cell-escaping (md_cell) to keep the markdown table well-formed, and comment-length capping consistent with the existingcomment-charts.ymlpattern.Also applies to: 41-68, 70-129, 130-215, 217-257
59-65: 🔒 Security & PrivacyPR number is already trusted here —
pr-number.txtis written fromgithub.event.pull_request.numberin the triggering workflow, so the fallback stays numeric and doesn’t create a path or template-injection issue.> Likely an incorrect or invalid review comment.
1 participant
Overview:
bound throughput by corrected ttft
Observed from local disagg corr study but may not be the case for Nvidia. Check with CI.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Performance Improvements
Testing