Repository navigation
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
This was referenced Sep 23, 2026
Fridah-nv
added this pull request to stack #2538
September 23, 2026 21:57
Fridah-nv
force-pushed
the
fridah/algo-cfg-2-write-mask
branch
from
September 23, 2026 22:02
dddf932 to
4178782
Compare
Contributor
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fridah/algo-cfg-1-capabilities #2535 +/- ##
==================================================================
+ Coverage 76.26% 76.30% +0.03%
==================================================================
Files 609 609
Lines 69311 69390 +79
==================================================================
+ Hits 52862 52945 +83
+ Misses 16449 16445 -4
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:
|
kevalmorabia97
pushed a commit
that referenced
this pull request
Sep 25, 2026
…y PR (#2547) ## 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: ``` ImportError while importing test module '.../tests/unit/torch/speculative/plugins/test_hf_streaming_dataset.py' E ModuleNotFoundError: No module named 'httpx' collected 2243 items / 1 error / 45 skipped !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!! ``` `unit-pr-required-check` aggregates it, so nothing currently merges on a fresh run. ## What happened **No code changed.** `modelopt/torch/speculative/plugins/hf_streaming_dataset.py` has imported `httpx` at module scope since #1509 (2026-06-02), and `httpx` has never appeared in `pyproject.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.0` with the separate **`httpx2<3,>=2.0.0`** distribution. Different package name, so `httpx` stopped being installed and the chain disappeared. The boundary is exact — every run *created* before that timestamp passes, every one after fails: | PR | run created | result | |---|---|---| | #2536 / #2535 | 09-23 22:02 | pass | | #2500 | 09-23 23:45 | pass — **merged 09-24 20:01 on this stale-green result** | | *hub 1.33.0 (still requires httpx)* | *09-24 09:49* | | | **hub 2.0.0 published** | **09-24 12:01** | ← | | #2539 | 09-24 16:57 | fail | | #2544 | 09-24 18:32 | fail | | #2216 | 09-25 11:58 | fail | #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 `httpx` in the `hf` extra `httpx` is not incidental to streaming — it is the only transport: - every fetch is HTTP: `POST /v1/completions` to the vLLM serve plus `GET /meta` and `/desc` against the connector's sidecar, all through `httpx.Client`; - there is no non-HTTP path — the base `StreamingDataset._fetch` is an abstract seam and `EagleVllmStreamingDataset._fetch` is its only implementation; - no other HTTP library appears in the module (`requests` / `urllib` / `aiohttp`: zero hits, and `requests` is not declared either); - even the retry predicate is built from it: `_TRANSIENT_FETCH_ERRORS = (httpx.HTTPError, OSError)`. It belongs in `hf` rather than in the core `dependencies`: the same module needs `transformers.trainer_pt_utils` at 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 — `httpx` has no 1.0 release, and 2.x is a different distribution. This is the part that stops it recurring. `[hf]` currently gets `httpx` only because `datasets` happens to require it — the same accident with a different supplier, one release away from repeating. ### 2. Acquire `httpx` in the test through the existing skip guard The test file already intends to skip where the extra is absent — it has `pytest.importorskip("transformers")` and a comment explaining why, and `transformers` is absent in this job too. It broke only because `import httpx` sat **five lines above** that guard, where a missing module ends collection instead of skipping one file. ## Verification - With everything installed: **18 passed**, no behaviour change. - The import-order property is checked with an AST walk over the module's top-level statements: no `hf`-extra-only import precedes the first `importorskip` (which is now line 41). - A faithful local reproduction was attempted and abandoned honestly: hiding `httpx` locally also breaks `huggingface_hub` 1.28, which `modelopt.torch.opt.plugins.huggingface` imports, so the local failure is not the CI one. CI is the oracle for that half — this PR's own `partial-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](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Optional Hugging Face installations now include `httpx`, supporting features that require HTTP communication without requiring it for all installations. * **Tests** * Hugging Face streaming dataset tests now skip when `httpx` is unavailable, allowing the remaining test suite to be collected and run without it. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fridah-nv
force-pushed
the
fridah/algo-cfg-2-write-mask
branch
2 times, most recently
from
September 28, 2026 21:03
4b518ad to
ce28182
Compare
…scope Calibration algorithms take an optional `should_process` predicate and AND it into their existing `is_enabled` filter, so an algorithm can be restricted to part of the model. `None` means the whole model, which is today's behaviour, so nothing changes for any existing caller. Three properties matter and each has a test: - It gates *writes* only, never enable-state. Reads are untouched, so the activations a search-based algorithm sees are identical either way. - It keys on module *identity*, not name, so it survives `layerwise_calibrate` handing an algorithm a reparented subtree whose module names are relative. - It has to reach helper calls *inside* an algorithm, not just its top-level module loop. `awq_lite` calls `enable_stats_collection(model)` directly and would otherwise calibrate the whole model regardless of scope. No caller passes a mask yet; the plan that does arrives in a later change. Note for reviewers: the guards in the `max_calibrate` path are mutually redundant -- removing any one of `enable_stats_collection`, `weight_only_quantize` or `finish_stats_collection` leaves the others covering it, so no single-guard mutation is observable. The test detects a real leak (5 of 7 cases fail when all three are removed), but redundancy is why it cannot pin any one of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fridah-nv <201670829+Fridah-nv@users.noreply.github.com>
Fridah-nv
force-pushed
the
fridah/algo-cfg-2-write-mask
branch
from
September 28, 2026 22:19
ce28182 to
7324291
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brief
What changes: Scoped calibration needs algorithms to write only their targets yet read the whole model; this enables that. Nothing changes for existing callers.
How: Adds
should_processto every calibration algorithm's public API — 8 algorithms and 3 helpers — so a stage writes only to its targets. It is ANDed into each algorithm's existingis_enabledfilter, defaults toNonemeaning the whole model, and never touches enable-state, so reads are unaffected.What does this PR do?
Type of change: new feature (dormant) + bug fix
Stack 2 of 4, splitting #2292. Base: #2534.
algo_cfgA write-mask so calibration can be confined to a scope
Calibration algorithms take an optional
should_processpredicate and AND it into their existingis_enabledfilter, so an algorithm can be restricted to part of the model.Nonemeans the whole model, which is today's behaviour, so nothing changes for any existing caller.Three properties matter, each with a test:
layerwise_calibratehanding an algorithm a reparented subtree whose module names are relative.awq_litecallsenable_stats_collection(model)directly and would otherwise calibrate the whole model regardless of scope.Reviewing this
One file of library change, and the question is narrow: is every write site gated? That is exactly the question the
awq_litebug above failed, which is why this is its own PR rather than buried in the plan machinery.No caller passes a mask yet — the plan that does arrives in PR 4. This is deliberate: the threading is mechanical and error-prone, the plan is neither, and mixing them makes both harder to check.
A caveat the tests cannot cover
The guards in the
max_calibratepath are mutually redundant. Removing any one ofenable_stats_collection,weight_only_quantizeorfinish_stats_collectionleaves the others covering it, so no single-guard mutation is observable. The test detects a real leak — 5 of 7 cases fail when all three are removed — but redundancy is why it cannot pin any one of them individually.Found while mutation-testing this PR's own test, which also turned up a genuine gap: the first version only snapshotted amax, so removing GPTQ's guard changed nothing detectable, because GPTQ writes weights. The test now snapshots amax, weights and
pre_quant_scale.Testing
New
tests/unit/torch/quantization/test_calib_write_mask.py— scope confinement formax/mse/awq_lite/gptq, the weight-only (no forward loop) path, and enable-state invariance.pytest tests/unit/torch/quantization/→ 1135 passed, 8 skipped, plus the same two pre-existingtest_huggingface.pyfailures and one pre-existing collection error described in #2534.Before your PR is "Ready for review"
CONTRIBUTING.md: N/A🤖 Generated with Claude Code