CI: fix the broken oneMKL cache path and drop a dead ctest exclusion - #161
Merged
Conversation
Two independent defects, both of which fail quietly. The Windows oneMKL cache never hits. `action.yml` pointed `path` at windows-deps\onemkl-2025.2.0.627 while setup.ps1 creates onemkl-$mklVersion with $mklVersion = "2026.1.0.226" (setup.ps1:384,390). The path was left behind when the version was bumped, so it names a directory that does not exist. Nothing errors: every MKL leg re-downloads 155 MB and then saves a cache entry for a missing path, so the cache can never hit. The key already said 2026.1.0.226, so only the path needed correcting -- and a comment now records that the two must agree with setup.ps1. Every other cache path/key pair in that file was checked and is consistent. `ctest --exclude-regex "^TestABRIK\.ABRIK_catch_instability"` excludes a test that does not exist. It appears nowhere in test/ or RandLAPACK/, and `git log -S` finds no history of it there under that name, so this has always been excluding nothing. Removed from all six sites: core-linux.yaml (x2), install-script.yaml (x2), and core-macos.yaml (x2). On macOS the regex was an alternation with TestQB.Polynomial_Decay_general1, which is the real Apple Accelerate gesdd quarantine from #157. Only the dead ABRIK half is removed there; the quarantine and its loud warnings are untouched. The cost of leaving it was not the wasted filter but the misdirection: a ctest line with an exclusion in it reads as though a known failure is being hidden, which invites exactly the wrong conclusion when someone audits the suite.
This was referenced Aug 13, 2026
mmelnich
added a commit
that referenced
this pull request
Aug 13, 2026
… backend selection, and docs (#162) ## Problem RandLAPACK's two installers had drifted apart. `install/install.ps1` was rebuilt in #156 and pins every dependency, checksums downloads, records provenance and link-and-run validates the BLAS before building anything. `install/install.sh` did none of that, and the documentation described a script that no longer existed in several respects. Eleven defects, all verified against `main` before touching anything: | Where | Defect | |---|---| | `install.sh:336,339,342` | three `git clone` with **no ref** — blaspp, lapackpp, random123 all tracked upstream default branches | | — | **no BLAS validation at all** | | — | `/opt/homebrew` hardcoded, **8 places** — hard-fails on Intel macOS and custom `HOMEBREW_PREFIX` | | `install.sh:331` | `mv "$REPO_DIR"` relocated the user's own clone, breaking git worktrees | | `install.sh:211` | `: > "$LOG"` truncated the log, destroying the previous run's output | | `install.sh:247` | libdir search omitted `lib/aarch64-linux-gnu` | | — | no `--prefix`; extras and benchmarks unconditional with no way to skip | | `install.ps1` | **zero** references to `RANDNLA_PROJECT_DIR`, no `--modify-rc` equivalent | | `INSTALL_SCRIPT.md` | documented the pre-backend flag list, and still told people the script would move their clone | ## The two things most worth reviewing ### Integer width is read back, not assumed BLAS++ probes `int32` before `int64`, and `blas_int` only filters which *library names* to try. For MKL that is a real choice — `mkl_intel_lp64` and `mkl_intel_ilp64` are different libraries. For OpenBLAS there is only `-lopenblas`, so **a successful `blas_int=int64` configure proves nothing**: an LP64 build passes the `int32` probe and is accepted. The resolved width therefore comes from BLAS++'s generated `blas/defines.h` after the build. Trusting the request would stamp LP64 installs as ILP64 and have every later run reuse them believing otherwise. ### Verification runs the program, and that is the point The conftest compiles, links and *runs* against the finished install — through BLAS++ and LAPACK++ rather than raw `dgemm_`, since that is how RandLAPACK reaches them — checking a `gemm` result and a `gesdd` factorization numerically. `gesdd` is chosen because it is the routine Apple's legacy Accelerate computes incorrectly. Running it rather than only linking matters for a reason specific to integer width. BLAS++ and LAPACK++ *do* guard the `int64_t` downcast — `to_blas_int` (`blaspp/src/blas_internal.hh:16-22`) throws rather than truncates — but that guard keys off `sizeof(blas_int)` **as declared by the header**. If the headers say 64-bit while the library actually loaded is LP64, the guard compiles out and 64-bit values reach routines reading 32 bits. That surfaces not as wrong numbers but as nonsense control values: a misread workspace query becomes an absurd `lwork`, and the run dies in allocation or never finishes. Nothing catches it by inspection. ## A cross-project collision, found while testing Dependency install directories now carry backend and GPU in their names (`blaspp-mkl-cpu-install`). That is not cosmetic. RandBLAS's installer uses the same `RandNLA-project` layout and named its BLAS++ install **identically** (`blaspp-<backend>-install`), with a different stamp filename. With a shared `RANDNLA_PROJECT_DIR`, this script would rebuild over RandBLAS's BLAS++ — their stamp never matches ours — and **RandBLAS's next run would reuse an artifact we had replaced underneath it, while its own stamp still described the original.** Distinct names make it impossible; sharing is instead explicit via `BLASPP_INSTALL_DIR`, verified by the conftest. ## Everything else - **macOS keeps Homebrew OpenBLAS as the default.** Apple's legacy Accelerate has a broken divide-and-conquer `gesdd`, and RandLAPACK calls `gesdd` in `rl_rsvd.hh:146`, `rl_abrik.hh:692`, `rl_revd2.hh:208`, `rl_preconditioners.hh:355` and `rl_util.hh:413`. That is what #157's quarantine is about. `--blas=accelerate` warns, citing #159. The hardcoded `/opt/homebrew` is fixed regardless. - **Extras and benchmarks stay built by default** (`--no-extras` / `--no-benchmarks`), unlike RandBLAS's examples — they need nothing this script has not already built. - **The clone is never moved**; a symlink gives the same layout and the path CI invokes still resolves. - **RandBLAS is untouched** — still a pinned submodule, still authoritative. - **`install.ps1`** now honours `RANDNLA_PROJECT_DIR` with the same precedence as `install.sh`, gains `-ModifyEnvironment` (User-scope, the only thing that survives a new shell) and `-Prefix`, and its path-length warning now checks the resolved path rather than only an explicitly passed one. - **Progress rendering** in three tiers, determinate from the build tool's own output, with tier 0 byte-identical to the plain step list — plus a CI assertion that redirected output carries no escape sequence. - **`INSTALL_SCRIPT.md`** rewritten: current flags, corrected layout diagram, and a new section 6 with a tested-configuration table drawn from the CI lanes, the per-backend integer width, whether LP64 actually limits RandLAPACK, and why macOS is on OpenBLAS. ## Verification Local Linux (gcc 15.2, oneAPI MKL, NVIDIA GPU present): | Scenario | Result | |---|---| | Fresh MKL build, `--no-gpu` | 9/9 steps, ILP64 confirmed from `blas/defines.h` | | Re-run in place | all six dependency steps reused, 1s rebuild | | Full default run (extras + benchmarks) | 13/13 steps | | `--no-gpu` → `--gpu` | BLAS++ provenance invalidated, rebuilt rather than reused | | Dependency discovery | all three reused, 3/3 steps | | Run from a git worktree | clone left in place, worktree still functional | | Output redirected | no ANSI escapes, no carriage returns | | **Full test suite** | **313/313 pass** | | RandBLAS then RandLAPACK, shared `RANDNLA_PROJECT_DIR` | both dependency sets intact | | RandLAPACK reusing RandBLAS's BLAS++ | reused, only LAPACK++ built, 5/5 steps | Windows 11, Windows PowerShell 5.1, VS 2022 Build Tools: all three `-ProjectDir` precedence cases resolve as intended, and the User-scope environment write round-trips. ## RandBLAS submodule bump Also bumps the vendored RandBLAS from `04f2018` to `952251c` (RandBLAS main), and `RandLAPACK_RandBLAS_PIN` with it. The three commits in that range are **#185** (vcpkg manifest fix), **#186** (CMake hygiene) and **#187** (installer scripts). None of them touch `RandBLAS/` headers — the library code is byte-identical, and everything in the range is build machinery. Two pieces of it matter here: - RandBLAS's CMake floor moved 3.12 → 3.21. RandLAPACK already declares 3.21, so nothing to change; the submodule now simply enforces what RandLAPACK already required. - RandBLAS exports `randblas_stage_runtime_dlls()` from its installed package and prints a configuration summary. As a subproject under RandLAPACK, which configures it with `BUILD_TESTS=OFF`, that summary reports tests as deliberately skipped rather than warning. `CMakeLists.txt` enforces that the pin variable and the submodule move together, and it means it: the cross-check compares the variable against `git ls-tree HEAD RandBLAS`, so a staged-but-uncommitted bump fails configure until both land in one commit. Worth knowing if you ever bump this iteratively — it is not satisfiable before committing. Verified: clean build against the bumped submodule, and the full suite re-run. ## Notes for reviewers - **Consolidated from #162 + #163 + #164** at Max's request, matching how the RandBLAS installer set was combined. Those two show as *merged* rather than closed because this branch was fast-forwarded to contain them — they merged into their stacked base, not into `main`. The tree is byte-identical to the state tested and green as three separate PRs. - Stacked on #161 (CI fixes), which stays separate — it is independent and useful on its own. - **`-ModifyEnvironment` is not verified end to end.** It runs at the end of a successful install, and I could not reach it: the clone sits on a `\\wsl.localhost` path and Windows `git` refuses it with `dubious ownership`. Precedence and the User-scope write are each verified directly, but not together in one completed run. - Two upstream gaps are filed rather than worked around silently: BLAS++ never looks for an ILP64 OpenBLAS (#166), and implements only Apple's legacy Accelerate interface (#165).
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
1 participant
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.
Problem
Two unrelated CI defects, both of which fail quietly rather than loudly.
The Windows oneMKL cache has never hit.
action.ymlpointedpathatwindows-deps\onemkl-2025.2.0.627, whilesetup.ps1createsonemkl-$mklVersionwith$mklVersion = "2026.1.0.226"(setup.ps1:384,390). The path was left behind when the version was bumped, so it names a directory that does not exist.Nothing reports an error.
actions/cachelooks up a key, misses, the job re-downloads 155 MB of oneMKL, and then the post-job step saves a cache entry for a path that isn't there. So the cache can never hit, on every MKL leg, indefinitely. The key already said2026.1.0.226— only the path was stale.A
ctestexclusion for a test that does not exist.--exclude-regex "^TestABRIK\.ABRIK_catch_instability"appears at six sites. That test is nowhere intest/orRandLAPACK/, andgit log -Sfinds no history of it there under that name, so it has always been excluding nothing.The cost isn't the wasted filter. It's the misdirection: a
ctestline carrying an exclusion reads as though a known failure is being suppressed, which invites exactly the wrong conclusion when someone audits what the suite actually covers.What this PR does
onemkl-2026.1.0.226, and adds a comment recording that the version there must track$mklVersioninsetup.ps1. Every other path/key pair in that file was checked and is consistent — OpenBLAS, GoogleTest, GoogleTest-ASan, Random123, BLAS++ and LAPACK++ all agree.core-linux.yaml(×2),install-script.yaml(×2),core-macos.yaml(×2).Notes for reviewers
TestQB.Polynomial_Decay_general1, which is the real Apple Accelerategesddquarantine from TEMPORARY: quarantine the macOS Accelerate gesdd test (revert after Mark's BLAS++/LAPACK++ review) #157. Only the dead ABRIK half is removed; the quarantine, its "suppression is ACTIVE" warning, and the check that shouts if the test starts passing are all untouched.install.shup to the standardinstall.ps1reached in Windows installation overhaul #156, mirroring the RandBLAS installer work. It is independent of the rest and useful on its own.