Skip to content

Add ExaChem CI integration for GHA and ADO pipelines + reorganize build scripts - #685

Open
Loris Ercole (lorisercole) wants to merge 104 commits into
mainfrom
ler/feat/exachem-ci
Open

Add ExaChem CI integration for GHA and ADO pipelines + reorganize build scripts#685
Loris Ercole (lorisercole) wants to merge 104 commits into
mainfrom
ler/feat/exachem-ci

Conversation

@lorisercole

@lorisercole Loris Ercole (lorisercole) commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Adds ExaChem (external CCSD/TAMM MPI application) build/cache/test CI integration to both the GitHub Actions and Azure DevOps pipelines (Linux x86_64 only), reusing qdk-chemistry's already-built C++ dependencies wherever possible instead of letting TAMM's CMSB superbuild rebuild redundant copies. See code comments in the changed files for the detailed rationale behind each fix.

Scope / sequencing (read this first)

This PR only adds the build/cache/install side. The Python-level consumer (qdk_chemistry.plugins.exachem, tests/test_exachem_ccsd_integration.py) lives on the separate, still-open PR #611 and is not part of this branch. Until #611 merges, the only thing exercising the built ExaChem binary is install-exachem.sh's own internal smoke test (link check + minimal mpirun run), not a qdk-chemistry-level pytest.

Pipeline changes

GitHub Actions (build-and-test.yaml)

  • Each Linux coverage matrix leg builds/caches GlobalArrays+TAMM+ExaChem (via install-exachem.sh) alongside the existing C++ deps cache, keyed on the same manifest/install-script hashes.
  • ExaChem's PATH/LD_LIBRARY_PATH are scoped only to the "Run Python tests" step, not the whole job.
  • test-with-qdk-branch no longer rebuilds C++/Python from scratch: it now depends on coverage and downloads that job's already-built install directory as an artifact, then just overlays the qdk branch package and re-runs pytest.
  • Removed the "Free up disk space" cleanup step (targeted GitHub-hosted-runner bloat that doesn't apply to our self-hosted pools).
  • Linux and Windows legs cap CMAKE_BUILD_PARALLEL_LEVEL by available RAM (~8 GB/job) instead of raw core count, to avoid OOM/heap-exhaustion kills on shared runners (issue emerged after Bumping LibInt2 version in CI/CD and CMake #692).

Azure DevOps (python-wheels.yaml)

  • New Build_linux_x86_64_exachem job (gated on linux_x86_64 being in the matrix) builds the same GlobalArrays+TAMM+ExaChem stack against BLIS+LibFLAME inside the wheel-build container, caches it (Cache@2, keyed including dockerImage), and publishes it as a pipeline artifact.
  • The linux_x86_64 wheel-test job downloads that artifact and exposes it to test-pip-wheels.sh the same way GHA does; a command -v ExaChem check confirms the binary is actually on PATH in the test environment.
  • dockerImage is now also an explicit parameter where python-wheels.yaml calls the ExaChem build template.
  • Removed the same disk-space cleanup step from build-pip-wheels.yml for consistency.

Shared install scripts (.pipelines/install-scripts/)

  • Centralized install-blaspp.sh/install-lapackpp.sh/install-cpp-deps.sh, used by both the devcontainer and the pipelines instead of duplicated inline logic; both forward BUILD_TYPE to blaspp/lapackpp, and install-cpp-deps.sh's job count now defers to the pipeline's own memory-aware CMAKE_BUILD_PARALLEL_LEVEL before falling back to raw core count.
  • BLIS/LibFLAME versions are now hardcoded (no caller ever overrode them); HDF5's version/URL/hash are sourced from cgmanifest.json (converted to an other-type entry recording the actual downloaded tarball, matching Libint's pattern) instead of a separate pipeline parameter.
  • Checksum verification prefers sha256sum/sha1sum over macOS-only shasum for portability.
  • .pipelines/install-scripts/exachem/patches/: upstream-portable patches for CMSB dependency reuse, GlobalArrays' LAPACK vendor selection, and ExaChem/GauXC/LibInt2 compatibility fixes — each documents its own root cause and pinned-commit verification. Bumping Libint to 2.13.1 (via a main merge) retired the old DEPRECATED-macro patch and required a new DATADIRLIBINT_DATADIR cherry-pick patch, since upstream's own fix for this also raises the minimum GCC version past what our runners have.
  • cpp/manifest/qdk-chemistry/cgmanifest.json: added GlobalArrays/TAMM/ExaChem/CMSB as development dependencies, for Component Governance visibility (they execute in CI even though never shipped in the wheel). The pins in install-exachem.sh remain authoritative for the build; keep both in sync if bumped.

Validation

Green ADO and GHA runs building/caching/smoke-testing ExaChem end-to-end on Linux (both OpenBLAS and BLIS+LibFLAME legs), plus green Linux/macOS/Windows wheel builds. No qdk-chemistry-level Python test yet exercises ExaChem (see "Scope / sequencing" above).

The generic 'install-*/' rule (meant for Python setuptools build
artifacts) also matched the install-scripts directory name, silently
ignoring any new files added there. Add negation entries so files in
.pipelines/install-scripts/ are tracked normally.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Single canonical scripts for building BLAS++ and LAPACK++, to be
reused by both the devcontainer and CI pipelines instead of each
maintaining its own copy of the build logic. BLAS vendor and the
commit to build are caller-supplied (no hardcoded defaults) so the
commit hash is always resolved from cgmanifest.json by the caller.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New CI orchestrator script for building spdlog, BLAS++, LAPACK++,
LibInt2, ECPint and GauXC, mirroring the devcontainer's dependency
set but calling the newly centralized install-blaspp.sh/
install-lapackpp.sh scripts. Defaults to x86-64-v3 and OpenBLAS to
match the GHA runners; BLAS vendor is overridable for future use in
the BLIS/LibFLAME-based ADO wheel pipeline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d BLAS++/LAPACK++ scripts

This script is now devcontainer-only; CI pipelines use the new
.pipelines/install-scripts/install_cpp_dependencies.sh instead.
Replaces the inline BLAS++/LAPACK++ cmake+build blocks with calls to
the shared install-blaspp.sh/install-lapackpp.sh scripts so there is
a single implementation of that build logic. No behavior change for
existing devcontainer/test-with-qdk-branch usage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
install_exachem.sh builds TAMM and ExaChem for use in CI, reusing
BLAS++/LAPACK++/LibInt2/GauXC from the C++ deps prefix built by
install_cpp_dependencies.sh, and MPI from the system (apt-installed
on Linux). Requires mpicc on PATH, fails fast otherwise. Patches are
carried over unchanged from the working ExaChem container build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extends the coverage job's Linux path to build and cache TAMM/ExaChem
so the ExaChem interface introduced in this branch can be exercised
in CI:
- installs openmpi-bin/libopenmpi-dev via apt (no MPI source build)
- switches the C++ deps install step to the new
  .pipelines/install-scripts/install_cpp_dependencies.sh, updating
  cache keys accordingly
- adds cache/build/save steps for install_exachem.sh, exporting the
  install prefix onto PATH/LD_LIBRARY_PATH for the test step

macOS, coverage-windows and test-with-qdk-branch jobs are unaffected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep only usage docs and non-obvious rationale (why a value is chosen,
external tool quirks); drop restated-code and cross-file pointer
comments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pin their repository URLs and commit hashes alongside the rest of
qdk-chemistry's C++ dependencies, so install_exachem.sh can resolve
them the same way blaspp/lapackpp/spdlog/etc. already do, instead of
hardcoding them in the script.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ersions from cgmanifest.json

- Move .devcontainer/scripts/exachem/ to .pipelines/install-scripts/exachem/
  (only used in CI, not the devcontainer, so it belongs alongside the
  other pipeline install scripts).
- install_exachem.sh now takes the cgmanifest.json path as an argument
  and resolves TAMM/ExaChem's repository URL + commit from it, instead
  of hardcoding them as script defaults.
- Trim/rephrase comments in the script and its patches so they stand
  on their own in this public repo (no references to internal-only
  build environments).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
install_exachem.sh now lives under .pipelines/install-scripts/exachem/
and takes the cgmanifest.json path as an argument; update the call
site and cache-key hashFiles() patterns accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An unescaped apostrophe inside a \ parameter expansion
broke bash's brace matching even though the whole string was
double-quoted, causing 'unexpected EOF while looking for matching' on
every Linux CI run. Reword the message to avoid the apostrophe.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
install-blaspp.sh's default BLAS_VENDOR=openblas forced BLAS++ to
only search for OpenBLAS, but macOS CI has no OpenBLAS installed (it
relied on BLAS++'s prior auto-detection of Apple's Accelerate
framework). Default to 'auto' on macOS and 'openblas' on Linux;
callers (e.g. the ADO wheel pipeline) can still override BLAS_VENDOR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TAMM's CMSB superbuild has an internal CMakeBuild_External step whose
install command is 'ninja install DESTDIR=<stage>' -- make(1) accepts
DESTDIR=... as a variable override, but ninja parses it as an unknown
build target and fails ('unknown target DESTDIR=...'). Drop -GNinja
from both the TAMM and ExaChem cmake invocations so CMake falls back
to its default Unix Makefiles generator, which CMSB is compatible
with.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CMSB's find_package(Eigen3 CONFIG) doesn't locate Ubuntu's apt-installed
Eigen3Config.cmake on its default search path, so it falls back to
git-cloning its own copy from gitlab.com -- which returned HTTP 403 on
every attempt on these CI runners (consistently, not just transient
flakiness). Discover the apt package's config directory and pass it
via -DEigen3_DIR so CMSB reuses it instead of touching gitlab.com.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
find ... | head -1 under 'set -o pipefail' aborts the script whenever
find is still writing when head exits after its first line (SIGPIPE),
regardless of whether a match was found. Use 'find -print -quit'
instead, which stops after the first match without a pipe, and guard
it with '|| true' so a permission-denied error during the (unlikely)
full-tree scan doesn't also trip 'set -e'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… flag

CMSB's nested TAMM_External re-configure only finds dependencies that
are actually installed under CMAKE_INSTALL_PREFIX (which it always
puts on CMAKE_PREFIX_PATH); it does not forward arbitrary -D flags
from the outer cmake invocation. Passing -DEigen3_DIR only satisfied
the outer configure, and the nested one still tried (and failed) to
git-clone Eigen3 from gitlab.com. Instead, copy the apt-installed
libeigen3-dev's cmake config + headers into INSTALL_PREFIX, mirroring
the layout CMSB would itself produce, so every configure pass in the
build finds it the same way it finds every other already-built
dependency.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EIGEN3_CMAKE_DIR is .../share/eigen3/cmake (3 levels below the actual
install prefix), but the prefix was computed with only 2 dirname
calls, yielding .../share instead of the real prefix (e.g. /usr) --
verified by simulating the exact path locally.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
libaec mirror changed, fix implemented in vcpkg
…AS_VENDOR a parameter

Renamed to match the other pipeline install-scripts (install-blaspp.sh,
install-lapackpp.sh, install-blis.sh, ...). BLAS_VENDOR is now an
optional 3rd positional argument instead of an env var, defaulting to
the same OS-aware value (openblas on Linux, auto on macOS) when
omitted.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
install_exachem.sh -> install-exachem.sh, install_cpp_dependencies.sh
-> install-cpp-deps.sh (also includes the user's own trim of a
redundant DESTDIR/Ninja comment).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
install_cpp_dependencies.sh -> install-cpp-deps.sh,
exachem/install_exachem.sh -> exachem/install-exachem.sh: update the
call sites and cache-key hashFiles() patterns.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… call site

The script now defaults blas_vendor to 'auto' unconditionally; the
Linux-vs-macOS choice (openblas vs auto, since macOS has no BLAS
package installed) is now made explicitly by the workflow step
instead of being baked into the script.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not needed on our CI runners
…ogether

Combines 'Install C++ dependencies' and 'Install ExaChem + TAMM
(Linux)' into a single step, and the two separate cpp-deps/exachem
caches into one cpp-deps-<os>-<uarch>-<hash> cache covering both
CPP_DEPS_PREFIX and EXACHEM_INSTALL_PREFIX. The cache key now also
hashes .pipelines/install-scripts/exachem/**, and the ExaChem-only
condition (Linux, only on cache miss) is preserved inside the merged
step via a bash if instead of a separate step-level if.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…deps

TAMM's CMSB superbuild only discovers already-installed dependencies
via a plain find_package(<Name> CONFIG) against CMAKE_INSTALL_PREFIX
(not the broader CMAKE_PREFIX_PATH) -- e.g. an Eigen3Config.cmake
copied there is found, but a CMAKE_PREFIX_PATH entry pointing at a
different prefix is not, even with 'Found spdlog/ecpint/...' log
lines appearing later in a nested reconfigure. As a result, spdlog
and libecpint -- both already built by install-cpp-deps.sh -- were
being needlessly rebuilt by CMSB (as SPDLOG_External/EcpInt_External)
because ExaChem/TAMM installed into a separate EXACHEM_INSTALL_PREFIX
directory that CMSB's initial dependency scan never saw.

Fix: drop the separate EXACHEM_INSTALL_PREFIX and pass CPP_DEPS_PREFIX
as ExaChem/TAMM's own install prefix too, so CMSB's CMAKE_INSTALL_PREFIX
scan finds the already-built spdlog/ecpint (confirmed their exported
config file names -- spdlogConfig.cmake, ecpint-config.cmake -- match
what find_package(spdlog)/find_package(EcpInt) look for) instead of
rebuilding them from source.

Some other TAMM_DEPENDENCIES remain unavoidably rebuilt by CMSB
regardless of this fix, for reasons unrelated to prefix visibility:
- nlohmann_json: Ubuntu's nlohmann-json3-dev ships headers only, no
  CMake config file at all (verified) -- nothing to point CMSB at.
- BLAS/LAPACK: libopenblas-dev has no LAPACKConfig.cmake/BLASConfig.cmake
  (CMSB's initial probe is CONFIG-mode only); it does get found and
  reused correctly later via CMake's MODULE-mode FindBLAS/FindLAPACK
  once LINALG_VENDOR=OpenBLAS is applied, but CMSB's initial scan
  already queued (and cheaply completes, ~20-30s) its own from-source
  build by then.
- Microsoft.GSL, doctest, HPTT, numactl: never installed by us anywhere
  (not via apt, not via install-cpp-deps.sh), so there is nothing to
  reuse for these; CMSB building them itself is expected and cheap.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TAMM's CMSB Findnumactl.cmake (cmake/find_external/Findnumactl.cmake)
hardcodes NO_DEFAULT_PATH on its find_path()/find_library() calls and
only ever searches CMAKE_INSTALL_PREFIX -- confirmed by reading its
source. apt's libnuma-dev (already installed for this exact purpose)
lives under /usr, which that module never looks at, so it always
rebuilt numactl from source. Seed numa.h/libnuma.so into INSTALL_PREFIX,
the same way as Eigen3 above, so it's found there instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Lists what's built from source (spdlog, BLAS++, LAPACK++, LibInt2,
ECPint, GauXC) vs. reused from the OS (the BLAS/LAPACK implementation
BLAS++/LAPACK++ link against). Also notes, for context, the further
OS-reused dependencies of qdk-chemistry's own C++ build that this
script doesn't touch at all -- verified against cpp/cmake/third_party.cmake
and external/macis/src/macis/CMakeLists.txt: Eigen3, HDF5, Boost,
OpenMP, Threads, MPI (ExaChem builds only), and GoogleTest (used if
found).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Installing TAMM/ExaChem directly into CPP_DEPS_PREFIX (to let CMSB
reuse spdlog/EcpInt from there) caused a serious regression: TAMM's
CMSB superbuild cannot avoid rebuilding its own static, generic-target
BLAS/LAPACK from source (no CONFIG package exists for either from
apt, regardless of LINALG_VENDOR), and once EXACHEM_INSTALL_PREFIX
was merged into CPP_DEPS_PREFIX, that rebuilt libopenblas.a/liblapack.a
landed in the same shared prefix CMAKE_PREFIX_PATH used by the main
qdk-chemistry/MACIS build -- which then linked against it instead of
the real system OpenBLAS, causing widespread SEGFAULTs across
unrelated core tests (BasisSetTest, HamiltonianTest, CholeskyTest,
etc.), confirmed via a live CI run.

Revert to separate prefixes (still installed/built in a single merged
step and cached together, as requested). spdlog and EcpInt go back to
being redundantly rebuilt by CMSB in EXACHEM_INSTALL_PREFIX, same as
before this change -- a small, bounded, and safe cost. The Eigen3 and
numactl seeding fixes are untouched and remain effective either way,
since they were always scoped to EXACHEM_INSTALL_PREFIX directly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Read NWChemEx-Project/CMakeBuild's source (cmake/macros/DependencyMacros.cmake,
cmsb_find_dependency) to find why BLAS/LAPACK were always rebuilt from
source despite LINALG_VENDOR=OpenBLAS: the option CMSB_DEBUG_CMAKE
(despite its name -- upstream's own comment just calls it 'enable lots
of extra CMake printing') defaults to ON and, when on, makes CMSB look
up its externally-buildable dependencies (including BLAS/LAPACK) via
find_package(... CONFIG NO_DEFAULT_PATH). apt's libopenblas-dev ships
no BLASConfig.cmake/LAPACKConfig.cmake, so that search always fails
and CMSB always rebuilds its own static, generic-target OpenBLAS.

With CMSB_DEBUG_CMAKE=OFF, the same dependencies fall back to a plain
find_package(... QUIET), which uses CMake's stock FindBLAS.cmake/
FindLAPACK.cmake (MODULE mode, standard search path) and correctly
finds the already-installed system OpenBLAS -- avoiding the rebuild
entirely, rather than just relocating it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

.pipelines/install-scripts/install-cpp-deps.sh:62

  • JOBS is always set to nproc/sysctl output here, so callers cannot cap dependency build parallelism (e.g., to avoid OOM on constrained runners). This script already supports overriding other knobs via env vars, so JOBS should follow the same pattern (default only when unset).
if command -v nproc >/dev/null 2>&1; then
    JOBS=$(nproc) # Linux
else
    JOBS=$(sysctl -n hw.logicalcpu) # macOS
fi

.pipelines/install-scripts/common.sh:68

  • The get_hash() doc comment says Boost doesn’t declare a hash, but cgmanifest.json now includes a Boost hash. This is misleading for future callers/maintainers; update the comment to be accurate/generic.
# Resolve the declared SHA-1 (`hash`) for an "other" (non-git) type component from cgmanifest.json, by
# component name. Not every "other" entry declares one (e.g. Boost doesn't); callers must check for empty.
get_hash() {

Copilot AI review requested due to automatic review settings September 1, 2026 18:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

.github/workflows/build-and-test.yaml:213

  • The dependency cache key doesn’t include .pipelines/install-scripts/common.sh, but install-cpp-deps.sh and exachem/install-exachem.sh both source it. If common.sh changes, the cache can restore stale installs built with different helper logic. Add common.sh to the hashFiles(...) list (and keep the save key in sync).

This issue also appears on line 257 of the same file.

        key: >-
          cpp-deps-${{ runner.os }}-${{ matrix.uarch }}-
          ${{ hashFiles('cpp/manifest/qdk-chemistry/cgmanifest.json', 'external/macis/manifest/cgmanifest.json', '.pipelines/install-scripts/install-cpp-deps.sh', '.pipelines/install-scripts/install-blaspp.sh', '.pipelines/install-scripts/install-lapackpp.sh', '.pipelines/install-scripts/exachem/**') }}

.pipelines/templates/build-exachem-linux.yml:33

  • The ExaChem cache key doesn’t include .pipelines/install-scripts/common.sh, but multiple scripts in this key list source it (e.g., install-cpp-deps.sh and exachem/install-exachem.sh). If common.sh changes, the cache can restore stale builds even though the build logic changed. Include common.sh in the Cache@2 key inputs so the cache is invalidated correctly.
      "linux-exachem-${{ parameters.march }}-v1" | "${{ parameters.dockerImage }}" |
      cpp/manifest/qdk-chemistry/cgmanifest.json |
      external/macis/manifest/cgmanifest.json |
      .pipelines/install-scripts/build-exachem-linux.sh |
      .pipelines/install-scripts/install-blis.sh |

.pipelines/templates/build-exachem-linux.yml:35

  • The Cache@2 path points to $(System.DefaultWorkingDirectory)/exachem_install, but the docker build writes ./exachem_install under /workspace/qdk-chemistry (i.e., under $(Agent.BuildDirectory)/qdk-chemistry/exachem_install). With the current path, the cache will likely miss and/or save an empty directory. Point the cache path at the actual produced install directory.

This issue also appears on line 49 of the same file.

      .pipelines/install-scripts/exachem/**/*
    path: $(System.DefaultWorkingDirectory)/exachem_install
    cacheHitVar: EXACHEM_INSTALL_CACHE_RESTORED

.pipelines/templates/build-exachem-linux.yml:51

  • This ownership-fix step uses the same $(System.DefaultWorkingDirectory)/exachem_install path as the cache, but the docker build output lives under the checkout directory (.../qdk-chemistry/exachem_install). If the path is wrong, the step won’t fix permissions and later cache/save/artifact steps can fail. Use the same install directory path that the build produces.
- script: |
    sudo chown -R $(id -u):$(id -g) $(System.DefaultWorkingDirectory)/exachem_install
  displayName: Fix ExaChem install directory ownership

.github/workflows/build-and-test.yaml:259

  • The cache save key should match the restore key. Since .pipelines/install-scripts/common.sh is sourced by install scripts, it should be part of the hashFiles(...) inputs here as well; otherwise a change to common.sh won’t invalidate the saved cache entry.
        key: >-
          cpp-deps-${{ runner.os }}-${{ matrix.uarch }}-
          ${{ hashFiles('cpp/manifest/qdk-chemistry/cgmanifest.json', 'external/macis/manifest/cgmanifest.json', '.pipelines/install-scripts/install-cpp-deps.sh', '.pipelines/install-scripts/install-blaspp.sh', '.pipelines/install-scripts/install-lapackpp.sh', '.pipelines/install-scripts/exachem/**') }}

Comment thread .github/workflows/build-and-test.yaml Outdated
…e JOBS

install-cpp-deps.sh computed its own JOBS via a bare nproc()/sysctl call,
ignoring build-and-test.yaml's memory-aware CMAKE_BUILD_PARALLEL_LEVEL (the
fix for the OOM failures during ExaChem/CMSB builds) entirely -- so its own
nlohmann_json/googletest/Catch2/spdlog/libint2/ecpint/gauxc builds were
still using every core regardless of available memory. JOBS now falls back
to CMAKE_BUILD_PARALLEL_LEVEL when not explicitly set, and the call site
passes it explicitly for clarity, matching how install-exachem.sh is
already invoked.

Also drop the separate LIBINT_JOBS=4 cap: libint2's build now just uses the
same memory-aware JOBS as everything else in this script, rather than an
extra, redundant throttle layered on top.
Copilot AI review requested due to automatic review settings September 1, 2026 18:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build-and-test.yaml:213

  • The cache key hashFiles list omits .pipelines/install-scripts/common.sh, but both install-cpp-deps.sh and exachem/install-exachem.sh source it. Changes to common.sh (e.g., download verification behavior) would not invalidate this cache, risking stale or mismatched dependency installs being restored.

This issue also appears on line 260 of the same file.

          ${{ hashFiles('cpp/manifest/qdk-chemistry/cgmanifest.json', 'external/macis/manifest/cgmanifest.json', '.pipelines/install-scripts/install-cpp-deps.sh', '.pipelines/install-scripts/install-blaspp.sh', '.pipelines/install-scripts/install-lapackpp.sh', '.pipelines/install-scripts/exachem/**') }}

.pipelines/templates/build-exachem-linux.yml:33

  • The Cache@2 key inputs omit .pipelines/install-scripts/common.sh, but the keyed scripts (install-cpp-deps.sh and exachem/install-exachem.sh) source it. Without including common.sh, changes to shared helpers won’t invalidate the ExaChem install cache and can restore stale builds.
      .pipelines/install-scripts/install-cpp-deps.sh |
      .pipelines/install-scripts/install-blaspp.sh |
      .pipelines/install-scripts/install-lapackpp.sh |
      .pipelines/install-scripts/exachem/**/*

.github/workflows/build-and-test.yaml:260

  • Same cache-key issue as above: the save-step hashFiles list omits .pipelines/install-scripts/common.sh, so updates to shared helpers won’t invalidate the saved cache entry.
          ${{ hashFiles('cpp/manifest/qdk-chemistry/cgmanifest.json', 'external/macis/manifest/cgmanifest.json', '.pipelines/install-scripts/install-cpp-deps.sh', '.pipelines/install-scripts/install-blaspp.sh', '.pipelines/install-scripts/install-lapackpp.sh', '.pipelines/install-scripts/exachem/**') }}

@lorisercole Loris Ercole (lorisercole) changed the title Add ExaChem CI integration for GHA and ADO pipelines Add ExaChem CI integration for GHA and ADO pipelines + reorganize build scripts Sep 1, 2026
GHA's Windows msvc/clang-cl ARM64 legs still hit C1060 (compiler out of
heap space) in engine.impl.h at 7 GB/job. Lower the job count further
across GHA (Linux + both Windows steps) and ADO's pip-wheels/cpp-deps
Windows scripts to keep the budget consistent everywhere it's set.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 1, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The current workflow/scripts have a couple of reliability issues (cache directory existence across matrix legs and non-idempotent shallow checkouts) that should be fixed to avoid CI flakes.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build-and-test.yaml:96

  • actions/cache is configured to restore/save ${{ env.EXACHEM_INSTALL_PREFIX }} for every matrix leg, but ExaChem is only built on Linux. On macOS runs, this directory is never created, which can cause the cache step to warn and skip saving/restoring. Pre-create the directory early in the job so cache restore/save has a real path regardless of OS.
        echo "PATH=${{ github.workspace }}/install/bin:${{ github.workspace }}/install/lib:$PATH" >> $GITHUB_ENV
        echo "PYTHONPATH=${{ github.workspace }}/install/lib/python${{ matrix.python-version }}/site-packages:${{ github.workspace }}/install/lib:$PYTHONPATH" >> $GITHUB_ENV
        # ${LD_LIBRARY_PATH:+...} avoids a trailing ":" when unset -- the dynamic loader treats an empty
        # path component as the current working directory, an untrusted-search-path risk.
        echo "LD_LIBRARY_PATH=${{ github.workspace }}/install/lib${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}" >> $GITHUB_ENV

.pipelines/install-scripts/install-cpp-deps.sh:162

  • shallow_checkout does git remote add origin ... and will fail if the destination directory already exists from a previous interrupted run (remote already defined). Since CI workspaces can be retried/reused, remove the target dir before calling shallow_checkout (and consider applying the same pattern to the other dependency checkouts below).
echo "=== Installing nlohmann_json ==="
shallow_checkout https://github.com/nlohmann/json.git "$NJSON_COMMIT" nlohmann_json
cd nlohmann_json
  • Files reviewed: 25/26 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 1, 2026 21:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

There are concrete maintainability/documentation issues in the new dependency installer (Libint extraction block and GauXC/Catch2 comment) that should be corrected to prevent confusion and reduce the risk of incorrect builds.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.pipelines/install-scripts/install-cpp-deps.sh:238

  • The Libint extraction comments are outdated (they reference the old 2.9.0 tarball) and the directory auto-detection can pick up a stale libint-* directory if a previous run left one behind. This can lead to configuring/building the wrong extracted tree. Consider clearing any existing libint-* directories before extracting, and make the comment version-agnostic.
    .pipelines/install-scripts/install-cpp-deps.sh:286
  • This comment says GauXC testing requires Catch2 v2, but the manifests pin Catch2 v3.3.2. If the rationale is simply that GauXC tests are not needed in this CI dependency build, update the comment to avoid a misleading version-specific claim.
  • Files reviewed: 25/26 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 2, 2026 11:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The ADO ExaChem cache/ownership steps currently target a directory path inconsistent with where the Docker build writes the install, and cache invalidation in GHA misses the shared common.sh helper.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

