Repository navigation
CI: run the standalone allocation cap tests in Fork Tests - #45
Merged
Merged
Conversation
tests/standalone/process_allocation_cap.cpp and process_allocation_cap_metal.cpp ran in no CI job. The process allocation cap is install-once in each process, so the tests cannot go into the tests binary. tests/CMakeLists.txt now builds them as their own executables on Metal builds and gives one ctest entry to each case of the Metal test. The build-cpp job of Fork Tests builds them in its existing build step and runs them with ctest, one fresh process for each case. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The upstream merge (ml-explore/mlx ml-explore#4634) made allocator::malloc return an owning Data and removed allocator::free. No CI job built tests/standalone/process_allocation_cap_metal.cpp, so it stopped compiling unseen: the first Fork Tests run that builds it fails with 15 errors. The test now calls allocator().malloc and allocator().free, which return and free a plain Buffer as the old functions did, the same port that the merge made in tests/residency_tests.cpp. The cases and their checks do not change. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The first CI run of the Metal cases failed 9 of 15 on the hosted runner,
which has a virtual GPU ("Apple Paravirtual device"). The allocator
makes no fixed heap there (metal/allocator.cpp), and the test assumed
one:
- install, retained, churn, foreign, gpu required heap_bytes > 0. They
now require a heap exactly when the device is not paravirtual, and
check the heap charge and the heap use against that.
- The registration cases inject a failure into the creation of a new
residency set. With the heap in set 0, the first page needs a new
set; without it, the page went into the empty set 0 and the failure
was never reached. The cases now fill one set with a page first, so
the next page needs a new set on both devices, and require that
residency sets are enabled.
The binary prints the device name and whether residency sets are
enabled, so the log shows which paths ran.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Fork Tests job 120943910531: churn failed "heap double charged" on the hosted runner. Without the fixed heap, the 8-byte buffer is a capped no-copy buffer of its own and is charged one page. The case now expects that page when the buffer is not from the heap, and waits for the credit to return before the churn loop. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
David ruled on 2026-10-24: skip by capability. The 4 registration-* cases inject a failure into the creation of a residency set. When ResidencySets::enabled() is false (no Metal 3 GPU family or macOS before 15, as on the virtual GPU of the hosted runner), the allocator registers nothing and that path does not exist. The cases then print the reason and exit with 77, which their ctest entries declare as SKIP_RETURN_CODE, so ctest reports them as skipped. On a device with residency sets they run unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Brings in mlx #44 and #46 so the CI run of this PR is on a head that contains them. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
ctest shows the output of a failed case only, so the reason that a registration case printed for its skip did not reach the log. The step now runs ctest with -V, which prints the output of every case (a few lines each). Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
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.
Summary
The standalone tests of the fork's process allocation cap (
tests/standalone/process_allocation_cap.cppandprocess_allocation_cap_metal.cpp, PR #15 / #37) ran in no CI job. This PR runs them in Fork Tests, on the macOS Metalbuild-cppjob, with no new build: the existing build step builds them, and a new step runs them withctest. The cap is install-once in each process, so each case runs in its own fresh process (one ctest entry per case).Running them found three real problems. Two are fixed here. The third (no residency sets on the hosted runner) is handled by David's ruling: skip by capability (finding 4).
Changes
tests/CMakeLists.txt(Metal builds only): executablesprocess_allocation_cap(CPU ledger, with-UNDEBUGbecause it checks withassert) andprocess_allocation_cap_metal(linksobjcfor the method interception). One ctest entry for the CPU test and one for each of the 14 Metal cases..github/workflows/fork-tests.yml, jobbuild-cpp: step "Run the standalone allocation cap tests" after the C++ CPU pass:ctest --test-dir build -R '^process_allocation_cap' --output-on-failure --no-tests=error. It writes coverage profiles like the other steps.tests/standalone/process_allocation_cap_metal.cpp: the fixes below. The case list and the checks on a device with the fixed heap and residency sets do not change.fork.yaml: the allocator section liststests/CMakeLists.txtand describes the ctest entries and the device-dependent checks; "Fork tooling" describes the new step.Findings, in CI order
The Metal test did not compile on main (
000c0149, Fork Tests job 120941291355, https://github.com/Layr-Labs/mlx/actions/runs/40117360878): 15 errors. The upstream merge (ml-explore/mlx Fix foreign buffer ownership ml-explore/mlx#4634) madeallocator::mallocreturn an owningDataand removedallocator::free. Nothing built this file, so it rotted unseen. Fix5e9ba74f:allocator().malloc/allocator().free, which return and free a plainBufferas the old functions did (the same port the merge made intests/residency_tests.cpp).The test assumed a fixed heap (
5e9ba74f, job 120942414585, https://github.com/Layr-Labs/mlx/actions/runs/40117610968): 6/15 passed. The hosted runner's GPU is "Apple Paravirtual device". On it, the Metal allocator makes no fixed heap (metal/allocator.cpp,is_vm).install,retained,churn,foreign,gpurequiredheap_bytes > 0. Fixesa7d8548a,db97e909: the cases require a heap exactly when the device is not paravirtual, check the heap charge that matches the device, and inchurnexpect one page for the small buffer when it is not from the heap. The binary prints the device name and whether residency sets are enabled.Residency sets are disabled on the hosted runner (
a7d8548a, job 120943910531, https://github.com/Layr-Labs/mlx/actions/runs/40118179184): the log showsdevice: Apple Paravirtual device, residency sets: disabled.ResidencySetsneeds the Metal 3 GPU family, which the paravirtual device does not report, soResidencySets::insertdoes nothing. The 4registration-*cases inject a failure into the creation of a residency set and check the rollback of the allocator. On this device that path does not exist. They now fail at once withresidency sets are disabled on this device. (They also fill one set with a page first, so that the injected failure is reached on a device with or without the heap in set 0.)Ruling, David 2026-10-24: skip by capability, like the 4 GiB capability skip.
cfd37c5e: eachregistration-*case checks at run timeResidencySets::enabled(), the flag that the residency code of the allocator uses (Metal 3 GPU family and macOS 15). When it is false, the case printsSKIP <case>: residency sets are disabled on this deviceand exits with 77. Their ctest entries declareSKIP_RETURN_CODE 77, so ctest reports them as skipped, not passed. On a device with residency sets they run unchanged. No runner name or CI tag is used.c3694e4a: the CI step runs ctest with-V, because ctest does not print the output of a skipped case otherwise; the log now shows the reason.ecbbe349merges main (with #44 and #46) into this branch (merge commit, no rebase).CI on the current head
Head
c3694e4a: ALL GREEN. Fork Tests https://github.com/Layr-Labs/mlx/actions/runs/40122460853 (build-cpp job 120956780754: 11 passed, 4registration-*skipped, each withSKIP <case>: residency sets are disabled on this deviceanddevice: Apple Paravirtual device, residency sets: disabledin the log), Build and Test https://github.com/Layr-Labs/mlx/actions/runs/40122460867, Fork Diff Check https://github.com/Layr-Labs/mlx/actions/runs/40122461191.Earlier, on
db97e909(before the ruling): Fork Tests https://github.com/Layr-Labs/mlx/actions/runs/40119475665, build-cpp 11/15 with the 4registration-*cases failing withresidency sets are disabled on this device. Onecbbe349(skip, before-V): Fork Tests https://github.com/Layr-Labs/mlx/actions/runs/40121592444 green, 11 passed and 4 skipped, but the skip reason was not in the log.🤖 Generated with Claude Code