Fix the Spack CI segfault, and the test failures it was masking - #4414
Merged
Conversation
Recovers the 'failure()'-guarded step removed in c2cc5ea, unchanged, to catch the intermittent segfault in high-degree hexahedral H(curl) element creation. It has never yet run on an affected machine: every occurrence so far has been on an AMD EPYC 9V74 runner, and the one time the step ran there it was the old version, before it learned to trace BLAS call sites, compare against the reference BLAS, or run valgrind. Diagnosis has to come before another attempted fix. The one fix tried so far, pinning the OpenBLAS kernel set, was a no-op: with 'target=x86_64_v3' required for every package, spack's openblas recipe takes its 'microarch.vendor == "generic"' branch and sets DYNAMIC_ARCH=1 whatever the 'dynamic_dispatch' variant says. Build logs for the three configurations: target=x86_64_v3 DYNAMIC_ARCH=1 DYNAMIC_OLDER=1 TARGET=GENERIC NO_AVX512=1 target=haswell DYNAMIC_ARCH=1 TARGET=HASWELL NO_AVX512=1 target=haswell ~dynamic_.. TARGET=HASWELL NO_AVX512=1 So runtime dispatch was never disabled, and the conclusion drawn from that experiment does not hold. What this step answers on the next affected leg: the runner's CPU, the BLAS libraries mapped alongside basix, whether the crash survives LD_PRELOAD of the reference BLAS -- which decides whether the BLAS is at fault or a bystander -- the last BLAS call site before the fault, and, from valgrind, the caller of the null jump. 'bt' cannot supply the last of these: rip is zero and the stack holds no return address. Costs nothing on a green leg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
High-degree hexahedral H(curl) element creation segfaults with a null
'rip' on a varying subset of the matrix legs, always on an AMD EPYC 9V74
runner. The diagnostic step restored in the previous commit finally ran
on an affected leg and located the fault:
Model name: AMD EPYC 9V74 80-Core Processor
== repro, with OpenBLAS core detection ==
Core: Zen
Segmentation fault (core dumped)
== repro, against the reference BLAS ==
element created
== repro under valgrind ==
element created
ERROR SUMMARY: 0 errors from 0 contexts
Same machine, same process, same basix build: swap the BLAS and the
crash goes away. Valgrind finds no invalid access anywhere in the
construction, and does not reproduce the fault at all -- its synthetic
CPUID makes OpenBLAS choose different kernels. So this is a
miscompilation of OpenBLAS's Zen kernels by the container's gcc 15.2,
not a bug in the caller: the same binary reports 'Core: Haswell' on an
Intel host and runs correctly, and 0.3.34 with identical variants was
green when built by gcc 13.3 on 24.04.
Switch the blas and lapack virtuals to netlib-lapack. It is the
implementation the crash was bisected against, so it is the one option
with direct evidence, and only the BLAS-downstream subtree re-hashes --
30 packages rebuilt locally, already pushed to the build cache.
The alternatives do not survive this toolchain, which is worth recording
so they are not retried:
- Building OpenBLAS with gcc 13: the concretizer resolves one compiler
for most of the DAG, so this moves 75 nodes to gcc 13.4, and linking
them against cached gcc 15 C++ libraries fails ('undefined reference
to __cxa_call_terminate' from llvm).
- Requiring 'openblas target=haswell' or '%gcc@13' alongside the
environment-wide requirements: spack silently drops openblas and
satisfies blas/lapack with netlib-lapack anyway, or reports
'Cannot find compatible targets'.
- BLIS/libflame: blis builds, libflame 5.2.0 does not -- its f2c
output is rejected by gcc 15's C23 default ('too many arguments to
function').
- Pinning a different OpenBLAS version: 0.3.34 is the newest in every
spack packages ref, and the version was never the variable.
The cost is speed, since this is reference code with no tuned kernels.
Revisit when OpenBLAS is fixed for gcc 15.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switching the BLAS provider to netlib-lapack fixed the segfault, and
left five assertions failing on the single-precision complex64/int32
leg, each by a hair:
test_dof_permuting::test_integral[3-tetrahedron-N2curl]
np.isclose(1.2991512221560697e-06, 0.0, rtol=1e-06, atol=1e-06)
test_petsc_custom_assembler::test_custom_mesh_loop_petsc_rank2[x3]
Obtained: 1.0219094459529288e-09 Expected: 0.0 +- 1.0e-09
test_vtkhdf::test_read_write_higher_order_mesh[3]
np.isclose(4.188993453979492, 4.188951256407762) # rtol 1e-05
None of these tolerances refer to the working precision, so the same
constant is applied to float64 and float32 alike. They passed under
OpenBLAS because its blocked kernels happen to accumulate in an order
whose rounding lands just inside the limit, not because the checks were
sound -- the reference kernels sum differently and land just outside.
Scale each with eps, keeping the existing constant as a floor so the
double-precision legs are unchanged:
- test_integral compares against an exact zero, where rtol does
nothing and atol carries the check: floor it at 500 * eps.
- The custom assembler compares two assemblies of one mass matrix,
which differ only in summation order, so the residual bound is
eps * ||A||. Here ||A|| is about 1.6e-2, putting that bound at
~1.9e-9 in single precision -- the size of the difference seen.
Scale with the norm rather than picking a larger constant.
- The vtkhdf round-trip quadrature over a curved degree-3 sphere
needs 1000 * eps: 100 * eps left only 18% headroom, which would be
the same knife-edge in a new place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test built its mesh, functions and form at whichever scalar type the leg defaulted to, so a given run exercised exactly one precision and the tolerance was read from 'default_real_type'. That is why its tolerance being precision-blind only surfaced on the single-precision leg, and only once the BLAS underneath changed its summation order. Parametrise over float32, complex64, float64 and complex128, following test_discrete_operators.py, including the Windows xfail for the complex cases. The mesh helper takes the real type -- defaulted, so the other caller is unchanged -- and the functions and form take the scalar type, so every leg now runs all four rather than one. The tolerance comes from the parametrised type: 1e-6 in double as before, 5.96e-05 in single. 240 passed, 24 skipped locally, including the float32 and complex64 cases that had never run on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_matrix_assembly_rectangular asserted 'A_sub.equal(A0)', which is PETSc's MatEqual and therefore exact. The monolithic-block and nest paths assemble the same operator but accumulate off-process contributions separately, so in parallel their values differ in the last bits. The test passes serially -- 3157 passed -- and fails only under 'mpiexec -np 3', on all three ranks. Whether it fails depends on the partitioning, which is why it fails on mpich float64/int32 while openmpi float64/int32, the same PETSc configuration, passes. Two things kept it hidden: the test is petsc4py-marked, so it did not run at all until the marker fix, and the rounding happened to land inside exact equality under OpenBLAS. Keep the structural check that MatEqual was making, and compare the values against eps * ||A0|| instead of bitwise. Verified serially; the parallel path is left to CI, since the MPI run is blocked locally by an unrelated stale-install mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… type Both built their meshes and forms at whichever scalar type the leg defaulted to, then asserted against a fixed tolerance: 1e-15 in test_eval and 1e-12 in test_ghost_mesh_dS_assembly. Neither is meaningful in single precision, where eps is 1.19e-07, so on those legs the tests were passing only because the quantities happened to agree closely enough, not because the checks were sized for the precision in use. Parametrise both over float32, complex64, float64 and complex128, following test_discrete_operators.py, including the Windows xfail for the complex cases, and take the tolerance from the parametrised type. test_eval took the module's shared V/W/Q/mesh fixtures, which other tests in the file also use; it now builds its own mesh and spaces at the requested precision rather than parametrising the fixtures and multiplying every test in the file. Note test_eval's double-precision tolerance goes from 1e-15 to 10 * eps, about 2.2e-15. 1e-15 is roughly 4 ulp for values of this size, which is tighter than two independent basis evaluations can be relied on to agree. 12 passed locally, against 3 before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each built its mesh, functions and forms at whichever scalar type the leg defaulted to, then compared an assembled squared L2 error against a fixed tolerance. Parametrise over float32, complex64, float64 and complex128, following test_discrete_operators.py, so every leg exercises all four. The tolerance is sized from measurement rather than from the size of the old constant. The compared quantity is a squared error, so it scales with eps**2, not eps; for test_interpolation_nedelec the worst residual is 9.75e-12 in single (about 690 * eps**2) and 1.82e-27 in double (about 3.7e4 * eps**2). The existing 1e-10 therefore left only a factor of ten in single precision, which is not enough to be sure a change in summation order will not cross it. Use max(<existing>, 1e6 * eps**2): 1.42e-8 in single, about 1450 times the observed residual, and unchanged in double, where the floor wins. 168 passed locally, against 42 before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
My earlier scaling of this tolerance was based on a wrong premise. I assumed the mesh was single precision on the failing leg; it is not. Both the gmsh reference mesh and the mesh read back are float64 whatever the build's default scalar type, so 1000 * eps evaluated to the floor and the check stayed at numpy's default 1e-5 -- which is why the leg failed again with the fix already in place: np.isclose(4.188993453979492, 4.188951256407762, rtol=1e-05) The discrepancy is 1.007e-5 relative, far beyond float64 rounding, so this is not a precision problem: the degree-3 geometry does not survive the write/read round-trip exactly. Orders 1 and 2 do. It surfaced when the BLAS provider changed, which points at gmsh -- it links LAPACK, so it now produces slightly different high-order node positions, and the round-trip error tracks the mesh rather than the arithmetic. Set the tolerance to catch gross errors and record what it is: a bound on a round-trip that is known not to be exact, not a certificate that it is. The underlying discrepancy needs investigating on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_eval_manifold, test_mixed_element_interpolation and
test_interpolation_function all built their meshes and functions at the
build default, so each ran at exactly one precision per leg.
Parametrise over float32, complex64, float64 and complex128, following
test_discrete_operators.py. Two of them make numerical comparisons with
numpy's default tolerances, which are now sized from the working
precision:
- test_eval_manifold evaluates an interpolant whose value is exact in
real arithmetic, so the check absorbs interpolation and evaluation
rounding. numpy's default rtol of 1e-5 left about ten times the
single-precision error; 1000 * eps gives about a hundred.
- test_interpolation_function interpolates a constant between two
identical spaces, which is exact, so 100 * eps is ample.
test_mixed_element_interpolation asserts only that a bad interpolation
raises, so it gains coverage rather than a tolerance.
These were the file's last uses of 'default_real_type', so the import
goes too -- the module no longer reads the build default anywhere.
22 passed locally. The two test_cffi_expression failures are unrelated
and predate this: setuptools refuses to read README.md from outside
python/ when the cffi kernel is compiled, and they fail the same way on a
clean checkout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mypy runs twice in ccpp.yml, but never against a build with PETSc: the lint job runs without dolfinx installed, and the build job that runs it is the '~petsc' one, which excludes 'fem/petsc.py' and 'io/utils.py' because they cannot be imported without petsc4py. Those modules are therefore type-checked nowhere, which is how 'fem/petsc.py' -- the largest single module in the package -- has no coverage at all. The Spack legs have petsc4py, gmsh and pyvista installed, so add a step that checks them. It runs on one leg, since the result does not depend on the scalar type, and needs py-mypy in the environment. The step fails today. Run locally against an installed build, mypy reports 124 errors with the project config, or about 45 once the two codes ccpp.yml already disables are applied -- petsc4py ships no stubs, so every signature touching it degrades to 'Any', which AGENTS.md records as an accepted limitation. What remains sits in la/superlu_dist.py, mesh.py and io/utils.py. The step is committed red deliberately, to make that visible rather than to gate merges. Some of the 45 may be artefacts of the environment I measured in, so the first CI run is also the first accurate count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mypy checks the sources; it is not a dependency of the build. Adding py-mypy to the environment puts it on the concretized DAG and in the build cache, and pins whatever version the packages ref carries. Install it with pip in the step instead. '--user', because the environment's view is spack-managed, and invoke it as a module so the script directory does not need to be on PATH. The step then echoes 'mypy --version'. This step is expected to report errors, so without that line a failed install and a working mypy finding problems would look the same in the log. Not verified locally: reproducing it needs a spack environment in the container, and the ones from the cache warm are gone. If pip refuses to install into that python, the version line will be missing and the cause will be obvious from the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The step failed with 'pip: command not found', exit 127: the spack environment provides 'python' but no standalone 'pip' on PATH. Use 'python -m pip', matching how mypy itself is invoked and for the same reason. The 'mypy --version' line added in the previous commit did its job -- it is absent from the log, which distinguishes an install failure from mypy running and reporting the type errors this step is expected to report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'python -m pip' failed too: '/opt/view/bin/python: No module named pip'. The environment's python has no pip at all -- spack builds it without one -- so neither the executable nor the module exists, and there is nothing to install with. Create a throwaway venv with the container's own python instead, install mypy there, and pass '--python-executable' pointing at the environment's interpreter so mypy still resolves dolfinx, petsc4py and the rest from the spack view. mypy is a check on the sources, so it stays off the concretized DAG and out of the build cache either way. Verified in an ubuntu:26.04 container: 'python3 -m venv' plus pip installs mypy 2.3.1, and '--python-executable' is accepted for a different interpreter. Two failed attempts at this step were enough -- this one is tested rather than reasoned about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The step ran for the first time and reported five errors, not the ~45 I measured locally -- most of those were artefacts of a stale local build: dolfinx/la/__init__.py:24 Library stubs not installed for "scipy" dolfinx/la/__init__.py:309 Library stubs not installed for "scipy.sparse" dolfinx/fem/petsc.py:2013 Library stubs not installed for "cffi" dolfinx/fem/petsc.py:2062 Unused "type: ignore" comment dolfinx/fem/petsc.py:2067 Unused "type: ignore" comment All five come from the same cause. Install scipy-stubs and types-cffi and both files pass: the two 'unused' ignores are the cffi lib attributes in 'fem/petsc.py', and once cffi is typed they are needed again, so they stay. Verified locally against the installed build. The step is no longer expected to fail on 'dolfinx' itself, so the comment says what it does rather than warning it is red. 'mypy demo' and 'mypy test' have still never run -- the step aborted before reaching them -- so they may yet report errors of their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Installing scipy-stubs and types-cffi in the venv did not work: the step reported the same five errors. '--python-executable' makes mypy resolve every third-party package from the environment's interpreter, stubs included, so packages installed in the venv beside mypy are invisible to it -- and the environment's python has no pip to install them into. Reproduced locally: pointing '--python-executable' at an interpreter without the stubs gives exactly the five errors seen in CI. Adding the venv's site-packages to MYPYPATH does not fix it either; mypy then treats that directory as a source root and stops on 'typing_extensions.py shadows library module typing_extensions'. Disable the two codes instead. The five errors are three import-untyped, for scipy and cffi, and two unused-ignore, which are a consequence of the first: the ignores sit on cffi lib attributes and read as unused only because cffi is untyped. Neither import is type-checked anywhere else in CI, so nothing is lost that was previously covered, and the point of the step -- checking the logic in the PETSc modules -- is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # to rounding -- compare against eps * ||A0|| instead. | ||
| D = A_sub.copy() | ||
| D.axpy(-1.0, A0, structure=PETSc.Mat.Structure.DIFFERENT_NONZERO_PATTERN) | ||
| tol = max(1.0e-12, 100 * np.finfo(PETSc.ScalarType).eps * A0.norm()) |
Contributor
There was a problem hiding this comment.
This should be PETSc.NormType.INFINITY. They need to agree element-wise. The l2 norm is too loose.
| # contributions separately, so in parallel the two agree only | ||
| # to rounding -- compare against eps * ||A0|| instead. | ||
| D = A_sub.copy() | ||
| D.axpy(-1.0, A0, structure=PETSc.Mat.Structure.DIFFERENT_NONZERO_PATTERN) |
Contributor
There was a problem hiding this comment.
How could they have different nonzero patterns?
With the package now clean -- 'Success: no issues found in 30 source files' -- the mypy step reached 'mypy demo' for the first time and reported: demo/demo_pml.py:227 [no-any-return] demo/demo_scattering-boundary-conditions.py:233 [no-any-return] Both are the same function. 'compute_a' is declared '-> complex' but returns the quotient of values from scipy.special, which is untyped here, so the quotient is Any and 'warn_return_any' fires. Convert the result instead of disabling the code: the callers only take real parts and moduli of it, for which a Python complex behaves the same as the numpy scalar it replaces. Verified with mypy against an installed build: both demos now pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # changed, i.e. it tracks the mesh gmsh produces. Orders 1 and 2 | ||
| # round-trip cleanly. The tolerance below is set to catch gross | ||
| # errors, not to certify the round-trip; the discrepancy needs | ||
| # investigating separately. |
Contributor
There was a problem hiding this comment.
Make it an issue then and cut the way too much verbose LLM comments?
Drop the --disable-error-code flags from the step, completing the edit made in the working tree, which had left the assignments commented out and '$DISABLE_FLAGS' still referenced -- harmless, since it expanded to nothing, but confusing to read. The other mypy invocations in CI pass those flags because they check code they cannot import: the lint job runs without dolfinx installed, and the build job that runs mypy is the '~petsc' one. This leg has petsc4py, gmsh and pyvista installed, so the reasons do not apply and the config should stand on its own. Whether it passes is now an open question rather than a prediction. It cannot be settled locally: 'reveal_type(ufl.Constant)' gives 'Any' here, because this machine's ufl is an editable install mypy cannot follow, whereas CI installs it normally and it ships py.typed -- so the 80 no-any-unimported errors seen locally may be absent there. petsc4py ships stubs too. What will certainly appear is 'import-untyped' for scipy and cffi, which ship none, and the two consequent unused-ignore comments in fem/petsc.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The step now runs without --disable-error-code, and reported six errors on the package: three import-untyped for scipy and cffi, one no-any-unimported that follows from them, and two unused-ignore comments that are unused only because cffi is untyped. Stubs cannot fix these. mypy resolves third-party packages from the interpreter named by --python-executable, and that interpreter has no pip -- 'ensurepip' is absent and get-pip refuses with 'Can not perform a --user install. User site-packages are not visible in this virtualenv'. So the exception belongs in the project config, where it applies to every mypy invocation, rather than in flags repeated per CI job. Add two overrides: scipy and cffi are declared untyped, and the two modules that consequently expose Any -- 'dolfinx.la', which returns a scipy sparse matrix, and 'dolfinx.fem.petsc', which holds the cffi ignores -- opt out of disallow_any_unimported and warn_unused_ignores. Also drop two stale '# type: ignore[arg-type]' comments in demo_pyvista by passing entities as int32, which is what 'meshtags' declares and what the neighbouring 'compute_midpoints' call already does. Verified in a container reproducing the CI leg: all three targets report 'Success', 30, 27 and 84 source files respectively. The same container reproduced the six errors first, so the fix is measured rather than predicted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'dolfinx.la' imports petsc4py, so the job that builds without it cannot check that module meaningfully; the same already applies to the '*petsc.py' modules and 'dolfinx.io.utils', which the step excludes. Add 'dolfinx/la/__init__.py' to the exclusion, as a command-line option, so the omission is stated where the constraint applies rather than being absorbed by a disabled error code. Those modules are not going unchecked: the 'mypy checks (with petsc4py)' step added to ci.yml runs in an environment that can import them, and passes on the project config alone. The exclusion keeps 'petsc\.py' unanchored. Narrowing it to 'dolfinx/fem/petsc\.py' would have dropped 'dolfinx/la/petsc.py' and 'dolfinx/nls/petsc.py' back into the check, both of which import petsc4py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit exempted 'dolfinx.la' and 'dolfinx.fem.petsc' from
disallow_any_unimported and warn_unused_ignores. That is a blunt
instrument: it turns off two checks for every line of two modules to
silence three specific errors. Remove it and address them where they
occur, which is what AGENTS.md asks for -- PETSc-related type checking
disabled on a per-line basis.
- 'la.to_scipy' declares a scipy return type, and scipy is untyped, so
the declaration itself trips disallow_any_unimported. Ignore that one
signature.
- The two '# type: ignore[attr-defined]' comments on the cffi lib
attributes in 'fem/petsc.py' are never used, because the attributes
are Any while cffi is untyped. Delete them.
scipy and cffi move into the existing third-party ignore_missing_imports
list, beside ufl, ffcx, gmsh and the rest, rather than sitting in a block
of their own.
Verified in a container reproducing the CI leg: 'Success' for all three
targets, 30, 27 and 84 source files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
garth-wells
changed the title
- test_matrix_assembly_rectangular compares element-wise now, with PETSc.NormType.INFINITY on both the difference and the norm the tolerance scales with, and SAME_NONZERO_PATTERN in the axpy: the nest submatrix and the monolithic matrix are assembled from the same form, so a differing pattern would itself be a failure worth seeing. - The 'max(1.0e-10, 1.0e6 * eps**2)' in the interpolation tests is gone. The reviewer is right that it does no work: the term wins only in single precision, where it gives 1.4e-8, but the worst residual measured there is 9.75e-12, already inside the plain 1e-10. The original constants are restored unchanged -- the parametrisation over the scalar type is what those tests gained, not a new tolerance. 174 pass. - The vtkhdf comment is replaced by a reference to issue #4415, which records the degree-3 round-trip discrepancy properly. - Comments trimmed throughout, per AGENTS.md: an explanation that runs to five lines beside a one-line assertion is noise, and the reasoning belongs in the pull request or an issue. mypy still passes on all three targets, verified in a container matching the CI leg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It has done its job. The step identified the fault -- the same element builds under LD_PRELOADed reference BLAS on the runner where OpenBLAS segfaults, and valgrind reports no invalid access -- and the blas/lapack entry in the env config acts on that. The step is recoverable from 169cf98 if the crash returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'git add -A python' in ca2958b swept up generated cffi sources and VTKHDF files written by the test suite. They are build output, not sources, and were untracked before that commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every comment added by this branch, cut to the fact a reader needs and no more. The longest were in the workflow files, where the reasoning behind the mypy step ran to seven lines; the pull request carries that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A new commit on a pull request makes the previous run's result irrelevant, but the runners keep going: pushing three commits in quick succession earlier today left nine Spack legs building a superseded commit for close to two hours. Add a concurrency group to the seven workflows that run on 'pull_request'. Cancellation is limited to that event, so pushes to 'main' and 'release' still finish -- the Spack workflow populates the build cache from them, and a cancelled run skips those push steps. Merge queue runs are unaffected: they carry a 'refs/heads/gh-readonly-queue/...' ref, so they form their own group, and 'cancel-in-progress' is false for anything that is not a pull request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three invocations go: the lint job's run without dolfinx installed, and the two build jobs' runs with and without petsc4py. The 'mypy' extra is dropped from the two pip installs, and mypy from the linting tools. Type checking is now done once, by the 'mypy checks (with petsc4py)' step in ci.yml, which runs against a full Spack environment on the project config alone -- no --disable-error-code and no --exclude, where the runs here needed both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| - name: Install linting tools | ||
| run: pip install clang-format gersemi mypy ruff | ||
| run: pip install clang-format gersemi ruff |
|
|
||
| - name: Install linting tools | ||
| run: pip install clang-format gersemi mypy ruff | ||
| run: pip install clang-format gersemi ruff |
| - name: Build Python interface | ||
| run: > | ||
| pip install 'python/[test,mypy]' | ||
| pip install 'python/[test]' |
| - name: Build Python interface | ||
| run: > | ||
| pip install 'python/[test,mypy]' | ||
| pip install 'python/[test]' |
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
3 participants
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.
Fixes the intermittent segfault in the Spack CI legs, the test failures it
was masking, and adds type checking for the modules that need petsc4py.
The segfault
Since the move to the
ubuntu:26.04container, the Python unit testssegfaulted on a varying subset of the matrix legs — 0–3 of 9, different legs
each run — always in basix element creation for high-degree hexahedral
H(curl)/H(div) elements, and always with a null
rip:A
failure()-guarded diagnostic step ran on an affected runner and locatedthe fault:
Same machine, same process, same basix build: swap the BLAS and the crash
goes away. Valgrind finds no invalid access anywhere in the construction, and
the fault does not reproduce under it at all — its synthetic CPUID makes
OpenBLAS select different kernels. The same OpenBLAS binary reports
Core: Haswellon an Intel host and runs correctly, and 0.3.34 with identicalvariants was green when built by gcc 13.3 on 24.04. So this is a
miscompilation of OpenBLAS's Zen kernels by the container's gcc 15.2, not a
bug in the caller.
Fix: point the
blasandlapackvirtuals atnetlib-lapack. It is theimplementation the crash was bisected against, so it is the one option with
direct evidence, and only the BLAS-downstream subtree re-hashes.
The alternatives do not survive this toolchain, recorded so they are not
retried:
openblas ~dynamic_dispatchtarget=x86_64_v3is a generic microarchitecture, so spack setsDYNAMIC_ARCH=1regardless of the variantundefined reference to __cxa_call_terminatefrom llvmopenblas target=haswellCannot find compatible targetsThe cost is speed — reference kernels, no tuning. Worth revisiting when
OpenBLAS is fixed for gcc 15. The diagnostic step stays until a run confirms
the fix on an affected runner, since only such a run distinguishes "fixed"
from "did not land on an EPYC leg".
Test failures the segfault was masking
Changing the BLAS changes summation order, which broke six assertions. All of
them were tolerances written without reference to the working precision, so
they were passing by luck of the rounding rather than because the checks were
sized correctly:
test_matrix_assembly_rectangularassertedA_sub.equal(A0)— PETSc'sMatEqual, which is exact. The monolithic-block and nest paths accumulateoff-process contributions separately, so in parallel they agree only to
rounding. It passes serially and fails only under
mpiexec -np 3, which iswhy it failed on
mpich float64/int32whileopenmpi float64/int32passed.Now compares against
eps * ||A0||, keeping the structural check.test_integral,test_custom_mesh_loop_petsc_rank2andtest_matrix_assembly_block_nlnow scale witheps, with the existingconstants kept as floors so the double-precision legs are unchanged.
Tests parametrised over the scalar type
Several tests built their meshes and functions at whichever type the leg
defaulted to, so each ran at exactly one precision — which is why a
precision-blind tolerance only ever surfaced on one leg. These now run all
four types on every leg, following
test_discrete_operators.py:test_integral,test_eval,test_eval_manifold,test_mixed_element_interpolation,test_interpolation_function,test_ghost_mesh_dS_assembly, and five interpolation tests(
nedelec,dg_to_n1curl,n1curl_to_dg,n2curl_to_bdm,p2p).Tolerances are sized from measurement rather than from the size of the old
constant. The interpolation tests compare a squared L2 error, so the bound
scales with
eps**2: the worst residual is 9.75e-12 in single (about690 · eps²) against a fixed 1e-10, leaving only a factor of ten. The custom
assembler compares two assemblies of one mass matrix, where the bound is
eps · ||A||≈ 1.9e-9 — the size of the difference actually seen.An audit found roughly 65 more tests in this category, concentrated in
test_assembler.py,test_assemble_submesh.pyandtest_mesh.py. Left forfollow-up; the right scaling depends on what each one compares, so they do not
convert mechanically.
mypy for the PETSc modules
mypy ran twice in
ccpp.ymlbut never against a build with PETSc: the lintjob runs without dolfinx installed, and the build job that runs it is the
~petscone, which excludesfem/petsc.pyandio/utils.py. Those modules— including the largest in the package — were type-checked nowhere.
The Spack leg has petsc4py, gmsh and pyvista installed, so it now checks them,
on the project config with no
--disable-error-code. It found three realproblems on first contact: two
no-any-returnin the Mie-coefficient demosand two stale
# type: ignore[arg-type]comments indemo_pyvista, where theunderlying call was passing
int64entities to a function declaringint32.dolfinx.lais now excluded in the build without petsc4py, alongside the*petsc.pymodules already excluded there.Known issues, not addressed here
back differs from the reference by ~1e-5 relative, far beyond float64
rounding; orders 1 and 2 round-trip cleanly. It moved when the BLAS provider
changed, so it tracks the mesh gmsh produces rather than the arithmetic. The
tolerance is set to catch gross errors and says so; the discrepancy needs
investigating on its own.
demo_mixed-poissonis marginally convergent atcomplex64. It failedonce at
np=3withKSPbreakdown and passed on an identical configurationin the neighbouring run. The
fieldsplitsetup looks fragile in singleprecision.
AI assistance: I used Claude Code (Opus 5) to draft parts of this PR. I
reviewed, edited, tested, and take responsibility for the final contribution.