Skip to content
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

Merged
merged 6 commits into from
Nov 15, 2024

Conversation

jameslamb
Copy link
Member

Description

Contributes to rapidsai/build-planning#118

Modifies libucxx.load_library() in the following ways:

  • prefer wheel-provided libucxx.so to system installation
  • expose environment variable RAPIDS_LIBUCXX_PREFER_SYSTEM_LIBRARY for switching that preference
  • load libucxx.so with RTLD_LOCAL, to prevent adding symbols to the global namespace (dlopen docs)

@jameslamb jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality DO NOT MERGE Hold off on merging; see PR for details labels Nov 14, 2024
@jameslamb jameslamb changed the title WIP: [DO NOT MERGE] prefer wheel-provided libucxx.so in load_library, use RTLD_LOCAL prefer wheel-provided libucxx.so in load_library, use RTLD_LOCAL Nov 14, 2024
@jameslamb jameslamb removed the DO NOT MERGE Hold off on merging; see PR for details label Nov 14, 2024
@jameslamb jameslamb requested a review from vyasr November 14, 2024 22:34
@jameslamb jameslamb marked this pull request as ready for review November 14, 2024 22:35
@jameslamb jameslamb requested a review from a team as a code owner November 14, 2024 22:35
Comment on lines +58 to +60
prefer_system_installation = (
os.getenv("RAPIDS_LIBUCXX_PREFER_SYSTEM_LIBRARY", "false").lower() != "false"
)
Copy link
Member

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 set RAPIDS_LIBUCXX_PREFER_SYSTEM_LIBRARY=true by default or is there some other strategy they will follow?

Copy link
Member Author

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?

Copy link
Member

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.

Comment on lines 64 to 83
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.
return libucxx_lib
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jameslamb .

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 042261d into rapidsai:branch-0.41 Nov 15, 2024
65 checks passed
@jameslamb jameslamb deleted the library-loading branch November 15, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants