Skip to content

Support detached Gadi Pixi environments in PETSc builds - #516

Merged
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:codex/gadi-detached-pixi-petsc-build
Aug 12, 2026
Merged

Support detached Gadi Pixi environments in PETSc builds#516
lmoresi merged 1 commit into
underworldcode:developmentfrom
gthyagi:codex/gadi-detached-pixi-petsc-build

Conversation

@gthyagi

Copy link
Copy Markdown
Contributor

Summary

  • accept detached Pixi hpc environments by validating the active environment name, project root, Conda prefix, and Python executable instead of requiring a hard-coded .pixi/envs/hpc path
  • prioritize the Pixi C++ runtime during Gadi PETSc and petsc4py builds for NumPy 2 compatibility
  • derive Gadi build concurrency from PBS_NCPUS, with a safe login-shell fallback, instead of hard-coding 40 jobs
  • modernize executable lookup and PETSc architecture diagnostics

This is a focused cherry-pick of c42b9b39 from feature/mantle-convection-benchmarks; no mantle-convection implementation commits are included.

Validation

  • bash -n petsc-custom/build-petsc.sh
  • git diff --check upstream/development...HEAD
  • mocked Gadi activation accepts a detached hpc prefix with the correct project root
  • mocked Gadi activation rejects a mismatched Pixi project root
  • mocked Gadi activation rejects a non-numeric PBS_NCPUS
  • confirmed on Gadi with a detached environment under /scratch: NumPy 2.4.6, OpenMPI 4.1.7, PETSc 3.25.4, parallel h5py, and editable UW3 import successfully

Replace the hard-coded .pixi environment path test with validation of the active hpc environment, project root, Conda prefix, and Python executable so detached environments on scratch are accepted safely.
Prioritize the Pixi C++ runtime during Gadi configure and petsc4py builds for NumPy 2 compatibility, derive build concurrency from PBS_NCPUS instead of oversubscribing with 40 jobs, and update versioned PETSc architecture diagnostics.
Also replace legacy which calls and resolve ShellCheck declaration warnings.

@lmoresilmoresi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adversarial review — PR #516: Support detached Gadi Pixi environments in PETSc builds

Scope reviewed: single commit 670a159f, one file — petsc-custom/build-petsc.sh (+60/−20).
Verified against the true merge-base (9eee174d); development has not touched this file since
the branch point, so there is no conflict risk. CI: both checks green (Style Gates, test_uw3) —
though neither exercises this script, so CI green is table stakes, not evidence.

Verdict: approve with minor suggestions. The logic is sound, the local (macOS/dev) path is
untouched, and the new require_hpc_pixi_env gate is strictly better than the old PATH-grep.
Findings below are Medium at worst; none block merge.


What we verified (the non-negotiables)

  • Local workflow unchanged. The local cluster branch, local configure invocation, and
    setup_local_macos_openmpi_build_env are byte-identical. The only changes outside the
    kaiju/gadi branches are a header-comment fix, whichcommand -v in the gadi-only
    configure path, and a local x=$(...) → declare-then-assign split in list_versions
    (SC2155 hygiene, behaviourally identical). ./uw and the petsc-local-* pixi tasks call
    the script unchanged.
  • PETSc non-relocatability is not worsened for the local build.PETSC_DIR is still
    ${SCRIPT_DIR}/petsc; nothing machine-specific is hardcoded (the only absolute paths are
    the pre-existing Gadi module paths /apps/ucc/1.3.0/lib, /usr/bin/gcc etc., inside the
    gadi-only function).
  • The detached-env fix is the right mechanism. Pixi detached environments move the env
    prefix out of .pixi/envs/, which is exactly why the old gate
    (grep '\.pixi/envs/hpc/bin' on $PATH) refused them. The replacement validates
    PIXI_ENVIRONMENT_NAME == hpc, a live CONDA_PREFIX, PIXI_PROJECT_ROOT resolving
    (physically, pwd -P on both sides) to the repo containing the script, and python3 on
    PATH being ${CONDA_PREFIX}/bin/python3. That is stricter and more honest than the PATH
    grep — it also correctly refuses sourcing the shared /g/data activation against a
    personal clone, which the old grep would have waved through.
  • No empty-element hole in LD_LIBRARY_PATH.${CONDA_PREFIX}/lib is interpolated into
    LD_LIBRARY_PATH in setup_gadi_build_env; an unset CONDA_PREFIX would create an empty
    element (= cwd to the loader). It cannot be unset: the cluster-config case runs at
    top-level on every invocation and require_hpc_pixi_env exits first if CONDA_PREFIX is
    empty. Ordering holds for both configure_petsc and build_petsc entry points.
  • _python capture ordering preserved. The pixi python is still captured before
    setup_gadi_build_env prepends /usr/bin to PATH — the one subtle invariant in this file,
    and the diff keeps it (only whichcommand -v).
  • PBS_NCPUS validation is anchored and rejects 0 and non-numeric; fallback of 1 on a
    login shell replaces the old hardcoded --with-make-np=40, which would have hammered a
    login node. Improvement.

