fix(benchmarks): OPS-8740 drop httpx backend attribution from the HTTP sweep harness by dmitry-tokarev-nv · Pull Request #14886 · ai-dynamo/dynamo

dmitry-tokarev-nv · 2026-09-15T21:38:01Z

Linear: OPS-8740

Summary

The HTTP sweep in benchmarks/multimodal/http selects a backend and labels each result with it. Only one backend exists since #14563.

run_one() writes DYN_HTTP_BACKEND, and it stores the name that the caller passed in RunResult.backend. The facade logs a warning for any value other than aiohttp and uses aiohttp. So a run that asks for httpx uses aiohttp but prints under an httpx column. The sweep does not fail. It prints an A/B table in which both columns measured aiohttp.

This change removes the selection, because there is nothing left to select:

runner.py: run_one() takes no backend argument and does not write to the environment. RunResult has no backend field.
stats.py: Summary has no backend field.
report.py: the table and the grid have one column, and _DISPLAY_ORDER is gone.
sweep.py: each cell runs once. _by_backend(), the alternating run order, and the two-column grid are gone.

A second commit rewrites the comments of this change in plain English. Each comment states one fact, checked against the current main.

Two commits address review. One removes a test fixture that no test needed. The other replaces two structural tests with a test that runs the sweep, because the structural tests did not run the sweep or the report.

Start the review at runner.py. Two lines caused the bug: os.environ["DYN_HTTP_BACKEND"] = backend and RunResult(backend=backend, ...).

Relates to #14563 and #14474.

Validation

Measured on the branch rebased onto 5a47eb6312, with this command on main and on this branch:

DYN_MM_ALLOW_INTERNAL=1 python -m benchmarks.multimodal.http.sweep \
    --server-processing-time-means-ms 10,20 --request-rate 50 --requests 30

On main, the log shows DYN_HTTP_BACKEND='httpx' is not supported; using aiohttp. two times. The table still prints two columns, and they differ only by noise:

mean_ms |     aiohttp wall / p50 / p99 / err |       httpx wall / p50 / p99 / err
     10 |    0.6 /   16.7 /    31.1 /    0 |    0.6 /   18.1 /    21.1 /    0
     20 |    0.6 /   30.4 /    35.0 /    0 |    0.6 /   28.5 /    35.1 /    0

On this branch, the sweep prints one column:

mean_ms |             wall / p50 / p99 / err
     10 |    0.6 /   19.3 /    22.2 /    0
     20 |    0.6 /   30.2 /    35.0 /    0

Other results:

The tests pin the fix. With runner.py, stats.py, report.py and sweep.py from main, all 3 tests fail. The sweep test fails with assert 8 == 4, because each of the 4 cells ran twice. With this branch, all 3 pass.
Each test catches a defect that the other two miss. A second run per cell, a grid header that names httpx, or a second grid column fails only test_sweep_runs_each_cell_once_and_prints_one_column. A run that writes DYN_HTTP_BACKEND=aiohttp fails only test_run_one_leaves_dyn_http_backend_unset. A run that deletes the variable fails only test_run_one_does_not_clobber_an_operator_set_backend.
The tests run in CI. In the vllm-runtime-test image (amd64, uid 1000, --network none), the marker expression of the CPU parallel job selects all 3 tests. They pass with -W error. The expression of the sequential job selects none of them.
The rebase changed nothing in the fix. git range-diff shows the original commit as identical, and no conflicts came up.
pre-commit run --all-files exits 0.

Note

Without DYN_MM_ALLOW_INTERNAL=1, every request of the sweep fails, on main and on this branch. Since #14474, a fetch_bytes call without a policy refuses loopback addresses at connect time, and the sweep fetches from a server on localhost. The sweep still exits 0. This comes from #14474 and is outside the scope of this change.

🤖 Generated with Claude Code

coderabbitai · 2026-09-15T21:41:49Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ef2229b8-f792-4412-80b4-14cc7a6b7fd0

📥 Commits

Reviewing files that changed from the base of the PR and between f1cb3ae and 05f1533.

📒 Files selected for processing (6)

benchmarks/multimodal/http/report.py
benchmarks/multimodal/http/runner.py
benchmarks/multimodal/http/stats.py
benchmarks/multimodal/http/sweep.py
benchmarks/multimodal/http/tests/conftest.py
benchmarks/multimodal/http/tests/test_http_sweep_runner.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Changes

HTTP benchmark simplification

Layer / File(s)	Summary
Single-path result contracts `benchmarks/multimodal/http/runner.py`, `benchmarks/multimodal/http/stats.py`	The runner uses the fixed aiohttp-backed path. `RunResult` and `Summary` no longer contain backend fields.
Single-path sweep and reporting `benchmarks/multimodal/http/sweep.py`, `benchmarks/multimodal/http/report.py`	The sweep runs once per cell. Reporting prints one summary and one combined metrics column per row.
Regression coverage and client cleanup `benchmarks/multimodal/http/tests/*`	Tests verify the removed backend interface and environment behavior. An autouse fixture closes the HTTP client after each test.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 05f15

The benchmark now uses and reports one HTTP fetch path per sweep cell without a remaining concrete merge risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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 identifies the benchmark fix and the removal of incorrect httpx backend attribution from the HTTP sweep harness.
Description check	✅ Passed	The description provides a detailed summary, implementation details, reviewer starting point, related issues, and validation results. It does not use every template heading or checkbox format, but it …

Fix all pre-merge checks with AI

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

The sweep selected a backend by writing DYN_HTTP_BACKEND and labelled the result with the string it was handed (runner.py: os.environ[...] = backend, then RunResult(backend=backend, ...)). Since #14563 the facade warns and falls back to aiohttp for any non-aiohttp value, so a run requested as "httpx" is measured by AiohttpClient and still printed under an "httpx" column. It does not crash or skip -- it emits a clean-looking A/B table in which both sides are aiohttp, with the two columns differing only by cold-start noise. Measured before the change, against a local server: run_one('httpx') -> label='httpx' client actually used=AiohttpClient run_one('aiohttp') -> label='aiohttp' client actually used=AiohttpClient There is one backend now, so remove the selection rather than relabel it: run_one() takes no backend argument and no longer touches the environment, RunResult/Summary carry no backend field, and report.py prints a single column. That also deletes the A/B scaffolding the comparison needed -- _by_backend(), the alternating run order, and the two-column grid. The README block that documented how to reproduce this sweep was removed in #14563, so the harness was orphaned as well as wrong. Tests pin the attribution away: no backend selector on run_one, no backend field on either result type, and a run leaves DYN_HTTP_BACKEND untouched. All five fail on the pre-fix code. Relates to #14563 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>

Rewrite the docstrings and comments that this change adds. Remove the em-dashes and the sentence fragments. Each comment now states one fact, checked against the current main: - The root conftest.py adds pre_merge, gpu_0 and defaulted to an unmarked test, and pr.yaml runs defaulted tests in the CPU parallel job of the dynamo-runtime pipeline. The logs of the last CI run show all five tests in that job, on amd64 and on arm64. - The shared HTTP client stays None after run_one([]), so no current test creates it. The conftest docstring now says that the fixture only guards a future test. - A run_one that writes DYN_HTTP_BACKEND=aiohttp without a backend parameter fails only test_run_one_leaves_dyn_http_backend_unset. Its docstring now names that case. Remove the docstring of test_run_one_does_not_clobber_an_operator_set_backend, because it only restated the name of the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>

The autouse fixture in benchmarks/multimodal/http/tests/conftest.py closed the shared HTTP client after each test, but no test creates that client. run_one() calls close_http_client() before and after each run, and the tests pass an empty URL list, so no fetch happens. Measured on c9a0b5c: with the file removed, all 5 tests pass with -W error, and the shared client is still None after run_one([]). Reported by rmccorm4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>

… one column The signature and dataclass tests never ran the sweep or the report, so they could not see the defect that a user sees. Measured on c9a0b5c: a sweep that runs each cell twice, or a grid header that names httpx, left all 5 tests passing. Replace those two tests with one test that runs the real sweep, run, summary and report code. It stubs only the media server and the URL list, so no request leaves the process. It checks that each pair of a request rate and a delay runs once, that the output names no backend, and that each grid has one column. On the pre-fix sources from main, the new test fails with assert 8 == 4, because each of the 4 cells ran twice. It also fails alone for each of three mutations: a second run per cell, a grid header that names httpx, and a second grid column without backend names. Keep both environment tests, because each catches a defect that the other two tests miss: - A run that writes DYN_HTTP_BACKEND=aiohttp fails only test_run_one_leaves_dyn_http_backend_unset. - A run that deletes the variable fails only test_run_one_does_not_clobber_an_operator_set_backend. Their docstrings now say so. Reported by rmccorm4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>

devin-ai-integration · 2026-10-01T23:37:25Z

✅ Dynamo PR CI passed — run 36937609182 (attempt 1) on `fdc25c1880`

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

_{Posted automatically by Devin for run 36937609182. Updated on every full-CI run of this PR.}

dmitry-tokarev-nv requested a review from a team as a code owner September 15, 2026 21:38

pull-request-size Bot added the size/L label Sep 15, 2026

github-actions Bot added the fix label Sep 15, 2026

dynamo-review-agent Bot reviewed Sep 15, 2026

View reviewed changes

dmitry-tokarev-nv and others added 2 commits September 30, 2026 15:41

dmitry-tokarev-nv force-pushed the dtokarev/benchmark-http-single-backend branch from 05f1533 to c9a0b5c Compare September 30, 2026 19:48

dmitry-tokarev-nv changed the title ~~fix(benchmarks): drop backend attribution from the HTTP sweep harness~~ fix(benchmarks): OPS-8740 drop backend attribution from the HTTP sweep harness Sep 30, 2026

dmitry-tokarev-nv changed the title ~~fix(benchmarks): OPS-8740 drop backend attribution from the HTTP sweep harness~~ fix(benchmarks): OPS-8740 drop httpx backend attribution from the HTTP sweep harness Sep 30, 2026

rmccorm4 reviewed Oct 1, 2026

View reviewed changes

Comment thread benchmarks/multimodal/http/tests/conftest.py Outdated

Comment thread benchmarks/multimodal/http/tests/test_http_sweep_runner.py Outdated

rmccorm4 approved these changes Oct 1, 2026

View reviewed changes

dmitry-tokarev-nv and others added 2 commits October 1, 2026 18:47

dmitry-tokarev-nv merged commit a88ef6d into main Oct 2, 2026
115 checks passed

dmitry-tokarev-nv deleted the dtokarev/benchmark-http-single-backend branch October 2, 2026 19:25

Navigation Menu

Repository navigation

fix(benchmarks): OPS-8740 drop httpx backend attribution from the HTTP sweep harness - #14886

fix(benchmarks): OPS-8740 drop httpx backend attribution from the HTTP sweep harness#14886
dmitry-tokarev-nv merged 4 commits into
mainfrom
dtokarev/benchmark-http-single-backend

dmitry-tokarev-nv commented Sep 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devin-ai-integration Bot commented Oct 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dmitry-tokarev-nv commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Uh oh!

coderabbitai Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devin-ai-integration Bot commented Oct 1, 2026

✅ Dynamo PR CI passed — run 36937609182 (attempt 1) on fdc25c1880

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmitry-tokarev-nv commented Sep 15, 2026 •

edited

Loading

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

✅ Dynamo PR CI passed — run 36937609182 (attempt 1) on `fdc25c1880`