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.

Sign up for GitHub

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

Jump to bottom

Fix loading of C++ libraries in wheels #118

Open
5 of 9 tasks
vyasr opened this issue Nov 8, 2024 · 3 comments
Open
5 of 9 tasks

Fix loading of C++ libraries in wheels #118

vyasr opened this issue Nov 8, 2024 · 3 comments
Assignees
@vyasr @jameslamb

Comments

@vyasr
Copy link
Contributor

vyasr commented Nov 8, 2024 •
edited by jameslamb
Loading

Description

Currently the wheels in which we are shipping native libraries have a conditional load logic that first searches for a library on the system and loads that before loading one in the wheel. As I've discussed in other channels, there is no way to support this kind of switching safely in general. If two different copies of a library exist and different packages make different assumptions when loading, the first one loaded wins and will be used in all cases. I'm working on more general solutions to this problem, but in the meantime we at minimum need to reconsider our default behavior, which is causing users confusion now. If we assume that for rapids native libraries the only consumers of those wheels are other rapids wheels that we control and are therefore consistently behaved, we can make some more sane default choices for the user. In general, we should assume that our packages are being installed by end users unfamiliar with packaging. The default behavior should support this, meaning that by default we probably want the cudf pip wheel to use libcudf from a wheel even if a system libcudf library exists by default. Conversely, more advanced users may install cudf into an environment (like a container) where the library already exists and should be reused. We should enable that use case using an environment variable. A global configuration is another option, but I'd like to reserve that for when I have a more general solution available.

We should also switch all our library loads to use RTLD_LOCAL instead of RTLD_GLOBAL to be safe. Global loads can easily lead to symbol clashes.

Benefits of this work

  • adds flexibility for different preferred methods of library-loading
  • makes library-loading safer (i.e. less likely to cause conflicts)

Acceptance Criteria

For all relevant RAPIDS libraries:

  • preference for system vs. wheel shared library is configurable via an environment
  • shared library loading defaults to preferring the library from the wheel itself
  • all library loads are done with RTLD_LOCAL

Approach

N/A

Notes

N/A

Task Lists

Updates
Beta Give feedback
@jameslamb
Copy link
Member

Linking some related conversations:

This was referenced Nov 13, 2024
Merged
Merged
Merged
Open
Merged
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this issue Nov 14, 2024
@jameslamb
prefer wheel-provided libcuspatial.so in load_library(), use RTLD_LOC…
20abaaa 
…AL (#1483)

Contributes to rapidsai/build-planning#118

Modifies `libcuspatial.load_library()` in the following ways:

* prefer wheel-provided `libcuspatial.so` to system installation
* expose environment variable `RAPIDS_LIBCUSPATIAL_PREFER_SYSTEM_LIBRARY` for switching that preference
* load `libcuspatial.so` with `RTLD_LOCAL`, to prevent adding symbols to the global namespace ([dlopen docs](https://linux.die.net/man/3/dlopen))

## Notes for Reviewers

### How I tested this

See "How I tested this" in rapidsai/cudf#17316

Also opened this PR originally pulling in built packages from rapidsai/cudf#17316

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: #1483
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Nov 14, 2024
@jameslamb
prefer wheel-provided libcudf.so in load_library(), use RTLD_LOCAL (#…
66c5a2d 
…17316)

Contributes to rapidsai/build-planning#118

Modifies `libcudf.load_library()` in the following ways:

* prefer wheel-provided `libcudf.so` to system installation
* expose environment variable `RAPIDS_LIBCUDF_PREFER_SYSTEM_LIBRARY` for switching that preference
* load `libcudf.so` with `RTLD_LOCAL`, to prevent adding symbols to the global namespace ([dlopen docs](https://linux.die.net/man/3/dlopen))

## Notes for Reviewers

### How I tested this

Locally (x86_64, CUDA 12, Python 3.12), built `libcudf`, `pylibcudf`, and `cudf` wheels from this branch, then `libcuspatial` and `cuspatial` from the corresponding cuspatial branch. Ran `cuspatial`'s unit tests, and tried setting the environment variable and inspecting `ld`'s logs to confirm that the environment variable changed the loading and search behavior.

e.g.

```shell
# clear ld cache to avoid cheating
rm -f /etc/ld.so.cache
ldconfig

# try using an env variable to say "prefer the system-installed version"
LD_DEBUG=libs \
LD_DEBUG_OUTPUT=/tmp/out.txt \
RAPIDS_LIBCUDF_PREFER_SYSTEM_LIBRARY=true \
python -c "import cuspatial; print(cuspatial.__version__)"

cat /tmp/out.txt.* > prefer-system.txt
# (then manually looked through those logs to confirm it searched places like /usr/lib64 and /lib64)
```

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: #17316
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this issue Nov 14, 2024
@jameslamb
prefer wheel-provided libkvikio.so, use RTLD_LOCAL (#551)
f1ad8b9 
Contributes to rapidsai/build-planning#118

Modifies `libkvikio.load_library()` in the following ways:

* prefer wheel-provided `libkvikio.so` to system installation
* expose environment variable `RAPIDS_LIBKVIKIO_PREFER_SYSTEM_LIBRARY` for switching that preference
* load `libkvikio.so` with `RTLD_LOCAL`, to prevent adding symbols to the global namespace ([dlopen docs](https://linux.die.net/man/3/dlopen))

## Notes for Reviewers

### How I tested this

Tested the general approach on rapidsai/cudf#17316 and rapidsai/cuspatial#1483.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #551
@jameslamb
Copy link
Member

update descriptions on still-open "Dynamic RAPIDS Deps" issues on wheel-planning board (https://github.com/orgs/rapidsai/projects/136)

done (see the links to the private issues in the event feed above)

@jameslamb jameslamb mentioned this issue Nov 15, 2024
Merged
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this issue Nov 15, 2024
@jameslamb
fix library-loading issues in editable installs (#553)
75a69cc 
Contributes to rapidsai/build-planning#118

The pattern introduced in #551 breaks editable installs in devcontainers. In that type of build, `libkvikio.so` is built outside of the wheel but **not installed**, so it can't be found by `ld`. Extension modules in `kvikio` are able to find it via RPATHs instead.

This proposes:

* try-catching the entire library-loading attempt, to silently do nothing in cases like that
* adding an import of the `kvikio` Python library in the `devcontainers` CI job, as a smoke test to catch issues like this in the future

## Notes for Reviewers

### How I tested this

Reproduced that with the CUDA 12.5 pip devcontainers today:

```shell
build-all
python -c "import kvikio"
# OSError: libkvikio.so: cannot open shared object file: No such file or directory
```

Confirmed that the changes in this PR fix that.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Lawrence Mitchell (https://github.com/wence-)

URL: #553
This was referenced Nov 15, 2024
Merged
Merged
@jameslamb
Copy link
Member

update https://github.com/rapidsai/build-planning/tree/main/docs with note about this

I'd added this to the task list for this issue but... let's skip it. The loading logic for C++ wheels is already not described in those docs, and it's still very much actively changing and being redesigned.

This issue and others linked to/from it are enough to document the current state, think it's ok to wait until that stabilizes a bit before updating those docs.

@jameslamb jameslamb mentioned this issue Nov 15, 2024
Open
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this issue Nov 15, 2024
@jameslamb
fix library-loading issues in editable installs (#1484)
0a79456 
Contributes to rapidsai/build-planning#118

The pattern introduced in #1483 breaks editable installs in devcontainers. In that type of build, `libcuspatial.so` is built outside of the wheel but **not installed**, so it can't be found by `ld`. Extension modules in `cuspatial` are able to find it via RPATHs instead.

This proposes:

* try-catching the entire library-loading attempt, to silently do nothing in cases like that
* ~adding an import of the `cuspatial` Python library in the `devcontainers` CI job, as a smoke test to catch issues like this in the future~ *(edit: removed those, [`devcontainer` builds run on CPU nodes](https://github.com/rapidsai/shared-workflows/blob/4e84062f333ce5649bc65029d3979569e2d0a045/.github/workflows/build-in-devcontainer.yaml#L19))*

## Notes for Reviewers

### How I tested this

Tested this approach on rapidsai/kvikio#553

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #1484
rapids-bot bot pushed a commit to rapidsai/ucxx that referenced this issue Nov 15, 2024
@jameslamb
prefer wheel-provided libucxx.so in load_library, use RTLD_LOCAL (#322)
042261d 
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](https://linux.die.net/man/3/dlopen))

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Peter Andreas Entschev (https://github.com/pentschev)

URL: #322
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Nov 19, 2024
@jameslamb
fix library-loading issues in editable installs (#17338)
9c5cd81 
Contributes to rapidsai/build-planning#118

The pattern introduced in #17316 breaks editable installs in devcontainers. In that type of build, `libcudf.so` is built outside of the wheel but **not installed**, so it can't be found by `ld`. Extension modules in `cudf` and `pylibcudf` are able to find it via RPATHs instead.

This proposes:

* try-catching the entire library-loading attempt, to silently do nothing in cases like that
* ~adding imports of the `cudf` and `pylibcudf` libraries in the `devcontainers` CI job, as a smoke test to catch issues like this in the future~ *(edit: removed those, [`devcontainer` builds run on CPU nodes](https://github.com/rapidsai/shared-workflows/blob/4e84062f333ce5649bc65029d3979569e2d0a045/.github/workflows/build-in-devcontainer.yaml#L19))*

## Notes for Reviewers

### How I tested this

Tested this approach on rapidsai/kvikio#553

#

Authors:
  - James Lamb (https://github.com/jameslamb)
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Matthew Murray (https://github.com/Matt711)

URL: #17338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vyasr vyasr

@jameslamb jameslamb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@vyasr @jameslamb