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

Remove duplicate librmm runtime dependency #1264

Merged
merged 3 commits into from
Feb 10, 2023

Conversation

ajschmidt8
Copy link
Member

libraft-headers has librmm listed as a host and run dependency in its recipe.

Since librmm has a run_exports value, this causes there to be two librmm runtime dependency entries in the resulting libraft-headers conda package, each with slightly version specifiers.

To prevent this from occurring, I've added librmm to the ignore_run_exports_from key in the libraft-headers recipe.

Note that another way to solve this would be to remove librmm from the run dependencies, but this is inconsistent with the rest of the RAPIDS recipes.

`libraft-headers` has `librmm` listed as a `host` and `run` dependency in its recipe.

Since `librmm` has a `run_exports` value, this causes there to be two `librmm` runtime dependency entries in the resulting `libraft-headers` conda package, each with slightly version specifiers.

To prevent this from occurring, I've added `librmm` to the `ignore_run_exports_from` key in the `libraft-headers` recipe.

Note that another way to solve this would be to remove `librmm` from the `run` dependencies, but this is inconsistent with the rest of the RAPIDS recipes.
@ajschmidt8 ajschmidt8 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 9, 2023
@ajschmidt8 ajschmidt8 requested a review from a team as a code owner February 9, 2023 19:02
@ajschmidt8
Copy link
Member Author

This picture of the libraft-headers metadata on its conda page demonstrates the current issue.

image

ajschmidt8 added a commit to ajschmidt8/xgboost-conda that referenced this pull request Feb 10, 2023
Similarly to rapidsai/raft#1264, this PR removes a duplicate `run` dependency on `librmm`.

The current packages on Anaconda.org have a dependency on the `librmm` packages with the old date string format which have been removed from the `main` label.

Additionally it increments the build number so that new packages are published as a result of this change.
@vyasr
Copy link
Contributor

vyasr commented Feb 10, 2023

Note that another way to solve this would be to remove librmm from the run dependencies, but this is inconsistent with the rest of the RAPIDS recipes.

Isn't this approach the better one, though? It lets librmm be the single source of truth dictating that linking to it also requires its presence as a runtime dependency. Maybe we merge this PR as is for consistency as you say, then go back through and refactor?

@ajschmidt8
Copy link
Member Author

Note that another way to solve this would be to remove librmm from the run dependencies, but this is inconsistent with the rest of the RAPIDS recipes.

Isn't this approach the better one, though? It lets librmm be the single source of truth dictating that linking to it also requires its presence as a runtime dependency. Maybe we merge this PR as is for consistency as you say, then go back through and refactor?

This part is complicated by the fact that we use conda recipes with multiple outputs.

If you take the libcudf recipe for example... we build the all of the libcudf packages (libcudf, libcudf-tests, libcudf_kafka, etc) using the top-level dependencies here (which, therefore, is where librmm needs to be listed).

Because librmm is only listed in the host dependencies of this top-level libcudf package, the run_exports listed in its recipe do not apply to the rest of the libcudf outputs packages.

The individual outputs entires (like libcudf) only run cmake --install cpp/build and therefore don't list librmm as a host/build dependency... because they don't need it. They only need cmake as a dependency since they're just installing the library that was already built.

As a result of this arrangement, we manually add librmm as a run dependency wherever else it's needed.

@ajschmidt8
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit bc3a9c5 into rapidsai:branch-23.04 Feb 10, 2023
@ajschmidt8 ajschmidt8 deleted the fix-recipe branch February 10, 2023 15:53
rapids-bot bot pushed a commit that referenced this pull request May 2, 2024
Contributes to rapidsai/build-planning#54.

The `libraft-headers` and `libraft-headers-only` conda packages are bundling `rmm`'s headers. I believe that's because the `librmm` conda package isn't available in the `libraft*` conda build environment, and as a result it's getting `rmm` via CPM (thanks to `rapids-cmake`).

As a result, this project and any that depend on it are seeing warnings like the following in conda builds where `conda`'s `path_conflict` setting is set to `warn` or `prevent` (like #2245):

```text
This transaction has incompatible packages due to a shared path.
  packages: rapidsai-nightly/linux-64::librmm-24.04.00a38-cuda11_240326_ga98931b9_38, rapidsai-nightly/linux-64::libraft-headers-only-24.04.00a93-cuda11_240326_g9637b3c2_93
  path: 'include/rmm/mr/device/arena_memory_resource.hpp'
```

To fix that, this proposes the following changes:

* make `librmm` a `host` dependency of the following conda packages: `libraft-headers-only`, `libraft-headers`

### Benefits of this change

* slightly smaller `libraft-headers` and `libraft-headers-only` conda packages
* reduced risk of runtime and installation issues caused by file clobbering

## Notes for reviewers

### History of changes to the `librmm` dependency for `libraft-headers`:

* both `run` and `host`: #508
* both `run` and `host`, but ignoring its `run_exports`: #1264
* just `run`, but ignoring its `run_exports`: #2102

In particular, #2102 referred to the `host` dependency on `librmm` as "extraneous" but from a packaging perspective, I don't think it is. `librmm` being in `host` means it'll be present in the build environment, which means its headers will be *found* instead of *downloaded*, and therefore not packaging into the `libraft*` conda packages.

### How I tested this

Built all the `raft` conda packages locally from `branch-24.06` and confirmed that they contain `rmm` headers. Then again from this branch and confirmed they were gone.

```shell
docker run \
    --rm \
    --env-file "${PWD}/aws-creds.env" \
    -v $(pwd):/opt/work \
    -w /opt/work \
    -it rapidsai/ci-conda:cuda12.2.2-ubuntu22.04-py3.10-amd64 \
    bash

CI="true" \
  ci/build_cpp.sh

# On 'branch-24.06', this showed the rmm headers being packaged.
# On this branch, they're omitted.
tar --bzip2 -tf \
  /tmp/conda-bld-output/linux-64/libraft-headers-only-24.06.00a50-cuda12_240430_g1e0e2283_50.tar.bz2 \
| grep 'include/rmm' \
| wc -l
```

Also checked the CI logs from `conda-cpp-build` jobs here. On other recent PRs, I see CPM downloading `rmm` ...

```text
-- CPM: Adding package [email protected] (branch-24.06)
```

... and all the `rmm` headers getting installed as part of the `libraft-headers` package

```text
-- Installing: /opt/conda/conda-bld/_h_env_placehold_placehold_..._placeho/include/rmm/cuda_stream.hpp
```

([build link](https://github.com/rapidsai/raft/actions/runs/8904352932))

Here, I see `librmm` coming through via the conda package requirements ...

```text
The following NEW packages will be INSTALLED:
    ...
    librmm:                      24.06.00a17-cuda12_240430_g26fa9ecb_17 rapidsai-nightly
```

... and being used instead of downloads via CPM ...

```text
-- CPM: Using local package [email protected]
```

... and no installation of the `rmm` headers as part of building any `libraft` packages.

([build link](https://github.com/rapidsai/raft/actions/runs/8910675575/job/24470450187?pr=2284))

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

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

URL: #2284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants