fix: bundle the Open MPI runtime so the packaged Palace actually runs - #40
Open
benvial wants to merge 2 commits into
Open
fix: bundle the Open MPI runtime so the packaged Palace actually runs#40benvial wants to merge 2 commits into
benvial wants to merge 2 commits into
Conversation
The staging script collected dependencies from ldd output alone, which sees only the Open MPI shared libraries. It misses the two other halves of an Open MPI installation: the MCA components (lib/openmpi, lib/pmix), which are dlopened, and orted, which Open MPI execs to bootstrap even a single rank. The resulting wheel shipped a Palace binary that could not start on any machine without a matching host Open MPI, aborting in opal_shmem_base_select before MPI_Init -- for `palace --version` as much as for a real simulation, and for `--serial` as much as under mpirun. Stage the components, the runtime executables and the help text, walk their dependency trees, and give them their own RPATHs. Install the upstream launcher as palace-launcher behind a wrapper that points Open MPI at the bundled runtime and puts the bundled bin dir first on PATH, so the launcher no longer resolves `mpirun` to whatever MPI the host happens to have -- which may be a different implementation entirely. Guard the build against staging one Open MPI's components beside another's libraries, and gate the release on a smoke test that runs `palace --version` from the fresh wheel with the runner's Open MPI hidden, before upload.
_binary_is_runnable checked only is_file() and os.access(X_OK), ignoring the lib_dir and timeout arguments it accepted. A runtime whose bundled MPI is incomplete passes both checks and then fails at the user's first simulation, and install_palace_runtime reported "OK" for it while check_palace_runtime reported FAIL -- the two disagreed about the same runtime. Run `--version` and require a clean exit, cached per binary so repeated resolution costs one launch. Verify on install too, on the fresh-unpack path and on the cached early return, with --skip-verify for installing on a machine that will not run Palace. Rejecting a broken runtime during resolution would have swallowed the cause, leaving a bare "No Palace executable found", so record the failed launch output and surface it in that error. Also copy share/ and restore the executable bit across all of bin/ when unpacking a cached runtime; zip extraction drops it, and orted must be executable for the runtime to start. The docs workflow installs with --skip-verify until a wheel is rebuilt through the fixed staging pipeline.
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
1 participant
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.
The problem
The packaged Palace binary can't start — it can't even print its version:
Same with
--serial, so it isn't aboutmpirunor the host's MPI setup.The cause
stage_palace_binary.shcollects dependencies fromldd, which finds the Open MPIlibraries but not the two pieces it can't see: the MCA components (
lib/openmpi/,lib/pmix/), which aredlopened, andorted, which Open MPI execs to start a rank —even a single serial one. The wheel ships part of an Open MPI install, so it only runs
where a matching one already exists to borrow from.
Changes
Packaging — stage the MCA components,
orted/orterunand the help text, walk theirdependencies, set their RPATHs. The upstream launcher becomes
palace-launcherbehind awrapper that points Open MPI at the bundled copy and puts the bundled
bin/first onPATH; otherwise the launcher grabs whatevermpirunis installed, which on my machinewas MPICH. The build now fails if components and libraries come from different Open MPI
builds.
CI — after building the wheel and before uploading it, install into a clean venv, hide
the runner's Open MPI, and require
palace --versionto work.Runtime —
_binary_is_runnable()only checked that the file existed and wasexecutable, which is how an unusable runtime got reported as fine; it now runs
--version(cached, one launch per process).
install_palace_runtime()verifies too, so install andcheck no longer disagree. Rejections say why, instead of "No Palace executable found".
Tests — 12 in
tests/test_palace_runtime_resolution.py, using small executablestand-ins rather than mocking
subprocess.Verification
Built a wheel through the fixed pipeline, installed it into a clean venv, and ran it with
MPICH's
mpirunfirst onPATH—--version,-np 2and--serialall printPalace version: v0.17.0-272-gb22f654ab. 31 unit tests pass, and reverting_binary_is_runnableconfirms the new tests fail against the old version.Questions
palace-cpu-wheel.ymlonly runs onworkflow_dispatchor apalace-cpu-v*tag, so thenew smoke test doesn't run on this PR. Could you kick off a manual run? It's the check
this PR rests on, and so far it's only been proven on my laptop.
docs.ymlinstalls with--skip-verifyfor now, since the released wheel is the brokenone and verification would take the docs build down. Both lines have a comment. Happy to
drop it and let docs fail loudly instead.
Unrelated aside, take it or leave it: building the binary package with
cibuildwheelina manylinux image would make it PyPI-installable, which would let the empty
palace-cpuextra become a real dependency and retire the download-from-Releases path in
palace_runtime.py. It wouldn't have caught this bug —auditwheelfollowslddtoo —so it's a separate change, and only worth it if PyPI distribution is a goal.
Once a new
palace-cpu-v*wheel is released, anyone with a cached runtime should clear~/.cache/palacetoolkit/runtime/.