Repository navigation
Conversation
parse_prometheus_text() interpolated into the +Inf bucket, so a finite latency distribution whose tail sits above the last finite bound reported p95/p99 = inf in the stage waterfall and transport breakdown. Return the highest finite bucket boundary when the selected quantile falls in the unbounded bucket, matching Prometheus histogram_quantile. Quantiles inside finite buckets still interpolate linearly. Closes ai-dynamo#16877 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
the
external-contribution
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Handle unbounded histogram buckets benchmarks/frontend/scripts/analysis/parsing_util.py, tests/benchmarks/test_frontend_parsing_util.py |
histogram_quantile returns the previous bucket boundary when the selected bucket is infinite or has the same count as the previous bucket. Parameterized parser tests check finite expected p50, p95, and p99 values across three count distributions. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Severity of issue fixed: Medium
Merge Risk: 
29dfb
-
The quantile fix matches the stated behavior and the added tests check the affected distributions. No actionable issue remains before merge.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 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: keeping histogram quantiles finite when the selected bucket is +Inf. Description check The description includes the required overview, details, reviewer starting point, related issues, validation results, and test coverage. It directly matches the pull request changes. Linked Issues check Directly linked issue #16877has two coding requirements. (1) When the selected quantile falls in the +Inf bucket, return the highest finite bucket bound. The change in benchmarks/frontend/scripts/ana…Out of Scope Changes check The whole-PR diff against the merge base 1ecc870 touches two files. parsing_util.py adds import mathand the one-condition change in histogram_quantile(). Both directly implement#16877. The new te…Full details: Linked Issues check
-
Explanation
Directly linked issue
#16877has two coding requirements. (1) When the selected quantile falls in the +Inf bucket, return the highest finite bucket bound. The change in benchmarks/frontend/scripts/analysis/parsing_util.py, histogram_quantile(), addsmath.isinf(le)to the existingcount == prev_countearly return. For the +Inf bucket it returns prev_le, the highest finite bound. (2) Keep linear interpolation inside finite buckets. The interpolation branch is unchanged. The new parametrized tests in tests/benchmarks/test_frontend_parsing_util.py cover the issue snapshot (tail-in-inf-bucket: p50 0.1, p95 1.0, p99 1.0), an all-in-+Inf case (all quantiles 1.0), and a finite-only case (p50 0.4375, p95 0.94375, p99 0.98875). I checked these expected values by hand against the interpolation formula. The issue asks for parser regression tests, and they exist. I did not run the test suite. The issue is CPU-only and the code uses no GPU or backend dependency. Unverified: the edge case of a snapshot with only a +Inf bucket returns 0.0. Prometheus returns NaN in that case. The issue does not specify this case, so it does not affect the verdict.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff against the merge base 1ecc870 touches two files. parsing_util.py adds
import mathand the one-condition change in histogram_quantile(). Both directly implement#16877. The new test file tests/benchmarks/test_frontend_parsing_util.py exercises only the changed behavior and the finite-interpolation regression. No unrelated files, refactors, or behavior changes appear in the diff. The test markers (unit, pre_merge, gpu_0) are test-infrastructure metadata, not feature changes.
-
-
- 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}
Overview:
Summary
parse_prometheus_text()in the frontend benchmark report interpolates into the+Infhistogram bucket. A finite latency distribution whose tail sits above the last finite bound reportsp95/p99asinfin the stage waterfall and transport breakdown.Repro on
main, from #16877: feed this snapshot toparse_prometheus_text()and
stage_durations["preprocess"]is{"p50": 0.1, "p95": inf, "p99": inf}.Details:
When the selected quantile lands in the unbounded bucket, return the highest finite bucket boundary (
1.0above). This is what Prometheushistogram_quantiledoes. Quantiles inside finite buckets still use linear interpolation, unchanged.The fix is one condition in
histogram_quantile(). The new test covers the tail-in-+Infcase, the all-samples-in-+Infcase, and a finite-only distribution to pin the existing interpolation.This picks up the fix proposed in #16880, which was closed before review.
Validation
Before the fix, the two
+Infcases fail:After:
pre-commit run --files <changed files>: isort, black, flake8, ruff, codespell pass.Where should the reviewer start?
benchmarks/frontend/scripts/analysis/parsing_util.py, thehistogram_quantile()loop.Related Issues
Summary by CodeRabbit