test: cover _build_source(pin=False, tier=ram) on CPU-only CI - #653
Conversation
…anAlpamys#623) The two deepest hops below the runtime boundary are CUDA-gated (TestTheAllocationItself here, TestRequirePinSurvivesEveryHop in test_v07203.py), so the RAM tier's unpinned path was unprotected on all nine CI cells. Add a direct, unmocked call to _build_source(pin=False, tier="ram") against a real one-layer shard; its "if not pin:" branch never requests page-locked memory, so no CUDA device is needed.
MakazhanAlpamys
left a comment
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
Approved — this closes exactly what I left #623 open for, and the mutation that matters dies.
I verified the head (52f85be4), then ran the two hops this is supposed to protect:
| mutation | result |
|---|---|
if not pin: → if False: in _build_source (:2212) |
KILLED — test_build_source_pin_false_ram_tier_yields_unpinned_ramsource |
pin_memory=self.pinned → pin_memory=True in RamSource (:331) |
KILLED — same test |
The second one is the point. Before #647 that mutation left 169 pre-existing streaming tests green, and after #647 it was caught only behind a CUDA gate. It now dies on a host with no GPU, which is what the issue asked for.
source.get(0, "weight").is_pinned() is False is why it works — you assert the allocation, not just source.pinned. An attribute assertion would have passed the deeper mutation happily, and that distinction is the whole reason the gap existed in the first place.
Driving the real, unmocked _build_source with a genuine safetensors shard on disk, rather than a mock, is the right call and it costs nothing: the unpinned branch never requests page-locked memory, so there is no device requirement to work around.
One structural limit, worth naming rather than fixing
The mirror direction is not covered and cannot be on a CPU-only cell. I mutated pin_memory=self.pinned → pin_memory=False — a "never pin" implementation:
TestBuildSourceCpuOnlyPinFalse: 1 passed <- survives
TestTheAllocationItself: 1 failed <- caught, but CUDA-gated
So a regression that silently stopped pinning altogether is still invisible on the nine CPU cells. That is not a defect in this PR: you cannot allocate pinned memory without CUDA, so the pin=True direction is untestable there by construction. I am recording it so nobody later reads this file as full coverage of both directions — the pin-requested direction remains CUDA-only, and TestRequirePinSurvivesEveryHop in test_v07203.py is where it lives.
Checks
- The new test passes standalone. 14/14 CI green including
pytorch-smoke— you are the first PR to land after I made that required, so it actually reported. ruffclean.- One removed line, and it is not a test:
Hops BELOW the runtime boundary are not re-tested here:→Most hops BELOW.... Correcting a docstring that your own change makes false is the detail I would have let slide. - No changelog fragment, correctly —
CONTRIBUTING.mdscopes those to user-visible changes and this is tests-only.
One note on my own run so it is not mistaken for a finding: three pre-existing tests in that file fail on my box with ImportError: huggingface-hub>=0.34.0,<1.0 is required ... found huggingface-hub==1.30.0. I checked the same file on main and it fails identically, so it is my local environment and not your branch. CI is green on all nine cells.
Merging shortly. Thank you for picking up a gap I had said I would file rather than waiting for the issue to appear.
Records why the test reaches the hop that survived 169 tests: it asserts the allocation via is_pinned(), not the recorded .pinned attribute, which is how the gap survived. Also records that they named the structural limit -- the mirror never-pin direction cannot be tested without a device -- rather than implying full coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merged as Credited specifically for the thing that makes it work rather than for the test existing: asserting Re-ran both hops before merging: You also did the two things I notice most: picked this up from the issue thread instead of waiting for me to file it, and named the structural limit rather than implying full coverage. I confirmed that limit is real — mutating Separately: thank you for the resident-RAM accounting gap you found on #644. That is now |
|
Good catch on the docstring line, that assertion change did make the old wording wrong. And thanks for actually running the pin_memory=False mutation and writing down that it's untestable on CPU rather than treating the gap as covered, that's the part I'd have wanted a reviewer to check. |
…658) Covers the one uncovered statement in `config/unknown_keys.py` — the `if not isinstance(key, str): continue` guard shipped in #628 by the same author. No source change; the guard is correct, only its coverage was missing. Found by measurement rather than by eye: a `--cov-branch` run over the full suite (19935 passed, 108 skipped, 81%) put the module at 97% with exactly one uncovered statement, line 118. Nothing in `tests/test_issue627_unknown_config_keys.py` had ever fed the walk a non-string key. Why the guard matters: YAML permits non-string mapping keys, and without it the key reaches `difflib.get_close_matches`, which iterates it — `TypeError: 'int' object is not iterable`. A config carrying `1:` or `true:` under any section would kill `soup train --config` with a bare TypeError raised from inside the unknown-key reporter, i.e. the code whose entire purpose is to turn a confusing failure into an actionable one. Six tests. The two that carry the weight assert the walk KEEPS GOING: a typo'd key placed after a non-string key is still reported with its suggestion, at the top level and one section down. A "does not raise" test alone would have passed an early-`return` rewrite that silently stops reporting every key after the first non-string one. Mutation-verified independently at `401fa79`, both directions: guard removed entirely pre-existing 45 passed (SURVIVED) / new 4 failed (KILLED) continue -> return pre-existing 45 passed (SURVIVED) / new 2 failed (KILLED) so "this line was uncovered" is measured, not asserted. Refs #627 — the issue stays open. #628 shipped phase 1 (warn); `UNKNOWN_KEY_REJECTION_VERSION = "0.75"` and `TestTheDeadline` hold the rejection phase, and that is what closes it. No changelog fragment: test-only, no user-visible change, matching #647 and #653.
…657) Closes both follow-ups the #654 merge comment recorded as untested. No source change; this is coverage for branches that shipped without any. 1. The graceful-degradation path. `_probe_budget_bytes` returns None when the driver cannot answer `mem_get_info`, and `_probe` then returns True — deliberately keeping the pre-#649 exception-only criterion rather than inventing a budget. That branch was unreachable from the existing suite because its fake is `lambda *a, **k: (free_before, total)`, which cannot fail. The new fake raises, and reports a 999 GB peak — larger than any budget could be — so a True can only have come from the `budget is None` path. A control with the identical peak and a working `mem_get_info` returns False, so the test cannot pass for the wrong reason. 2. The `AcceleratorError` contract. `_is_cuda_oom` never names the class; it catches it via `isinstance(exc, RuntimeError)`. The existing `_FakeAcceleratorError` also subclasses RuntimeError, so that test passed whether or not the real one does. The new test pins the real upstream class where the resolved torch has it, and skips with a stated reason where it does not — rather than passing vacuously. The third mutation below is beyond what #654 asked for and is the one I value most: nothing previously pinned the NARROWNESS of `_is_cuda_oom`. It could have been widened to `Exception` with no test objecting, which would swallow the illegal-access and device-assert errors its own docstring says must propagate. Mutation-verified independently at `9604eb2`, torch 2.5.1: budget-is-None: return True -> False pre-existing 10 passed (SURVIVED) / KILLED _is_cuda_oom: RuntimeError -> Exception pre-existing 10 passed (SURVIVED) / KILLED The author declined to rewrite `_is_cuda_oom` to match `AcceleratorError` by name, correctly treating a behaviour change as the maintainer's call, and stated the consequence instead. Known environment dependency, disclosed in the PR rather than found in review: the contract guard only has teeth when the resolved torch is >= 2.8. On this box (2.5.1) it skips with a reason. `train = ["torch>=2.5.0"]` is a floor and not a pin, so CI resolves a recent wheel and the guard executes — but a future pin below 2.8 would silently turn it into a permanent skip, and a skipped test is the same colour as a passing one. No changelog fragment: test-only, no user-visible change, matching #647 and #653.
2 participants
What does this PR do?
Fixes #623: a CPU-only assertion that
_build_source(pin=False, tier="ram")returns aRamSourcewith.pinned is False, the one thing you left the issue open for after #647 landed._build_source'sif not pin:branch (src/soup_cli/utils/layer_stream_runtime.py:2212-2213) is the only branch that is never CUDA-gated, and neither existing test drives it with a real, unmocked construction:TestRequirePinSurvivesEveryHop(test_v07203.py) only builds aRamSourcesubclass that fails whenpin=True, and this file's ownTestTheAllocationItselfis@requires_cuda. The new test calls_build_sourcedirectly against a real one-tensor shard and checks.pinnedand.get(0, "weight").is_pinned()on the constructedRamSource, no CUDA needed sincetorch.empty(pin_memory=False)works on any host.Type of change
Verification
ruff check src/soup_cli/ tests/test_issue623_stream_pin_allocation.py: clean.torch+safetensors+numpy(the module's own lazy imports), so this did not exercisepytest tests/in full; the touched file's other classes need the[train]stack, which I did not install.Checklist
ruff check src/soup_cli/ scripts/ tests/passespytest tests/ -vpasses (scoped run above; see note)