Vendor Eigen instead of letting lwtnn configure its own - #353
Open
nsmith- wants to merge 5 commits into
Open
Conversation
lwtnn's BUILTIN_EIGEN downloads Eigen and configures Eigen's CMake project,
which drags in its unsupported/ test suite. On any system that has MPFR but
no GMP development headers -- CERN's GitLab CI runners among them -- that
dies at configure time with
GMP_LIBRARIES (ADVANCED)
linked by target "mpreal_support" in directory .../unsupported/test
and takes the whole wheel build with it. It first bit in 2.9.0, where lwtnn
started being built unconditionally. Our set(BUILD_TESTING OFF) cannot reach
it, since the ExternalProject configures with its own cache.
Eigen is header only, so vendor it as a submodule and point lwtnn's bundled
FindEigen3 at those headers instead; Eigen's own CMake project never runs.
Only eigen/Eigen/** is shipped in the sdist, and -DEIGEN3_INCLUDE_DIR=...
builds against a system copy.
Vendoring pins 3.4.0 rather than the 3.3.7 lwtnn downloaded, which moves the
lwtnn test fixture by two ULP. That assertion was already exact-float
fragile -- #348 disabled the entire aarch64 test suite over a one ULP
difference there -- so compare approximately and test aarch64 wheels again.
Closes #350
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
The wheel builds run the suite on every architecture we ship, but an approximate assertion only ever says pass or fail, so we never learn how much float spread is actually out there -- which is how #348 ended up skipping all of aarch64 in response to a single ULP. Route the three approximate assertions through a fixture that records the ULP distance from the reference and print a summary at the end of the run, so each cibuildwheel job's log carries a per-architecture record: ========================= float assertion ULP report ========================= arm64 darwin python 3.13.12 test_lwtnn_example 2 ulp 0.9518682535564676 vs 0.9518682535564679 test_ndpoly 3 ulp 1.0801881480751712 vs 1.0801881480751705 test_tformula 4 ulp 1.2512381067949132 vs 1.251238106794914 Purely informational; the assertions themselves are unchanged. Worth noting that the two pre-existing ones are already 3 and 4 ULP out on macOS arm64, so exact comparison was never what was being tested there. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
The report is only useful if you can read it without opening a log, so collect it as markdown and append it to the job summary, where it lands just under cibuildwheel's wheel table as one row per interpreter and assertion. It cannot be written to GITHUB_STEP_SUMMARY from the test run itself, which is the obvious approach: cibuildwheel builds its own summary and commits it with write_text() after every build and test has finished, so anything written there earlier is discarded. The tests write to CORRECTIONLIB_ULP_REPORT instead and a following workflow step appends that, which also survives cibuildwheel failing. Linux tests run in a container, so the path has to be passed in with environment-pass and then resolved against the /host mount to reach the runner's filesystem; the other platforms see the runner environment directly. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses source-build failures introduced in v2.9.0 by preventing lwtnn from configuring/building Eigen via its ExternalProject path, instead pointing lwtnn at a vendored (or user-specified system) Eigen header tree. It also relaxes float assertions and adds per-architecture ULP reporting so wheel CI can run tests across architectures without brittle bit-exact expectations.
Changes:
- Vendor Eigen headers and force lwtnn to use
EIGEN3_INCLUDE_DIRrather thanBUILTIN_EIGEN. - Replace strict float equality assertions with a shared
ulp_reportfixture that asserts viapytest.approxwhile recording ULP distances. - Append an ULP report into the GitHub Actions job summary for wheel builds.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Forces lwtnn to use a vendored/system Eigen include dir and disables lwtnn’s builtin Eigen ExternalProject. |
pyproject.toml |
Adjusts sdist contents to ship only Eigen headers needed for builds; re-enables aarch64 testing and passes ULP report env into Linux containers. |
tests/conftest.py |
Adds the ulp_report fixture and end-of-run reporting (terminal + optional markdown). |
tests/test_lwtnn.py |
Makes lwtnn numeric assertion tolerant and records ULP spread. |
tests/test_ndpolyfit.py |
Uses ulp_report for float comparisons. |
tests/test_core.py |
Uses ulp_report for a numerically sensitive TFormula assertion. |
.github/workflows/wheels.yml |
Collects and appends the ULP report into the job summary after cibuildwheel finishes. |
.gitmodules |
Adds Eigen as a submodule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
CVMFS benchmarksTop 25 slowest-loading corrections, sorted by mean time:
|
Collaborator
Author
|
@mads-hb let me know if this is a satisfactory fix for you |
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.
Note
This description is agent-generated text, written by Claude Opus 5 via Claude Code.
Closes #350.
The failure
Building 2.9.0 from source dies while CMake configures the bundled lwtnn:
lwtnn's
BUILTIN_EIGENacquires Eigen withExternalProject_Add, which runs afull CMake configure of Eigen — including its
unsupported/testsuite. Where MPFRis present but GMP's development headers are not, Eigen's
mpreal_supporttargetends up linking
GMP_LIBRARIES-NOTFOUNDand CMake treats that as fatal. CERN'sGitLab CI runners are in exactly that state.
This became everyone's problem in 2.9.0, when
add_subdirectory(lwtnn)startedrunning unconditionally. And as the reporter notes, our
set(BUILD_TESTING OFF)genuinely cannot reach it: the ExternalProject configures in a separate CMake
invocation with its own cache.
The fix
Eigen is header-only — nothing links against it, the templates are instantiated
into lwtnn's objects. So there is no reason to configure Eigen's build system at
all. This vendors Eigen as a submodule alongside pybind11/rapidjson/xxhash and
points lwtnn's own
FindEigen3.cmakeat the headers, which short-circuits on apre-set
EIGEN3_INCLUDE_DIR. Eigen's CMake project never runs, so GMP and MPFRnever enter the picture — for anyone, not just those who can pass a flag.
Also fixes the thing the reporter couldn't work around:
-DEIGEN3_INCLUDE_DIR=/usr/include/eigen3now builds against a system Eigen.
Only
eigen/Eigen/**ships in the sdist (337 files, ~1 MB compressed); tests,docs, demos and
unsupported/are excluded. lwtnn's only Eigen include is<Eigen/Dense>. As a bonus this removes one of the two network fetches a sourcebuild performs — Boost's ExternalProject stays, but it has a no-op
CONFIGURE_COMMANDand so was never implicated here.Why the test change comes along
Vendoring pins Eigen 3.4.0 rather than the 3.3.7 lwtnn downloads, and the summation
order differs enough to move the lwtnn fixture by two ULP:
electron_fastsim_sf0.95186825355646787(bit-exact to the assertion)0.95186825355646760assert sf == 0.95186825355646787was already too strict to survive a platformchange: #348 disabled the entire test suite on
manylinux_aarch64threecommits ago because that build differed by one ULP. Comparing with
pytest.approx(..., rel=1e-12)covers both, and lets the aarch64 wheels be testedagain — this PR touches
pyproject.toml, sowheels.ymlwill actually exercisethat here.
3.4.0 rather than 3.3.7 also seems the better bet for the reporter's toolchain:
3.3.7 predates gcc 15 by six years, and they build with gcc 15.2.0.
Testing
cmakeconfigure + build against the vendored copy, andpip install .— cleancvmfs) passes against Eigen 3.4.0-DEIGEN3_INCLUDE_DIR=<eigen-3.3.7>to confirm the ULP shift is theEigen version and nothing else, and that the override path works
eigen/entries, all undereigen/Eigen/