-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
prefer wheel-provided libucxx.so in load_library, use RTLD_LOCAL #322
Changes from 2 commits
eb197d7
2b520c7
962a722
c69bf83
4191fad
2928242
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,35 @@ | |
import ctypes | ||
import os | ||
|
||
# Loading with RTLD_LOCAL adds the library itself to the loader's | ||
# loaded library cache without loading any symbols into the global | ||
# namespace. This allows libraries that express a dependency on | ||
# this library to be loaded later and successfully satisfy this dependency | ||
# without polluting the global symbol table with symbols from | ||
# libucxx that could conflict with symbols from other DSOs. | ||
PREFERRED_LOAD_FLAG = ctypes.RTLD_LOCAL | ||
|
||
|
||
def _load_system_installation(soname: str): | ||
"""Try to dlopen() the library indicated by ``soname`` | ||
|
||
Raises ``OSError`` if library cannot be loaded. | ||
""" | ||
return ctypes.CDLL(soname, PREFERRED_LOAD_FLAG) | ||
|
||
|
||
def _load_wheel_installation(soname: str): | ||
"""Try to dlopen() the library indicated by ``soname`` | ||
|
||
Returns ``None`` if the library cannot be loaded. | ||
""" | ||
if os.path.isfile(lib := os.path.join(os.path.dirname(__file__), "lib64", soname)): | ||
return ctypes.CDLL(lib, PREFERRED_LOAD_FLAG) | ||
return None | ||
|
||
|
||
def load_library(): | ||
"""Dynamically load libucxx.so and its dependencies""" | ||
# If libucx was installed as a wheel, we must request it to load the library | ||
# symbols. Otherwise, we assume that the library was installed in a system path | ||
# that ld can find. | ||
|
@@ -28,27 +55,29 @@ def load_library(): | |
libucx.load_library() | ||
del libucx | ||
|
||
# Dynamically load libucxx.so. Prefer a system library if one is present to | ||
# avoid clobbering symbols that other packages might expect, but if no | ||
# other library is present use the one in the wheel. | ||
prefer_system_installation = ( | ||
os.getenv("RAPIDS_LIBUCXX_PREFER_SYSTEM_LIBRARY", "false").lower() != "false" | ||
) | ||
|
||
soname = "libucxx.so" | ||
libucxx_lib = None | ||
try: | ||
libucxx_lib = ctypes.CDLL("libucxx.so", ctypes.RTLD_GLOBAL) | ||
except OSError: | ||
# If neither of these directories contain the library, we assume we are in an | ||
# environment where the C++ library is already installed somewhere else and the | ||
# CMake build of the libucxx Python package was a no-op. Note that this approach | ||
# won't work for real editable installs of the libucxx package, but that's not a | ||
# use case I think we need to support. scikit-build-core has limited support for | ||
# importlib.resources so there isn't a clean way to support that case yet. | ||
for lib_dir in ("lib", "lib64"): | ||
if os.path.isfile( | ||
lib := os.path.join(os.path.dirname(__file__), lib_dir, "libucxx.so") | ||
): | ||
libucxx_lib = ctypes.CDLL(lib, ctypes.RTLD_GLOBAL) | ||
break | ||
if prefer_system_installation: | ||
# Prefer a system library if one is present to | ||
# avoid clobbering symbols that other packages might expect, but if no | ||
# other library is present use the one in the wheel. | ||
try: | ||
libucxx_lib = _load_system_installation(soname) | ||
except OSError: | ||
libucxx_lib = _load_wheel_installation(soname) | ||
else: | ||
# Prefer the libraries bundled in this package. If they aren't found | ||
# (which might be the case in builds where the library was prebuilt | ||
# before packaging the wheel), look for a system installation. | ||
libucxx_lib = _load_wheel_installation(soname) | ||
if libucxx_lib is None: | ||
libucxx_lib = _load_system_installation(soname) | ||
|
||
# The caller almost never needs to do anything with this library, but no | ||
# harm in offering the option since this object at least provides a handle | ||
# to inspect where libucxx was loaded from. | ||
# to inspect where the libucxx was loaded from. | ||
jameslamb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return libucxx_lib | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this logic breaks the case of editable installs for developers in (for example) the devcontainer setup. In that scenario, we don't have a wheel-provided DSO, and we don't have a "system" provided one. Instead, the cython files in the ucxx package have the rpath to the locally built DSO baked in, so ld.so happily finds it. However, here, since we're just asking ld.so to resolve a DSO name, we fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, just pushed a commit with the changes we discussed offline and seen in rapidsai/kvikio#553. That should address that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to prefer wheel-provided
libucxx.so
? I thought it was not in the past due to uses with HPC-X containers, are those containers going to setRAPIDS_LIBUCXX_PREFER_SYSTEM_LIBRARY=true
by default or is there some other strategy they will follow?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm gonna need some context on what "uses with HPC-X containers" is, I'm not familiar with that. Can you describe (here or offline) what that is and how
libucxx.so
is getting installed and expected to be found there?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per discussion offline, this won't affect containers using HPC-X as they do not contain UCXX, so it should be good as is.