fix(deps): declare httpx, unbreaking partial-install (torch) for every PR - #2547
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/Model-Optimizer/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe ChangesHF optional dependencies
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The dependency declaration and collection guard address the reported partial-install failure. No outstanding merge-blocking issue is evident. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
71db41c to
a5354ca
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2547 +/- ##
==========================================
+ Coverage 71.56% 78.64% +7.07%
==========================================
Files 607 607
Lines 67609 67609
==========================================
+ Hits 48387 53169 +4782
+ Misses 19222 14440 -4782
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a5354ca to
1396ba1
Compare
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review — CRITICAL: 0, IMPORTANT: 1, SUGGESTION: 0
Full review; both changed files (pyproject.toml, tests/unit/torch/speculative/plugins/test_hf_streaming_dataset.py) reviewed, plus the surrounding install machinery (noxfile.py, uv.lock, .pre-commit-config.yaml, modelopt/torch/speculative/plugins/).
The one blocking item
uv.lock was not regenerated. The committed resolution's hf extra (uv.lock:2742-2754) and requires-dist (uv.lock:2786+) still omit httpx, so in lock-driven installs — uv sync --extra hf, and the uv run --frozen --extra dev pre-commit hooks — httpx continues to arrive only as a transitive requirement of datasets (uv.lock:945). That is the exact accident the PR description sets out to remove. It also means the next uv lock rewrites the lock in an unrelated branch, or bump_uv_lock.yml absorbs this change into a dependency-bump PR where nobody sees it. Run uv lock and commit it.
This does not affect the CI job being fixed: noxfile.py:87 installs .[dev-test] from the extras directly, so partial_unit(subset='torch') is unblocked by the importorskip alone.
What I checked and found correct
- The
importorskipfix is right and complete.partial_unit(subset='torch')installs.[dev-test]+megatron-core, sotransformersis absent too and the pre-existing guard would have skipped the file anyway — the only defect wasimport httpxsitting above it, turning a skip into a collection abort.httpx = pytest.importorskip("httpx")binds the module, and every use in the file (lines 103, 277-283, 302-324, 463-465) is an attribute access on it, including themonkeypatch.setattr(hf_streaming_dataset.httpx, "Client", ...)indirection. No other file undertests/importshttpxor transitively reacheshf_streaming_dataset, so this was the only collection-time exposure. hfis the correct extra, not coredependencies.hf_streaming_dataset.py:48importshttpxat module scope alongsidetransformers.trainer_pt_utils, so one extra already gates the whole module. Plugin laziness is intact:plugins/__init__.pynever importshf_streaming_dataset(onlymegatron_*and thehf_*modules underimport_plugin), so a core install cannot reach the import.- The example scripts are covered.
scripts/send_conversation_vllm.py:23andcollect_hidden_states/send_conversations_for_hiddens.py:23also importhttpxat module scope and theirrequirements.txtdoesn't list it — butexamples/speculative_decoding/README.md:43instructspip install -U nvidia-modelopt[hf]before the requirements file, so declaring it inhffixes those too. - The bound matches the API in use.
httpx.Client,httpx.Timeout(..., connect=...),httpx.HTTPError,httpx.MockTransportare all well within>=0.23.0, and the upper cap is consistent with the extra's existing style (transformers>=4.57,<5.15).
Risk: Low. Behaviour-neutral dependency plumbing with no source changes; the lock omission is the only thing that leaves the stated goal half-applied.
🤖 Generated with Claude Code
|
/claude review |
…absent
`partial-install (torch)` has been red on every PR since 2026-09-24, with a
collection error rather than a test failure, so it takes the whole run down:
ImportError while importing test module '.../test_hf_streaming_dataset.py'
E ModuleNotFoundError: No module named 'httpx'
Nothing in the repo changed. `hf_streaming_dataset.py` has imported httpx at
module scope since #1509 (2026-06-02) and httpx has never been declared anywhere
in pyproject.toml -- it arrived only as a transitive dependency, via
dev-test -> timm -> huggingface_hub. huggingface_hub 2.0.0, published
2026-09-24T12:01Z, replaced its `httpx<1` requirement with the separate `httpx2`
distribution, and the chain went away. Every CI run created before that timestamp
passes and every one after it fails; PRs merged later merged on a green result
from a run that predated it.
Two changes, for two different reasons.
`httpx` is now declared in the `hf` extra. It is not incidental to streaming: the
whole fetch path is HTTP (POST /v1/completions to the vLLM serve, GET /meta and
/desc against the connector's sidecar) through `httpx.Client`, there is no
non-HTTP transport -- the base `StreamingDataset._fetch` is an abstract seam and
the only implementation is that one -- and even the retry predicate is built from
`httpx.HTTPError`. It belongs in `hf` rather than the core dependencies because
that same module needs `transformers.trainer_pt_utils` at module scope, so one
extra already gates the file; core installs have no use for an HTTP client. The
bound matches the 0.x API the code uses; httpx has no 1.0 release.
The test acquires httpx through `pytest.importorskip` instead of importing it
five lines above the guard. That file already intends to skip on an install
without the extra -- it has `importorskip("transformers")` and a comment saying
why -- and transformers is absent in this job too. It only broke because one
import sat on the wrong side of the guard, where a missing module ends collection
instead of skipping one file.
Declaring the dependency is what keeps this from recurring: `[hf]` currently gets
httpx only because `datasets` happens to require it, which is the same accident
with a different supplier.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
1396ba1 to
23072e0
Compare
There was a problem hiding this comment.
Claude review (re-run on 1396ba1) — CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 2
Full review of both changed files (pyproject.toml, tests/unit/torch/speculative/plugins/test_hf_streaming_dataset.py), plus the install machinery needed to judge them: noxfile.py, uv.lock, .pre-commit-config.yaml, .github/workflows/bump_uv_lock.yml, .github/CODEOWNERS, and the httpx call sites in modelopt/ and examples/. No inline comments this round — nothing rose to blocking, and the one prior finding is re-graded below rather than re-posted.
Revising the previous review's one blocking item
The earlier run flagged the stale uv.lock as IMPORTANT. Having verified it against the repo, I'm downgrading that to non-blocking and approving. The factual claim holds — requires-dist (uv.lock:2786+) has no httpx edge under extra == 'hf', so the lock is out of sync with pyproject.toml:85 — but the impact assessment was wrong on three counts:
- No gate validates lock/pyproject sync. The only
uvinvocations in the repo areuv run --frozen --extra devin.pre-commit-config.yaml(lines 60, 66, 81, 168).--frozendeliberately neither re-resolves nor validates againstpyproject.toml— that is--locked/uv lock --check, which appear nowhere in.github/workflows/ornoxfile.py. Nothing fails. - Regenerating it would change zero installed packages.
httpxis already a locked package (uv.lock:1351) and a hard requirement of bothdatasets5.0.1 (uv.lock:941) anddiffusers0.40.0 (uv.lock:1014) — both members of thehfextra.uv lockwould add a declared edge, not a package, to anyuv sync --extra hf/--extra devenvironment. - The lock is separately owned and self-healing.
.github/CODEOWNERS:16assignsuv.lockto@NVIDIA/modelopt-setup-codeowners, andbump_uv_lock.ymlre-resolves it frompyproject.tomlevery Monday — so the next bump picks up the new direct edge on its own. Touching it here would pull an extra codeowner review onto a CI-unblocking hotfix and risk conflicting with that automation, for no functional delta.
So leaving uv.lock alone in this PR is defensible, arguably preferable. Worth knowing, not worth blocking on.
What I verified as correct
- The
importorskipfix is right, and it does not silently disable the test. This was the real bug:import httpxsat above the guard, so a missing module aborts collection instead of skipping one file.httpx = pytest.importorskip("httpx")binds the module, and all uses in the file are attribute accesses on it (lines 103, 277-283, 302-324, 463-465), including themonkeypatch.setattr(hf_streaming_dataset.httpx, "Client", ...)indirection — so the module-object binding is sufficient. Critically, the test still runs in the main unit session:noxfile.py:72installs.[all,dev-test], andpyproject.toml:143makesallincludehf, which now declareshttpx. The skip is confined topartial_unit(subset='torch')(noxfile.py:87,.[dev-test]+megatron-core), wheretransformersis absent too and the file was always going to skip. hfis the right extra, and plugin laziness is intact.hf_streaming_dataset.py:48importshttpxat module scope besidetransformers.trainer_pt_utils, so one extra already gates the module.speculative/plugins/__init__.pynever importshf_streaming_dataset, so a core install cannot reach the import.httpxhas exactly one runtime home. Across the repo only four files touch it: the module (:48,:85_TRANSIENT_FETCH_ERRORS,:441-442Client/Timeout), this test, and twoexamples/speculative_decoding/scripts — the latter covered because that README directspip install -U nvidia-modelopt[hf]. No other test importshttpxor transitively reaches the module, so this was the only collection-time exposure.- No constraint conflict.
httpxappears nowhere else inpyproject.toml, and noconstraint-dependenciesis active.
Suggestions (non-blocking)
- Stale
uv.lockedge — as above. If the setup codeowners would rather the lock trackpyproject.tomlcommit-for-commit,uv lockis the fix; otherwise the Monday bump handles it. - The
<1cap is a small resolution risk for a library. It matches house style (transformers>=4.57,<5.15) and the 0.x API in use, so it is fine as written. Butnvidia-modeloptis installed alongsidedatasets/diffusers, which requirehttpxunpinned; if either later requireshttpx>=1, the cap turns into an unsatisfiable resolution or a silentdatasetsdowngrade — the same class of upstream-shift breakage this PR is fixing, from the other direction. A barehttpx>=0.23.0would carry less of it.
Assessment
Risk: low. Two lines of dependency plumbing with no source changes; the diagnosis in the description checks out against the code, the fix lands on the actual failure mechanism (import above the guard, not the guard itself), and the test remains covered where the extra is installed. Approving.
🤖 Generated with Claude Code
|
2 participants
Summary
partial-install (torch)has been failing on every PR since 2026-09-24 — including PRs whose branches predate the breakage — and because it is a collection error rather than a test failure, it aborts the entire run:unit-pr-required-checkaggregates it, so nothing currently merges on a fresh run.What happened
No code changed.
modelopt/torch/speculative/plugins/hf_streaming_dataset.pyhas importedhttpxat module scope since #1509 (2026-06-02), andhttpxhas never appeared inpyproject.toml. It arrived only transitively:dev-test→timm→huggingface_hub→httpx.huggingface_hub 2.0.0, published 2026-09-24T12:01:21Z, replaced
httpx<1,>=0.23.0with the separatehttpx2<3,>=2.0.0distribution. Different package name, sohttpxstopped being installed and the chain disappeared.The boundary is exact — every run created before that timestamp passes, every one after fails:
#2500 merging afterwards is not a counterexample: GitHub does not re-run checks at merge time, so it merged on a result from ~20 hours earlier. That is also why this went unnoticed.
The changes
1. Declare
httpxin thehfextrahttpxis not incidental to streaming — it is the only transport:POST /v1/completionsto the vLLM serve plusGET /metaand/descagainst the connector's sidecar, all throughhttpx.Client;StreamingDataset._fetchis an abstract seam andEagleVllmStreamingDataset._fetchis its only implementation;requests/urllib/aiohttp: zero hits, andrequestsis not declared either);_TRANSIENT_FETCH_ERRORS = (httpx.HTTPError, OSError).It belongs in
hfrather than in the coredependencies: the same module needstransformers.trainer_pt_utilsat module scope, so one extra already gates the whole file, and a core install has no use for an HTTP client. The bound matches the 0.x API the code uses —httpxhas no 1.0 release, and 2.x is a different distribution.This is the part that stops it recurring.
[hf]currently getshttpxonly becausedatasetshappens to require it — the same accident with a different supplier, one release away from repeating.2. Acquire
httpxin the test through the existing skip guardThe test file already intends to skip where the extra is absent — it has
pytest.importorskip("transformers")and a comment explaining why, andtransformersis absent in this job too. It broke only becauseimport httpxsat five lines above that guard, where a missing module ends collection instead of skipping one file.Verification
hf-extra-only import precedes the firstimportorskip(which is now line 41).httpxlocally also breakshuggingface_hub1.28, whichmodelopt.torch.opt.plugins.huggingfaceimports, so the local failure is not the CI one. CI is the oracle for that half — this PR's ownpartial-install (torch)run is the check that matters.Scope
Two files, five lines of declaration and four of test import order. Deliberately not folded into any feature PR: it blocks the whole repo, and burying a repo-wide fix inside unrelated work is how these stay invisible.
🤖 Generated with Claude Code
Summary by CodeRabbit
httpx, supporting features that require HTTP communication without requiring it for all installations.httpxis unavailable, allowing the remaining test suite to be collected and run without it.