Repository navigation
ci: run the C++ and Python tests a second time on the CPU - #30
Merged
Merged
Conversation
Build with clang coverage instrumentation through CFLAGS and CXXFLAGS. Both builds in build-macos run cmake from scratch and read them. The test step writes one profile for each process and instrumented binary. After the tests, merge the profiles and report line coverage for the C++ test binary and the installed Python extension and libraries. Write the totals to the job summary and upload the full report as an artifact. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The first report counted the generated sources in the two build folders and the headers of the uv Python. Ignore every path under a build folder and the uv Python folder, so that the totals count only the mlx sources. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The job summary now shows one table with Lines, Branches and Functions for all host C++ under mlx/ and for backend/metal, backend/cpu, backend/common, distributed and io, each against the 80% target. The target is reported only. The report now also leaves out the Metal shader source in mlx/backend/metal/kernels/, the CUDA backend and the vendored pocketfft header. The step reads each value from the llvm-cov TOTAL row by its column name and fails when a value is missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The kernels exclude dropped steel/conv/params.h, a header that host C++ includes and that has coverage records. The shader source has no records, so the exclude is removed. The text says "link" instead of "run", and a comment notes that value_of rejects the "-" of a column without counters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tests run with the GPU as the default device, so the CPU code of every op with a Metal implementation does not run. A second test step sets DEVICE=cpu for the C++ and Python tests. Its profiles go to the same folder with a cpu- prefix, and the coverage report merges both runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CPU pass ran the Python tests one file after the other and did not finish in the 60-minute job limit. It now runs one process for each test file, three at a time, and each process writes its own log. The step leaves out the two tests that take more than 5 minutes on the CPU (test_sdpa_long_masked_sequence and test_sdpa_full_head_dim_256). The GPU pass still runs them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test_conv2d_winograd_batch_tiling checks that a tiny Winograd working set makes the Metal conv fall back to implicit GEMM. The CPU backend has no Winograd path and ignores the MLX_CONV_WINOGRAD_* variables, so every run is the same CPU conv and the "differs from untiled" check cannot hold. test_fft_too_large checks the size limit of the Metal four-step FFT plan. The CPU FFT uses pocketfft and has no such limit. Both tests skip only when Metal is not available, so on macOS with DEVICE=cpu they ran on the CPU. They now also skip when the default device is not the GPU. The GPU pass still runs every assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9 tasks done
# Conflicts: # .github/workflows/fork-tests.yml
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.
Proposed changes
mlx/backend/cpuhas 45.89% line coverage in the Fork Tests report (run 36587584091). The C++ and Python tests run one time, with the GPU as the default device. So the CPU code of an op that also has a Metal implementation does not run.quantized.cpp,masked_mm.cpp,scan.cpp,softmax.cpp,logsumexp.cppandfft.cppare at 0%.A new step, "Run the tests on the CPU", runs after the GPU test step in the same job. It sets
DEVICE=cpu.tests/tests.cppandpython/tests/mlx_tests.pyboth readDEVICEand set the CPU as the default device.The step runs the C++ tests (
./build/tests/tests) one time. Then it runs one process for each Python test file, three at a time (the runner has 3 cores). Each file runs in a child process that writes its own log, so a crash stops only that file. The step prints each log in its own group, and one result line for each file.The step leaves out two tests on the CPU, by a named list (
CPU_EXCLUDE) in the workflow. They take more than 5 minutes on the CPU. The GPU pass still runs them:test_fast_sdpa.TestFastSDPA.test_sdpa_long_masked_sequence: 21 min 36 s on the CPU.test_fast_sdpa.TestFastSDPA.test_sdpa_full_head_dim_256: 6 min 46 s on the CPU.Nothing else is left out. The tests that fail on the CPU run and show as failures.
The step writes its profiles to the same profiles folder, with the name pattern
cpu-%p-%m.profraw. The report step merges all profiles, so one report covers the two runs.The step fails when a test fails. A failure makes the job red. The report step still runs after a failure, as it does after a GPU test failure.
The job summary shows the result of each test step, and one new sentence says that the tests run two times.
Two Python tests check behavior that exists only in the Metal backend. They skipped only when Metal is not available, so on this macOS runner they ran on the CPU and failed. Each now also skips when the default device is not the GPU. The GPU pass still runs every assertion:
test_conv.TestConv.test_conv2d_winograd_batch_tilingchecks that a working-set budget too small for one batch element makes the Metal conv fall back from Winograd to implicit GEMM, so the output differs bit for bit from the Winograd output. The CPU backend has no Winograd path:MLX_CONV_WINOGRAD_WORKING_SETandMLX_CONV_WINOGRAD_TILE_BATCHare read only inmlx/backend/metal/conv.cpp:75and:99. On the CPU every run is the same conv, so "differs from untiled" cannot hold. Not a CPU defect.test_fft.TestFFT.test_fft_too_largechecks that the Metal four-step FFT plan throws for2**25values (mlx/backend/metal/fft.cpp:115,:148). The CPU FFT uses pocketfft (mlx/backend/cpu/fft.cpp) and has no size limit, so it gives a result. Not a CPU defect.ml-explore/mlxhas the same two tests with the samemx.metal.is_available()guard. Upstream runsDEVICE=cpuonly on Linux and Windows, where Metal is not available, so the guard skips them there.fork.yaml:test_conv.pyandtest_fft.pymove from the upstream v0.32.2 sync section (which says its files are byte-identical to upstream) to "Fork tooling", with one sentence on the CPU pass and the two skips.The distributed tests and the example extension do not run a second time. No library code changes.
Results
Fork Tests run on head ed2e48f: https://github.com/Layr-Labs/mlx/actions/runs/36778058130 (success). Before is #29's run 36587584091 on head 02720cc. Values are from the TOTAL rows in the step log.
mlx/mlx/backend/cpumlx/backend/metalmlx/backend/commonThe denominator does not change (50737 lines).
Ran 875 tests ... OK (skipped=52).test_conv2d_winograd_batch_tilingandtest_fft_too_largeran and passed.Earlier run 36752129570 (head 8aadfc1, attempt 2) had the two CPU failures that this head fixes. Its attempt 1 was cancelled at the 60-minute limit in the GPU test step, which this PR does not change: the GPU Python tests printed their result and then the process did not exit for 41 minutes.
Test plan
actionlint .github/workflows/fork-tests.yml: no findings./usr/bin/python3 scripts/check_forkdiff.py --upstream-ref refs/remotes/upstream/main: passes.CONTRIBUTING checklist
actionlintpasses on the workflow; the Python change is two guard lines per test.Dependencies
Merge this PR after #28 and #29. This branch is based on
ci/coverage-scope(#29's head, 02720cc), which is based on #28. The PR base isci/coverage-scope, so the diff shows only this change.Authorship
Written by Claude (Opus 5.5) under David Tai's direction.
🤖 Generated with Claude Code