Repository navigation
fix(umbp): dlopen libhipfile instead of linking it - #650
Merged
Merged
Conversation
`import mori` dies on any host whose ROCm ships without hipFile:
File ".../mori/cpp/__init__.py", line 44, in <module>
ImportError: libhipfile.so.0: cannot open shared object file
GdsEngine is one optional transfer path inside umbp_common, and
umbp_common is linked into libmori_pybinds.so -- so `hip::hipfile`
became a DT_NEEDED on the Python extension. Worse, the dependency was
auto-detected (`find_package(hipfile QUIET)`), so whether a wheel
carries it depends on what happened to be installed on the build host:
built where hipFile exists, it cannot be imported anywhere else. That
is what broke ATOM CI -- four accuracy jobs died before the first
token, none of which touch UMBP at all.
Build against the headers, resolve the library at runtime. hipfile_dl
dlopen's libhipfile.so.0 on first use and dlsym's the four entry points
GdsEngine calls; the function-pointer types come from <hipfile.h> via
decltype, so they cannot drift from the real signatures. PoolClient
registers GdsEngine only when GdsEngine::Available(), and without the
library RegisterFile returns an invalid ref, leaving the SSD path
host-staged exactly as an hipfile-less build already does.
Note that dropping the link alone is not enough -- `patchelf
--remove-needed` on a shipped wheel just moves the failure to
"undefined symbol: hipFileHandleDeregister". The calls themselves have
to go through dlsym.
Verified in a container with a stand-in libhipfile installed at build
time: no DT_NEEDED on the extension, GdsEngine still compiled in,
`import mori.shmem` succeeds with the library absent, and with it
present Available() is true and the library's hipFileHandleRegister is
actually called. test_gds_engine passes both ways.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Problem
import morifails on any host whose ROCm ships without hipFile:GdsEngineis one optional transfer path insideumbp_common, andumbp_commonis linked intolibmori_pybinds.so— sotarget_link_libraries(umbp_common PUBLIC hip::hipfile)(added in #540) became aDT_NEEDEDon the Python extension.Because the dependency is auto-detected (
find_package(hipfile QUIET)), whether a wheel carries it depends on what happened to be installed on the build host. A wheel built where hipFile exists cannot be imported anywhere else.This broke ROCm/ATOM CI: four accuracy jobs died before the first token —
import mori.shmem→ all 8 DP ranks down →Engine Core Mgr: Received unexpected SHUTDOWN signal. None of those jobs use UMBP at all;rocm/atom-dev:latestruns ROCm 7.2.4, which has no hipFile.Fix
Build against the headers, resolve the library at runtime.
hipfile_dl.{h,cpp}:dlopen("libhipfile.so.0")on first use,dlsymthe four entry pointsGdsEnginecalls, cached understd::call_once. Function-pointer types come from<hipfile.h>viadecltype, so they cannot drift from the real signatures.GdsEngine::Available()reports whether the library loaded.PoolClientregisters the engine only when it did; without itRegisterFilereturns an invalid ref and the SSD path stays host-staged — exactly what an hipfile-less build already does.hip::hipfile'sINTERFACE_INCLUDE_DIRECTORIESand links${CMAKE_DL_LIBS}.GLOBALis kept so the tests directory can still gatetest_gds_engineon the same target; nothing links it.No capability is lost: GDS is still compiled in whenever the headers are present, and it works wherever the library exists.
Dropping the link alone is not enough.
patchelf --remove-needed libhipfile.so.0on a shipped wheel just moves the failure toundefined symbol: hipFileHandleDeregister— the calls themselves have to go throughdlsym. SoPUBLIC→PRIVATEor--as-neededwould not have fixed this.Verification
Reproduced and fixed locally, in a container with a stand-in libhipfile installed at build time:
readelf -d libmori_pybinds.so | grep hipfileNEEDED libhipfile.so.0import mori.shmem, library absentImportErrorimport mori.shmem, library presentGdsEnginesymbols compiled intest_gds_engineA probe confirms both directions of the runtime decision:
Caveats, stated plainly: the library used in verification was a stand-in that implements the four symbols, so symbol resolution and dispatch are covered but a real GDS disk read is not; and the import check ran in a Python 3.10 build container rather than in
rocm/atom-devitself (different Python and protobuf/gRPC versions). Onrocm/atom-dev:latest,libhipfile.so.0was confirmed to be the only unresolved dependency of the shippedamd_mori_nightlywheel.Follow-up, not in this PR
libmori_pybinds.sostill hard-links gRPC / protobuf / abseil / OpenSSL throughumbp_common. Those happen to resolve in ATOM's image today, but the same class ofImportErrorreturns the moment a base image moves to a different protobuf soname. Makingimport moriindependent of UMBP's transitive dependencies — e.g. UMBP as a separately-loadedlibmori_umbp.so— would close the category rather than this one instance.🤖 Generated with Claude Code