Findings (severity-ranked)

  1. [Medium] Kaiju is collateral and untested. The stricter gate is applied to the kaiju
    branch too, but every validation claim in the PR body is Gadi-only (mocked + real). The
    new gate requires the activation to export PIXI_ENVIRONMENT_NAME, CONDA_PREFIX, and
    PIXI_PROJECT_ROOT — true of pixi shell-hook output, but the kaiju activation script
    lives in an external repo (jcgraciosa/uw3-hpc-baremetal-install-run) and we cannot
    verify it here. An activation that merely prepends .pixi/envs/hpc/bin to PATH (the thing
    the old check tested for) now fails. Before merge, either confirm kaiju_install_user.sh
    uses pixi shell-hook, or ask jcgraciosa to smoke-test activation + ./build-petsc.sh help
    on kaiju.

  2. [Medium] The detached env prefix becomes a load-bearing path for the PETSc arch — and on
    Gadi it will typically live under /scratch.
    Configure runs under the detached env's
    python (recorded in reconfigure-*.py / makefiles), petsc4py is built against that
    python, and NumPy 2 at build time depends on ${CONDA_PREFIX}/lib's libstdc++. NCI purges
    /scratch files untouched for ~100 days, and pixi clean / a project-path move relocates
    the detached prefix — either strands a PETSc arch that still exists but can no longer
    reconfigure or import petsc4py coherently, with a confusing failure signature. This is an
    ops footgun, not a script bug, but our house rule ("PETSc is not relocatable") now
    implicitly extends to "…and neither is the env it was configured from." One paragraph in
    docs/developer/guides/hpc-cluster-setup.md (which this PR does not touch) would pay for
    itself. The PR also adds no docs for the detached-env workflow it exists to support.

  3. [Low] ${CONDA_PREFIX}/lib ahead of /usr/lib64 is safe today because the hpc
    feature is pure-Python + patchelf.
    No conda libmpi/libhdf5 can shadow the module
    libraries, so contamination is limited to the intended newer libstdc++ plus python's own
    runtime deps. But this safety is a property of pixi.toml, enforced nowhere: if someone
    later adds a conda-compiled dependency to [feature.hpc.dependencies], configure-time
    test executables silently start resolving against conda libraries. Worth a one-line
    comment at the LD_LIBRARY_PATH export ("depends on the hpc feature staying pure-Python")
    or in the pixi.toml hpc block.

  4. [Low] Silent -j1 fallback on login shells.PBS_NCPUS unset → make-np=1 with no
    warning. A full PETSc build at -j1 will exceed Gadi's login-node CPU limits anyway, so
    the practical effect is "build dies mid-way hours in" rather than "clear message up
    front". Suggest an explicit echo "PBS_NCPUS not set — make-np=1; run the build step inside a PBS job" when the fallback engages.

  5. [Note, outside the diff] The PR body validates with an "editable UW3 import" on Gadi.
    Repo policy is never editable installs — the rationale (.pth files in the env plus
    .so in the shared source tree contaminating other envs) applies on Gadi with detached
    envs too, since the source tree is still shared across envs. Not a defect of this diff
    (build-petsc.sh does not install UW3), but the external Gadi install scripts should be
    checked for pip install -e before that workflow calcifies.

  6. [Nit] (a) The two # shellcheck disable=SC2218 directives are stale —
    require_hpc_pixi_env is defined above its call sites, so SC2218 cannot fire; delete
    them. (b) The kaiju configure block still carries the literal TAB on --with-slepc4py=1
    that the gadi block fixed. (c) The error messages no longer name the specific activation
    script (gadi_install_shared.sh → "the cluster activation script") — defensible now that
    user/shared variants coexist, but the docs are the only remaining pointer, so finding 2's
    doc paragraph matters more.

What's good

Focused cherry-pick with no mantle-convection payload, as advertised. The mocked negative
tests in the PR body (wrong project root, non-numeric PBS_NCPUS) are exactly the negative
controls we ask for. command -v over which, declare-then-assign in list_versions, and
the anchored PBS_NCPUS regex are all correct hygiene. The project-root equality check is a
genuine safety improvement over the old PATH grep — it catches env/source-tree mismatch,
which the old check could not.

— review conducted read-only against pr516-review (fetch of pull/516/head), 2026-08-12

@lmoresi
lmoresi merged commit c1cddb9 into underworldcode:developmentAug 12, 2026
2 checks passed
@gthyagi
gthyagi deleted the codex/gadi-detached-pixi-petsc-build branch August 16, 2026 13:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@gthyagi@lmoresi