Repository navigation
Conversation
get_prefill_ttft() documents that it returns None on benchmark failure, but both the default and attention-DP paths asserted that benchmark_prefill() succeeded. A nonzero AIPerf exit therefore raised AssertionError and aborted the whole profiling sweep instead of letting the caller skip that measurement. Return None in both paths. The two callers (thorough.py and profile_prefill.py) already skip a None TTFT, matching the decode helper. Closes ai-dynamo#16971 Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi lonexreb! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
fix
external-contribution
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Return None for failed prefill benchmarks components/src/dynamo/profiler/utils/aiperf.py, components/src/dynamo/profiler/tests/unit/test_aiperf.py |
Both execution paths return None when benchmark_prefill returns no result. A parameterized test checks the return value and verifies that the benchmark is called once for attention data-parallel sizes 1 and 2. |
Priority:
Estimated code review effort: 2 (Simple) | ~5 minutes
Severity of issue fixed: Medium
Merge Risk: 
24bdf
-
Failed prefill measurements can now be skipped so the profiling sweep continues, with both execution paths covered by the new test.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: returning None from get_prefill_ttft when AIPerf fails. Description check The description includes all required sections, explains the failure and fix, identifies review starting points, links related issues, and documents validation results. Linked Issues check Directly linked issue #16971requires that get_prefill_ttft() return None when AIPerf fails, in both the default path and the attention-DP path, instead of raising AssertionError. The diff to componen…Out of Scope Changes check The whole-PR diff against the merge base touches only two files. components/src/dynamo/profiler/utils/aiperf.py changes only the two assertion sites in get_prefill_ttft() to None returns. components/s… Full details: Linked Issues check
-
Explanation
Directly linked issue
#16971requires that get_prefill_ttft() return None when AIPerf fails, in both the default path and the attention-DP path, instead of raising AssertionError. The diff to components/src/dynamo/profiler/utils/aiperf.py replacesassert aiperf_result is not Nonewithif aiperf_result is None: return Nonein both paths (the DEP branch at the benchmark_prefill call, and the default branch after it). The function already returns None for TTFT extraction failures, so the documented None contract holds for all failure points. The new test test_get_prefill_ttft_returns_none_when_benchmark_fails in components/src/dynamo/profiler/tests/unit/test_aiperf.py is parametrized over attention_dp_size 1 and 2. It patches dynamo.profiler.utils.aiperf.benchmark_prefill to return None and asserts that the result is None and that the benchmark is called once. This covers the coding requirement. The caller in components/src/dynamo/profiler/utils/profile_prefill.py skips points where ttft is None, so the sweep continues as the issue expects. The PR's reported before/after pytest output was not re-run by this assessment. The assessment relies on the diff and source read at head. Issue#16672and#7038are out of scope for this issue and are not required here.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff against the merge base touches only two files. components/src/dynamo/profiler/utils/aiperf.py changes only the two assertion sites in get_prefill_ttft() to None returns. components/src/dynamo/profiler/tests/unit/test_aiperf.py adds the import of get_prefill_ttft and one parametrized regression test for the same behavior. No unrelated code, configuration, or documentation changes appear. Both changes directly implement issue
#16971. The docstring already states the None-on-failure contract, so no doc change is needed.
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
External PR triageBlocker: Trusted CI has never run (no Next action (@ai-dynamo/dynamo-planner-codeowners): Review the PR and post Supply-chain audit of Status as of 2026-10-29 for head |
Overview:
Summary
get_prefill_ttft()documents that it returnsNoneon benchmark failure, but both its default and attention-DP paths doassert aiperf_result is not Noneright afterbenchmark_prefill(). A nonzero AIPerf exit (for example an endpoint returning HTTP 503) therefore raisesAssertionErrorand aborts the whole profiling sweep, instead of letting the sweep skip that one measurement.Details:
Replace the two asserts with
return None.benchmark_prefill()already logs the AIPerf exit code, stdout and stderr before returningNone, so nothing is lost. Both callers already handle aNoneTTFT:components/src/dynamo/profiler/thorough.pyappends a prefill row onlyif ttft is not Nonecomponents/src/dynamo/profiler/utils/profile_prefill.pydoes the same in_profile_prefill_helperThis matches the decode helper, which returns
(None, None)on failure. It picks up the fix proposed in #14322, which was closed before review.Validation
New unit test patches
benchmark_prefillto returnNoneand callsget_prefill_ttft()withattention_dp_size=1and2.Before the fix:
After:
pre-commit run --files <changed files>: isort, black, flake8, ruff, codespell pass.Where should the reviewer start?
components/src/dynamo/profiler/utils/aiperf.py,get_prefill_ttft().Related Issues
Summary by CodeRabbit