Conversation
5af18e7 to
b282597
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #861 +/- ##
=======================================
Coverage 93.42% 93.42%
=======================================
Files 113 113
Lines 16924 16924
Branches 3010 3010
=======================================
+ Hits 15811 15812 +1
+ Misses 1113 1112 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@egpbos could you clarify the motivation for this PR? Do we need to support both pixi and the current approach - or should we just have this one and deprecate the other? Also, please can you test the Linux case to ensure that it also functions as expected? |
|
Motivation is basically that I wanted to try it out just to see how much it would change, both as a possible ramp-up to the solutions we came up with in #721 but also simply because pixi is gaining traction and I just wanted to try it out myself on my new laptop :) Since it turned out to need only very minimal changes (because pixi envs are conda backwards compatible, e.g. they also define conda environment variables if none exist, allowing scripts (like ours) that depend on those to just treat it as a conda env) I thought we could just support it, since it's basically for free. Regarding #721: pixi environments also have a convenient built-in mechanism for defining other parts of your environment, like the RAD_DIR, FWLDATA and other env vars we currently leak into shell rc. Pixi thus makes it much easier to completely isolate multiple PROTEUS installs (+ deps) on one machine.
In progress! (This was how I found the missing netcdf-fortran on Kapteyn, now trying on Habrok instead.) |
|
I am confused, didn't we in some issue discuss that we do not want go with pixi? |
|
The discussion in #721 was not conclusive, I thought. We discussed two options there. What I'm going for here is really besides both options (it doesn't solve the Julia or env overreach issues directly). The main find here is that supporting pixi doesn't need much change in the setup at all (see the commit diff, most lines are in the lock file which is updated automatically by pixi; in fact, we could keep it out, that works too). That means that we simply give users an alternative to conda without any maintenance cost. We already almost were supporting it. I do foresee that a next step could be to solve the env overreach by delegating that to pixi (when pixi is used, with a fallback to the current behavior). I could also try that out as part of this PR, so we can see the whole thing in one go and then judge whether it works for us. |
b282597 to
12c7685
Compare
5a786f0 to
27d2609
Compare
|
Alright, this is now ready for review. Took a while because I changed a few (minor) things (and spawned a bunch of separate issues while digging into some details). Verified on Linux as well now (Kapteyn, Rocky 9.8). Summarizing, this PR finally just makes three small changes:
The first two are the only things necessary for pixi support, the third is just to prevent clutter. ** Note that by pinning the openssl version in the pixi.toml file (which is used for environment creation) most if not all of the self-heal and repair machinery in install.sh becomes unnecessary. This is why, despite the fact that all that machinery needs conda, the pixi install worked. This implies that we might also simplify conda env creation in a similar way by creating a (very similar looking) environment.yml. Related to #885 (comment), which pins the other side of this incompatible pair. Opened #887 to look into this further. Unfortunately we can't use one file, so if we want to keep both conda and pixi support we'll have to duplicate this, but it's only a small file (much smaller anyway than the openssl repair code in install.sh). |
pixi sets CONDA_DEFAULT_ENV and CONDA_PREFIX, so install.sh runs against a pixi environment unmodified. What such an environment does not have is a conda binary, so get_cvode.sh failed at the SUNDIALS install step and install.sh downgraded that to a warning, silently leaving Aragog on the step-size-fragile scipy Radau fallback. The manifest declares sundials, and the script now installs it only when the prefix does not already provide it, which leaves the conda path unchanged. pip is declared because a conda-forge python does not pull it in and install.sh calls pip directly. openssl is held at 3.5 or newer, which juliacall needs to match the Julia 1.12 OpenSSL_jll. hdf5 and libnetcdf are deliberately absent: the netCDF4 wheel carries its own, so the MPI build conflict that install.sh repairs under conda cannot arise. pixi.lock is gitignored rather than committed, so the environment resolves against the current package set instead of a pinned one. Environment variables stay with install.sh and the shell rc file, so nothing changes for existing conda installations. Verified on osx-arm64: proteus doctor reports all checks passed, the unit suite is green, and the AGNI transparent-limit and grey-gas coupling tests pass.
The presence check only looked for libsundials_cvode in the prefix, so an environment carrying SUNDIALS 6.x satisfied it and the wrapper was then built against an ABI it does not support. Read the major out of sundials_config.h instead and fail with a clear message. Checking the header rather than the library also covers the runtime-only case, where the shared object is present but the headers the build needs are not.
27d2609 to
44c68ee
Compare
|
Forgot this! A howto for the reviewers. Using pixiInstall pixi, then from a PROTEUS clone: That replaces step 2 of the installation guide, the Afterwards, prefix commands with Or use The lock file
KapteynKapteyn has no netcdf-fortran, so the same two steps the Kapteyn guide documents for conda are needed here, in their pixi form: and, in [activation.env]
LD_LIBRARY_PATH = "$CONDA_PREFIX/lib"Without the second, SOCRATES builds but its binaries fail at run time with This is not a good solution: both are hand edits to a tracked file, so every Kapteyn user carries a local diff. #886 looks into removing the need for the run-time path altogether, which would likely make the second step unnecessary. Not preempting that here. |
nichollsh
left a comment
There was a problem hiding this comment.
Thanks for this @egpbos. Pixi is new to me, but it seems great!
I ran the Pixi install steps as you've described them in this PR, and it works well on my laptop (Fedora 44).
My main suggestion is that we switch to requiring Julia 1.11 or 1.13, and deprecate 1.12 - this is consistent with the approach taken by Obliqua and AGNI. Would it be possible to 'containerise' Julia in the same manner as Conda, too? At the moment this script interferes with my system's Julia install (related to #721 ?). It makes some very presumptive changes to my bashrc file relating to Julia.
Overall really promising! I think we should seriously consider making pixi our 'standard' method to install PROTEUS and its environment.
timlichtenberg
left a comment
There was a problem hiding this comment.
Tested this locally, works as advertised.
A few things needed before this merges. Docs are not touched: installation.md and manual_installation.md still only describe conda, and pixi should be added there as an install option. No CI job touches the pixi path, so it has zero regression coverage; worth at least one workflow step. get_cvode.sh falls back to a literal conda binary when $CONDA_EXE is unset (line 27), which is the case under pixi; not hit today since pixi.toml always pre-installs sundials, but if that pin moves this breaks with a confusing error on a pixi-only machine. Worth a pixi-aware fallback, or at least a comment.
|
Thanks for the reviews! @nichollsh indeed, the Julia environment issue already existed before this PR and is tracked by #721. I did not want to touch it here, because my Julia-fu is not strong enough. Perhaps we can look into it next week. About Julia versions: so, to get this to work I had to switch from 1.11 to 1.12. This is where #885 came from for me. Perhaps my issue was something different that can still be fixed, but if we're moving all our submodules to 1.13 anyway, should we just stop supporting everything below 1.13? I do think this is outside of the scope of this PR, what do you think? I'm happy to open a separate PR based on this and #885, which also cleans up some of the repair machinery (to do a full cleanup we also need to treat #887, but this can be yet another separate PR). @timlichtenberg Indeed, I skipped docs and testing. I'd be willing to add docs, up to you, but I'd personally like to keep it very minimal for now, perhaps even explicitly label it [experimental] in the docs. I don't know if we have something like that already? The point is: while this PR already allows us to use pixi, we still have some points to figure out before deciding whether how far we'd want to go with it (e.g. how does letting it handle the environment variables work? can it also deal with Julia?); this also relates to @nichollsh's enthousiasm ;) I personally also like it so far, but haven't decided on whether it makes sense to go all-in or even part-in, it depends on the added value vs the usage cost and we still need to try out some things. So, tl;dr: shall I add e.g. the above short howto to the docs in a separate page and link it as an experimental alternative from the main install page? That way people can start trying it out if they like, but we can also easily remove it again if it doesn't work out. The same hesitation applies to your other comments, shortly:
|
|
Thanks for these details. I would be happy to just require Julia 1.13 for everything, if that simplifies things. |
Description
Adds a
pixi.tomlso PROTEUS can be installed into a pixi environment, and removes the one thing that stoppedinstall.shfrom completing in one.Additive and opt-in: the conda path is unchanged and a contributor who never runs
pixisees no difference. Related to #721 but does not pre-empt the design decision under discussion there.pixi.tomldeclares python 3.12, pip, openssl >= 3.5 and sundials.pipbecause a conda-forge python does not include it andinstall.shcalls pip directly;opensslbecause juliacall must match the Julia 1.12OpenSSL_jll.hdf5andlibnetcdfare deliberately omitted, since thenetCDF4wheel carries its own and the MPI build conflictinstall.shrepairs under conda cannot arise.tools/get_cvode.shinstalls SUNDIALS only when the prefix does not already provide it. A pixi environment has nocondabinary, so the unconditionalconda installfailed andinstall.shdowngraded that to a warning, silently leaving Aragog on the scipy Radau fallback. When SUNDIALS is absent the original conda command runs unchanged.pixi.lockis gitignored rather than committed, so the environment resolves against the current package set.is related to #721
Validation of changes
macOS 26.6 on Apple Silicon, Python 3.12.14, pixi 0.79.0. Installed from a clean clone with
pixi run bash install.sh, which completed all phases without modification.proteus doctor: all checks passed, every submodule editable.proteus start --offline -c input/dummy.toml: completes.The conda path was not re-run, since no conda is installed on the test machine; the macOS CI job covers it.
Checklist
Docs are unchanged: documenting pixi as a supported route would present the #721 decision as settled. No tests added; this is a dependency manifest plus a guard in an installer script, covered by the suites above.