.pipelines/templates/build-exachem-linux.yml:50

  • This ownership-fix step uses $(System.DefaultWorkingDirectory)/exachem_install, but the build writes exachem_install under the repo checkout directory (see the Docker -w /workspace/qdk-chemistry above, and the artifact upload path in .pipelines/python-wheels.yaml). If these paths differ, chown will fail or silently miss the actual install dir.
- script: |
    sudo chown -R $(id -u):$(id -g) $(System.DefaultWorkingDirectory)/exachem_install

.github/workflows/build-and-test.yaml:248

  • Same as the restore key above: the save key should also include .pipelines/install-scripts/common.sh so cache invalidation stays correct when shared helper logic changes.
        key: >-
          cpp-deps-${{ runner.os }}-${{ matrix.uarch }}-
          ${{ hashFiles('cpp/manifest/qdk-chemistry/cgmanifest.json', 'external/macis/manifest/cgmanifest.json', '.pipelines/install-scripts/install-cpp-deps.sh', '.pipelines/install-scripts/install-blaspp.sh', '.pipelines/install-scripts/install-lapackpp.sh', '.pipelines/install-scripts/exachem/**') }}
  • Files reviewed: 24/25 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread .github/workflows/build-and-test.yaml Outdated
Comment thread .pipelines/templates/build-exachem-linux.yml
Comment thread .pipelines/install-scripts/install-cpp-deps.sh
common.sh is sourced by install-cpp-deps.sh and install-exachem.sh
(download/hash-verification logic) but wasn't part of either cache
key's hashFiles/Cache@2 input list, so changes to it wouldn't
invalidate a cached install tree.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 2, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces wide-ranging CI/pipeline behavior changes (new caches, artifacts, external MPI application build/install flow) that are difficult to fully validate via diff-only review.

