Repository navigation
test(stream): resolve mps in the streamed builders and run the 32 skipped cases on MPS (#1355) - #1375
Conversation
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@Momoyeyu, thank you, and welcome to Soup. This is the PR #1355 needed, done carefully:
- You ran the 32 cases on real MPS hardware and on CUDA, and reported branch-vs-
maincounts for both. That is exactly the evidence CI cannot give us, since a macOS cell only tells us that something failed, not why. - You pushed back on the issue's own proposed fix (pinning
TrainingArgumentsto CPU for the CPU-pinned bit-exact cases) with a reason, instead of adding code no test exercises. The reason holds: I checked every CPU-pinned case (test_v07204.py:491,:509,:1182,:1195,test_issue1049_untied_streamed_preference.py:63,test_v07300.py:167,:203), none of them callstrainer.train(), and none was ever MPS-skipped. They already run and pass on GitHub's macOS runner today (all 11 cases, in #1258's CI log). - One resolver next to
cuda_availableintests/conftest.py, the three private copies gone, and the modules that re-imported a copy now import fromconftest, as the #833 rule asks. Both bitsandbytes skips and the MPS-only parity test are untouched.
What I ran (Windows, Python 3.12, torch 2.14.0+cu130, this branch merged with current main; there is no MPS here, so the macOS side rests on your M3 Ultra run and on CI):
- The six files plus
tests/test_issue833_gpu_marker.pywith the GPU hidden: 445 passed, 22 skipped, identical tomaintest by test (467 cases, no outcome differs). The 32 cases take about 15 s on CPU. - The same files with an RTX 5070 visible: 465 passed, 2 skipped on both trees, and
accelerator_device()answerscudathere. - Mutations of
tests/conftest.py:accelerator_device()always returning"cpu"fails 40 cases on the GPU box (28 of them among your 32), all on device mismatch; ignoring CUDA fails the same 40. Forcing the MPS probe toTrueon this Windows box fails 51 cases with "PyTorch is not linked with support for mps devices" and turns on the six bitsandbytes skips, so every builder and every kept skip really does read the resolver. - GitHub's macOS runner (image
macos-26-arm64) passesTestStreamedLargeLayerParityMpson MPS in #1258's CI run today, which is a good sign for the streamed training steps this PR turns on there.
One thing to coordinate: #1258. It adds tests/test_issue1174_head_prefetch_after_lookup.py, which imports _mps_is_the_accelerator from tests.test_v07204 and uses it to skip three classes. The two PRs merge without a textual conflict, but on the combined tree collection stops at:
ImportError: cannot import name '_mps_is_the_accelerator' from 'tests.test_v07204'
!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
which would fail every test cell. So whichever of the two lands second needs one more push. If #1258 lands first, please rebase this branch and convert that file like the others: import mps_is_the_accelerator from tests.conftest for its skip or, if its three classes (TestBothTimingsAreBitIdentical, TestTheInstalledDefault, TestTheLoadsMoveButDoNotMultiply) pass on your M3 Ultra, drop _NO_MPS altogether, which is what #1355 had in mind for them.
Also on macOS: besides the 32, 18 setup-only tests that use the builders' default device (for example TestNoSecondModelInstance, TestStreamRuntimeIsReleased and TestNF4EndToEndSetup::test_setup_builds_a_real_trl_trainer_under_nf4) move from CPU to MPS on the macOS cells. They passed in your six-file run, so there is nothing to do. I mention it so that a red macOS cell is read correctly.
Optional, none of these block:
-
A test for the resolver itself. Today its MPS branch only runs on the three macOS cells: putting
main's expression back intoaccelerator_device()leaves the six files green on any machine without MPS (411 passed, 22 skipped here). This test runs everywhere. On your branch it passes (5 passed), and it fails the always-"cpu"mutation (3 failures), the ignore-CUDA one (2) and the revert tomain's expression (1). The #833 guard still passes with it in the tree, and a streamed train step run after it in the same pytest run still lands oncuda, so itscache_clear()leaves no stale answer behind. For example, astests/test_issue1355_accelerator_device.py:"""#1355 — the streamed builders build on the device ``TrainingArguments`` picks by itself. Pinned without a Mac or a card: otherwise the MPS branch of the resolver only ever runs on the macOS cells, and the CUDA branch only on a GPU box. """ from __future__ import annotations from types import SimpleNamespace import pytest from tests import conftest @pytest.fixture() def fake_accelerators(monkeypatch): """Fake ``torch.cuda`` and ``torch.backends.mps``; both probes are cached, so clear them.""" import torch def apply(*, cuda: bool, mps: bool | None) -> None: monkeypatch.setattr(torch.cuda, "is_available", lambda: cuda) backend = None if mps is None else SimpleNamespace(is_available=lambda: mps) monkeypatch.setattr(torch.backends, "mps", backend, raising=False) conftest.cuda_available.cache_clear() conftest.mps_is_the_accelerator.cache_clear() yield apply # Runs before monkeypatch restores torch, so the next caller re-probes the real one. conftest.cuda_available.cache_clear() conftest.mps_is_the_accelerator.cache_clear() @pytest.mark.parametrize( ("cuda", "mps", "expected"), [ (True, True, "cuda"), (True, False, "cuda"), (False, True, "mps"), (False, False, "cpu"), (False, None, "cpu"), # a torch build with no torch.backends.mps at all ], ) def test_the_builders_device_is_cuda_then_mps_then_cpu(fake_accelerators, cuda, mps, expected): fake_accelerators(cuda=cuda, mps=mps) assert conftest.accelerator_device() == expected assert conftest.mps_is_the_accelerator() is (expected == "mps")
-
tests/test_v07202.py:34-37: the NF4skip_on_mpsthat stays says "MPS is untested for NF4 streaming (measured on CUDA + CPU only)", which reads like the 32 skips this PR removes. Its real reason, that bitsandbytes has no 4-bit MPS kernels, lived in the docstring of the deleted helper. A one-line comment would stop a future cleanup from removing it. -
The docstrings at
tests/test_v07204.py:238and:353still name only two streaming dtypes, bf16 on CUDA and float32 on CPU. On MPS it is bf16 when the runtime accepts it, float32 otherwise. -
tests/test_issue975_backward_tail_embed_prefetch.py:62-63: two blank lines are left where the class-levelpytestmarkwas.
No changelog fragment is needed, since this only touches tests. The required checks were still queued when I looked, and the three macOS cells are the real check for the 32. Apart from the #1258 ordering above, this now waits only on those checks.
|
@Momoyeyu, #1258 is merged ( |
…sses on MPS, and address MakazhanAlpamys#1375 review
9846a19 to
067e314
Compare
|
Thanks for checking all of this, including the mutations. #1258 landed first, so I rebased on Also in the new commit:
On the M3 Ultra, the six files, the #833 guard, #1174 and the new test give 481 passed, 27 skipped. ruff is clean. |
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@Momoyeyu, thank you, that was quick and thorough. The file from #1258 is converted the better of the two ways: you ran its three classes on the M3 Ultra first and then dropped _NO_MPS rather than re-pointing it, which is what #1355 had in mind for them. The resolver test and all three smaller points are in, and the new dtype docstrings describe what layer_stream.py actually does on MPS.
Some good news from CI, too. The test (macos-latest, 3.11) cell of the run on your previous push finished green. That push has the same first commit, so its log is a real GitHub macOS runner result. All 32 cases passed there, along with the 18 setup tests that now build on the default device. The MPS parity test ran, which confirms MPS was the accelerator on that runner, and the six bitsandbytes cases skipped as they should.
What I ran (Windows, Python 3.12, torch 2.14.0+cu130 with the GPU hidden, on this branch alone and merged with current main):
-
The six files, the #833 guard,
test_issue1174_head_prefetch_after_lookup.pyand the new test: 485 passed, 23 skipped on both trees, identical test by test.test_issue1174_head_prefetch_after_lookup.pyitself gives 35 passed, 1 skipped, so the collection error from my earlier review is gone. Collecting the wholetests/tree on the merged tree also finishes with no error. Onmainthe same files give 480 passed, 23 skipped, and the only difference is the five new cases. -
tests/test_issue1355_accelerator_device.pyfails onmain(5 failures and 5 errors,module 'tests.conftest' has no attribute 'mps_is_the_accelerator'). Here it catches every resolver mutation I tried:- always returning
"cpu": 3 failures - ignoring CUDA: 2
main's expression, i.e. the MPS branch dropped: 1- the MPS probe ignoring CUDA: 1
Before, the third one could only be caught on a Mac.
- always returning
-
Forcing the MPS probe to
Trueon this machine, which has no MPS, fails 67 cases. 65 of them fail with "PyTorch is not linked with support for mps devices", including all 14 of #1174's cases. The six bitsandbytes skips also turn on, so every builder, #1174's included, goes through the resolver. -
The fixture's cleanup: one pytest run of the resolver's
cuda=Truecase followed by two real streamed train steps gives 3 passed. With the teardown'scache_clear()calls removed, both steps fail with "No CUDA GPUs are available". So the fixture leaves nothing stale behind for later tests.
Nothing else is needed from you. GitHub holds workflow runs from a first-time contributor until a maintainer approves them, and it asks again on every push, so the runs for this push are waiting on that. After that, this PR waits only on the required checks. The macOS cells will be the first GitHub run of #1174's three classes on MPS.
|
Thank you — a pleasure working with you on this one. Looking forward to the next. :) |
|
Merged as |
Closes #1355.
The streamed test builders resolved their device as
"cuda" if cuda_available() else "cpu"(onmain:tests/test_v07204.py:378,tests/test_v07200.py:2501,tests/test_v07202.py:1832,tests/test_issue975_backward_tail_embed_prefetch.py:53). On Apple Silicon the model was built on CPU whileTrainingArgumentspickedmps:0, so the firsttrain()step failed on a device mismatch, and 32 cases carriedskipif(_mps_is_the_accelerator(), reason="MPS is untested for layer streaming ...").What does this PR do?
tests/conftest.py:mps_is_the_accelerator()(cached,Falsewhen CUDA is present) andaccelerator_device(), next tocuda_available.accelerator_device()returns the deviceTrainingArgumentspicks by itself: cuda, else mps, else cpu.accelerator_device()._mps_is_the_acceleratorcopies (test_v07200.py,test_v07202.py,test_v07204.py) are deleted, and the three modules that re-imported one (test_issue975_...,test_issue1011_...,test_issue1049_...) import fromconftest, so no test module probes the device itself (~51 CUDA-only tests have no marker, no documented way to run them and no results record; both known runs on real cards found defects (#776, and 9 failures from three defects on H100) #833).test_v07204.py11 (test_loss_matches_a_resident_run_of_the_same_loss×4,test_layer_zero_adapter_receives_gradient×4,test_kto_streams_at_batch_two,test_a_reference_using_loss_reads_more_layers_than_sft×2),test_issue1011_streamed_save_guard.py5,test_issue1049_untied_streamed_preference.py10,test_issue975_backward_tail_embed_prefetch.py5 (class-level),test_v07200.py1 (test_one_training_step_actually_runs).test_v07202.py'sskip_on_mps, now backed by the conftest helper, andtest_v07204.py's "bitsandbytes has no 4-bit MPS kernels"), and thenot mps_is_the_accelerator()mark onTestStreamedLargeLayerParityMps.No case needed a strict xfail: all 32 pass on MPS.
No CPU pin for
TrainingArguments. I tried pinning the CPU-pinned cases' trainer to CPU (ACCELERATE_USE_CPU) and took it out again: with the pin disabled, all 441 tests in the six files still pass on MPS. None of thedevice="cpu"bit-exact cases reaches aTrainingArgumentsstep, so the pin would have been code no test exercises. If one ever does, it will fail loudly on macOS with the device-mismatch message rather than pass silently.Verification
gpumarkers and the two bitsandbytes skips). Full suite: 29080 passed / 3 failed / 221 skipped on this branch against 29047 / 4 / 253 onmainat the same commit. The skip count drops by exactly 32, and there are no new failures (the 3 failures also fail onmain:soup data forge --judge-providerwrites an empty-answer row for every failed judge call and still reports "synth complete" #1221 forge judge, Feature proposal: resumable offline AutoDistill pipeline with adaptive data generation #580 MLX venv launcher,test_lambda_submit_interrupt).pytest -m gpuover the six files gives 20 passed both on this branch and onmain, so the CUDA path is unchanged (accelerator_device()returnscudathere, as the old expression did). One failure in the six files,TestShardCacheIdentityBinding::test_changed_source_weights_invalidate_the_cache, is intermittent and not from this change: on the same machine it passes 3/3 in isolation on this branch and fails in a whole-file run onmain.Type of change
Checklist
ruff check src/soup_cli/ scripts/ tests/passespytest tests/ -vpasses, apart from the three failures above that also fail onmain(Mac Studio full run)README.mdand the matching page underdocs/) if needed: not needed, test-only