Skip to content

Retire legacy-Accelerate accommodations: macOS runs the full library - #155

Merged
mmelnich merged 4 commits into
mainfrom
retire-legacy-accelerate
Aug 27, 2026
Merged

mmelnich merged 4 commits into
mainfrom
retire-legacy-accelerate

Conversation

@mmelnich

@mmelnich mmelnich commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ Upstream requirement met (2026-08-27)

The gate this PR was waiting on has cleared: icl-utk-edu merged the new-Accelerate support in both BLAS++ (icl-utk-edu/blaspp#134) and LAPACK++ (icl-utk-edu/lapackpp#88) on 2026-08-27. core-macos builds its dependencies from the icl-utk-edu masters, so it now exercises the new interface for real.

Refreshed 2026-08-27, same scope plus:

What this PR does

Makes macOS a full-library, first-class platform by removing all 18 __APPLE__ guards, and switches macOS CI to Apple's new Accelerate interface.

Background. Apple ships two LAPACKs in Accelerate. The legacy default is 3.2.1 (2009): gesdd returns wrong results on Apple Silicon (the reason core-macos has carried a deliberately red canary test), and routines RandLAPACK needs (orhr_col, geqrt, ...) don't exist — the reason BQRRP threw "not supported" on every Mac and ten benchmarks were stubbed out. The new interface (macOS ≥ 13.3, ACCELERATE_NEW_LAPACK, LAPACK 3.12 on current SDKs) fixes both, verified on this repo's CI: run 31065130007 — 311/311 tests pass on macOS, including the gesdd canary, all of TestBQRRP (first macOS execution ever), and test_orhr_col.

Changes:

  • Drivers: BQRRP's runtime "not supported on Apple" throw removed; HQRRP's CholQR-path stub removed; CQRRPT's BQRRP-subroutine path enabled on macOS.
  • Tests: test_bqrrp (whole file), the guarded sections of test_cqrrpt/test_hqrrp/test_bqrrp_gpu, and test_orhr_col now compile everywhere — macOS gains 16 tests.
  • Benchmarks: the ten "cannot run on Apple machines" stub mains removed; real benchmark code everywhere.
  • CI: core-macos builds BLAS++ with -Dblas=accelerate and hard-fails if the new interface wasn't selected (no silent legacy fallback). The canary test is included and expected green — the canary era ends with this PR.
  • Docs: INSTALL.md documents the macOS requirement (new interface or Homebrew OpenBLAS; install.sh on macOS uses OpenBLAS and is unaffected). docs/CI.md replaces the quarantine entry with the new-Accelerate arrangement.

Follow-up, not in this PR: advancing the pinned BLAS++/LAPACK++ commits in install/install.sh and the Windows CI provisioner, allowing Accelerate ILP64, and re-evaluating the macOS installer default are tracked in #165 (RandLAPACK) and BallisticLA/RandBLAS#189.

mmelnich added a commit that referenced this pull request Aug 12, 2026
…ark's BLAS++/LAPACK++ review) (#157)

> [!WARNING]
> **This is a temporary change and is meant to be reverted.**
> It is one clean `git revert` of a single commit.

### Revert when these land

The quarantine exists only until Apple's new Accelerate interface is in
use. Both are open and awaiting review:

| PR | Status |
|---|---|
|
[icl-utk-edu/blaspp#134](icl-utk-edu/blaspp#134)
— New Apple Accelerate support (#74, rebased and completed) | open |
|
[icl-utk-edu/lapackpp#88](icl-utk-edu/lapackpp#88)
— Support Apple's new Accelerate interface (closes #43) | open |

**Order matters:** lapackpp#88 depends on the `defines.h` from
blaspp#134, so #134 merges first. blaspp#134 is a rebased and completed
continuation of the stalled
[blaspp#74](icl-utk-edu/blaspp#74).

Once both are merged,
[RandLAPACK#155](#155)
(retire legacy-Accelerate accommodations) becomes mergeable and
`TestQB.Polynomial_Decay_general1` should pass on its own — at which
point the warning added here fires and tells you to delete this
quarantine.

## Why

`TestQB.Polynomial_Decay_general1` fails on Apple Silicon because
Apple's old Accelerate LAPACK has a broken divide-and-conquer `gesdd`.
It has been left failing deliberately, as a canary for the planned
migration.

The side effect is that **`core-macos` has been permanently red** —
`build` and `build-asan`, on `main` and on every branch. A lane that is
always red carries no information: you cannot distinguish a new macOS
regression from the known one, so the job gets ignored and the canary
signals to nobody.

## What this does

Quarantines the test without discarding the signal:

- The main `ctest` run excludes it, so the job reflects real macOS
health.
- The test is then **still run**, separately, and cannot fail the job.
- Every macOS run prints a warning that the suppression is active and
names the condition for removing it.
- If the test ever **passes**, the job prints a louder warning saying to
delete the quarantine.

That last point matters: the migration signal is preserved and actually
improved. Previously it was "a red job turns green", which nobody was
watching because the job was always red. Now it is an explicit
annotation telling you what to delete.

## Scope

Deliberately surgical — `core-macos.yaml` (both jobs) and the
corresponding `docs/CI.md` entry, nothing else, so the revert stays
clean. The dead `TestABRIK.ABRIK_catch_instability` exclusion in these
same lines is left alone for that reason.
Apple's legacy Accelerate LAPACK (3.2.1, 2009) breaks gesdd on Apple
Silicon and lacks orhr_col/geqrt & co., which forced 18 __APPLE__
guards into the codebase: BQRRP threw 'not supported' on every Mac,
HQRRP's CholQR path was stubbed, CQRRPT could not delegate to BQRRP,
test_bqrrp and test_orhr_col were compiled out, and ten benchmarks were
stub mains. Apple's new interface (ACCELERATE_NEW_LAPACK, macOS >= 13.3,
LAPACK 3.12 on current SDKs) fixes all of it -- verified end to end:
311/311 tests pass on macOS including the gesdd canary, all of
TestBQRRP, and test_orhr_col.

Remove every guard; switch core-macos to the new interface
(-Dblas=accelerate + a hard defines.h check so a silent fallback to the
legacy interface fails the job); document the macOS requirement in
INSTALL.md. Requires BLAS++/LAPACK++ with new-Accelerate support, so
this stays unmerged until icl-utk-edu merges the two PRs from
BallisticLA:new-accelerate -- core-macos here is deliberately red until
then and flips green by itself when upstream lands.
# Conflicts:
#	.github/workflows/core-macos.yaml
@mmelnich
mmelnich force-pushed the retire-legacy-accelerate branch from d9e6c3e to e553cd1 Compare August 27, 2026 14:11
@mmelnich
mmelnich marked this pull request as ready for review August 27, 2026 14:30
@mmelnich

Copy link
Copy Markdown
Contributor Author

CI on the refreshed branch, first run against the merged upstream masters:

  • core-macos build (Release): green. The un-quarantined TestQB.Polynomial_Decay_general1 passes on Apple's new Accelerate, along with the full suite including TestBQRRP. This is the flip the PR was designed around.
  • core-linux (both jobs), install-linux/macos/windows, and all Windows legs: green.
  • core-macos build-asan (Debug/asan, not a required check): one failure, TestBQRRP.BQRRP_half_zero_luqr, and it is numerical rather than a sanitizer report (ASAN is silent). The factorization itself is fine (relative residual 5.7e-14, orthogonality 7.4e-15, rank 1000/1000); the max-col-norm pivot-quality metric lands at 1.06e-12 against the eps^0.75 = 1.03e-12 bound, a 3% overshoot (test_bqrrp.cc:105, actual 8.242e-11 vs 8.014e-11). The same test passes in the Release job on the same runner and BLAS, so the difference is Debug codegen in RandLAPACK's own kernels, not Accelerate misbehavior. This is also the first Debug/asan execution of TestBQRRP on macOS ever, so the tolerance has never been calibrated for this configuration.

I have deliberately not touched the tolerance: atol = eps^0.75 is shared by every BQRRP test and carries no dimension factor, so widening it is a test-design decision. Options: (a) add a modest constant or dimension factor to the pivot-quality bound, (b) leave it and treat this as a known-marginal configuration until the asan lanes become required. Happy to implement either.

The max-col-norm metric compares the largest residual column of A - QR
against the same column of the original A. The residual loop measures
full columns, nrm2(m, ...), but the reference used nrm2(n, ...), so for
tall matrices the denominator only covered the first n of m entries and
the bound atol * ||A_j|| was too tight by about sqrt(m/n). On a wide
matrix the same line reads past the end of the last column. This is what
failed core-macos build-asan on BQRRP_half_zero_luqr (5000x2000): the
metric landed 3% over a bound that was 58% too small.

Same copy-pasted helper fixed in all six test drivers.
@mmelnich

Copy link
Copy Markdown
Contributor Author

Root cause of the build-asan failure found, and it is a test bug, not a tolerance problem — 08a36b8 fixes it, so the options in my previous comment are moot.

The max-col-norm metric measures the largest residual column with its full length, nrm2(m, ...), but the reference denominator used nrm2(n, ...) — only the first n of m entries of that column of the original A. For tall matrices that under-measures ||A_j|| by about sqrt(m/n), so the check enforced a bound roughly 58% tighter than the intended ||(A - QR)_j|| <= atol * ||A_j|| on the 5000x2000 test. The asan run's 3% overshoot was against that too-tight bound; with the correct denominator it passes with ~1.5x margin. As a bonus, on a wide matrix (m < n) the old line reads past the end of the last column of the copy.

The same copy-pasted helper exists in six test drivers (test_bqrrp, test_hqrrp, test_cqrrpt, test_cqrrt, test_bqrrp_gpu, test_cqrrpt_gpu); all six are fixed. atol = eps^0.75 is untouched. Linux/MKL Release: 772/772 after the change.

@mmelnich
mmelnich merged commit c593aac into main Aug 27, 2026
12 checks passed
@mmelnich
mmelnich deleted the retire-legacy-accelerate branch August 27, 2026 15:03
mmelnich added a commit that referenced this pull request Aug 27, 2026
…#172)

## Summary

Follow-up to #155, which retired the legacy-Accelerate accommodations in
the library and CI. This PR does the installer half: with new-Accelerate
support merged upstream on 2026-08-27
([icl-utk-edu/blaspp#134](icl-utk-edu/blaspp#134),
[icl-utk-edu/lapackpp#88](icl-utk-edu/lapackpp#88)),
the reasons macOS defaulted to Homebrew OpenBLAS — the legacy `gesdd`
bug, the missing routines, the missing ILP64 — are gone. The default
macOS build now uses Accelerate through Apple's new interface and needs
no Homebrew BLAS. (ILP64 turned out to be blocked upstream — see below.)

Closes #165 except its ILP64 item, which is split into #173: this PR's
own install-macos lane discovered that LAPACK++ does not compile against
Accelerate ILP64 (`lapack_int` becomes Apple's `long` while the wrappers
assume `int64_t`; filed upstream as icl-utk-edu/lapackpp#89). Accelerate
is therefore constrained to LP64 for now — the same width the old
OpenBLAS default produced on macOS, so nothing regresses. RandBLAS
mirror: BallisticLA/RandBLAS#198 (unaffected: RandBLAS does not need
LAPACK++, and its Accelerate ILP64 lanes are green).

## Changes

- **Pins**: `installers/install.sh` and
`.github/actions/setup-randlapack-deps-windows/setup.ps1` advance BLAS++
to `2d8d4e9` (the #134 merge) and LAPACK++ to `b9439cf` (the #88 merge).
- **Backend default**: `--blas=auto` on macOS resolves to `accelerate`
(previously `openblas`). `--blas=openblas` remains fully supported.
- **Correctness guard**: a build in which BLAS++ silently fell back to
Apple's LEGACY interface is refused immediately after the BLAS++ build,
by checking the generated `defines.h` for `ACCELERATE_NEW_LAPACK` — the
legacy `gesdd` computes wrong singular values, so this must be a hard
error, not a warning. The numerical `gesdd` conftest stays as the second
net. On macOS < 13.3 the error message points at `--blas=openblas`.
- **Width**: the gesdd warning is removed. Accelerate stays LP64 for
now, and an explicit `--blas-int=ilp64` is refused with a message citing
icl-utk-edu/lapackpp#89 / #173 (previously the refusal cited the
long-gone interface gap).
- **Portability fix found by this PR's CI**: macOS ships bash 3.2, where
expanding an empty array under `set -u` is an "unbound variable" error;
with the old OpenBLAS default the affected arrays were never empty on
macOS, so the default flip exposed it (it was already reachable with an
explicit `--blas=accelerate` or `--blas=mkl` on any Mac). All
possibly-empty array expansions now use the bash-3.2-safe guard, and the
install-script lanes print `install.log` on failure so errors of this
class are never silent again.
- **install-script CI (macOS)**: the default lane deliberately does NOT
`brew install openblas` anymore, and asserts the summary reports
`accelerate, LP64`; a new explicit `--blas=openblas` leg (with its own
project dir and test-suite run) keeps that route exercised.
- **Rename, mirroring the review on BallisticLA/RandBLAS#198**: the
`install/` folder is now `installers/`. The root `install.sh` wrapper
keeps working (`bash install.sh` unchanged for users) and now execs
`installers/install.sh`; references updated in INSTALL.md,
INSTALL_SCRIPT.md, INSTALL_WINDOWS.md, docs/CI.md, the install-script
workflow, and the Windows guard scripts.
- **Docs**: INSTALL.md, INSTALL_SCRIPT.md (§6.1 table, §6.2 width table,
§6.4 rewritten), docs/CI.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Revert the macOS Accelerate quarantine (PR #157) once the upstream Accelerate PRs are approved

1 participant