Review details
  • Files reviewed: 24/25 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The merge from main dropped the job-wide 'Set memory-aware build
parallelism' step for the Linux/coverage matrix, leaving ExaChem/TAMM's
build to run at the matrix's static build-parallel (raw core count,
sized for running already-built binaries, not compiling from scratch)
and get OOM-killed again. Compute the RAM-capped job count locally
inside this one step instead of a separate job-wide step, so it only
affects the dependency/ExaChem build and not later test-run steps.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 2, 2026 20:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The GHA cache-save step can silently skip saving the C++ deps cache on non-Linux legs because EXACHEM_INSTALL_PREFIX may not exist, causing path validation to skip caching entirely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.pipelines/install-scripts/install-cpp-deps.sh:237

  • This comment still references the old libint-2.9.0 tarball name, but the manifest pins Libint 2.13.1. Updating it will avoid confusion when debugging extraction/layout issues.
  • Files reviewed: 24/25 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +244 to +251
if [ "${{ matrix.os-name }}" == "Linux" ]; then
# A separate prefix from CPP_DEPS_PREFIX: TAMM's CMSB superbuild rebuilds its own BLAS/LAPACK, which
# would shadow the real OpenBLAS for the main build otherwise (caused SEGFAULTs).
INSTALL_PREFIX="${{ env.EXACHEM_INSTALL_PREFIX }}" \
BUILD_ROOT="${{ github.workspace }}/exachem_build" \
JOBS="$jobs" \
bash ${{ github.workspace }}/.pipelines/install-scripts/exachem/install-exachem.sh
fi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD Pipelines & dev tools github_actions Pull requests that update GitHub Actions code ready-to-review A PR that is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants