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 extraneous host pinnings from libraft-headers-only. #2102

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jan 19, 2024

We have run pinnings for librmm, spdlog, and fmt, so we don't need to list them in host as well. Currently, we are allowing fmt>=10.1.1,<11 in host, which is pulling a newer version 10.2.1 and adding a run-export that is fmt >=10.2.1,<11.0a0, which disallows fmt 10.1.1 contrary to our intentions.

@bdice bdice requested a review from a team as a code owner January 19, 2024 19:22
@raydouglass raydouglass added bug Something isn't working non-breaking Non-breaking change labels Jan 19, 2024
@bdice bdice mentioned this pull request Jan 19, 2024
@bdice
Copy link
Contributor Author

bdice commented Jan 19, 2024

I confirmed that the host environment no longer includes fmt. Current build log, previous recent build log.

I'm still waiting for CI to complete to know about the run-exports being fixed for sure, but this should be a sufficient fix from what I can observe so far.

@cjnolet
Copy link
Member

cjnolet commented Jan 19, 2024

/merge

@bdice
Copy link
Contributor Author

bdice commented Jan 19, 2024

Double-confirmation: I downloaded the artifacts and I see what I expected: libraft-headers-only depends on fmt >=10.1.1,<11. This is good to merge.

@rapids-bot rapids-bot bot merged commit d7eebc6 into rapidsai:branch-24.02 Jan 19, 2024
61 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

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
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants