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

use rapids-build-backend #1044

Merged
merged 30 commits into from
Jun 12, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Jun 5, 2024

Contributes to rapidsai/build-planning#31
Contributes to rapidsai/dependency-file-generator#89
Contributes to rapidsai/build-planning#72

Proposes:

  • introducing rapids-build-backend as this project's build backend, to reduce the complexity of various CI/build scripts
  • making get-data-from-pyproject.toml code in conda recipe a bit stricter

Notes for Reviewers

This reverts some of the workarounds introduced for docs builds in #1041. See this thread for context: #1041 (comment)

To do that, I set 2 environment variables manually in in the readthedocs console (link):

  • RAPIDS_DISABLE_CUDA=true = used to prevent needing nvcc in the docs-building environment
  • RAPIDS_MATRIX_ENTRY='cuda=12.2' = used to ensure that pip install . tries to install a real package (e.g. libucx-cu12)

I think this is the first place we're using the rapids-build-backend environment variable support.

see #1044 (comment)

@jameslamb jameslamb changed the title WIP: use rapids-build-backend use rapids-build-backend Jun 12, 2024
@@ -136,8 +150,6 @@ dependencies:
- matrix: {cuda: "11.*"}
packages:
- libucx-cu11==1.15.0
# NOTE: this fallback needs to be a real, suffixed version
# so 'pip install .' (e.g. as used in docs builds) will work
Copy link
Member Author

Choose a reason for hiding this comment

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

I accidentally left behind this comment in #1041 ... it's not accurate on branch-0.39 or on this branch.

# # install CUDA-12-specific dependencies
# # (e.g. libucx-cu12)
# RAPIDS_MATRIX_ENTRY="cuda=12.2"
#
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 to @pentschev for giving me access to set environment variables in the readthedocs console for this project 😁

@jameslamb jameslamb marked this pull request as ready for review June 12, 2024 15:13
@jameslamb jameslamb requested review from a team as code owners June 12, 2024 15:13
@jameslamb jameslamb requested a review from bdice June 12, 2024 15:13
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A few suggestions, but overall this is pretty close.

# # install CUDA-12-specific dependencies
# # (e.g. libucx-cu12)
# RAPIDS_MATRIX_ENTRY="cuda=12.2"
#
- ../../
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we use a conda env with a pip dependency on ../... I think it would be clearer to install a conda env with the relevant dependencies and then invoke pip install with the appropriate flags on the command line instead of requiring environment variables in the UI. Is that an option?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not an option. readthedocs doesn't allow you to customize the flags passed to pip install.

You can see the history that led to this pattern here: #1041 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I saw there were options to run arbitrary commands like installing with poetry or uv, so I figured we could do the same with pip and custom flags. https://docs.readthedocs.io/en/stable/build-customization.html#install-dependencies-with-poetry

Copy link
Member Author

@jameslamb jameslamb Jun 12, 2024

Choose a reason for hiding this comment

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

!!! in all of that conversation in #1041, I was so focused on "how do we change the install: step", I never considered the possibility "just ignore it completely and install the library through one of the other steps where you can run custom commands". The tunnel vision was real 😭

Thanks for that link, let me try this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This worked! I can't believe I missed that going through options in #1041, thank you so much 😅

I removed the environment variables in the readthedocs console, then pushed the changes you now see in .readthedocs.yaml on this PR.

I think this is unambiguously better than having the pip: section in a conda environment YAML file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aw Yeah

conda/recipes/ucx-py/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/ucx-py/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/ucx-py/meta.yaml Outdated Show resolved Hide resolved
@@ -47,12 +50,12 @@ test:
- ucp

about:
home: {{ data.get("project", {}).get("urls", {}).get("Homepage", "") }}
license: {{ data.get("project", {}).get("license", {}).get("text", "") }}
home: {{ data["project"]["urls"]["Homepage"] }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the .get(...) was required because conda-build doesn't actually inject the Jinja variables on every pass and that led to problems in metadata. I'm not sure. We use the .get(...) logic in dask-cuda, too. https://github.com/rapidsai/dask-cuda/blob/branch-24.08/conda/recipes/dask-cuda/meta.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks to me like this is working as expected.

wget \
  https://downloads.rapids.ai/ci/ucx-py/pull-request/1046/43beaa1/ucx-py_conda_python_cuda12_py39_x86_64.tar.gz

tar -xvzf ./ucx-py_conda_python_cuda12_py39_x86_64.tar.gz
cat ./linux-64/repodata.json | jq .

For example, I see the right license: string there.

And looking in one of the packages, all this metadata looks correct too:

mkdir ./delete-me
cp ./linux-64/ucx-py-0.39.00a1-py39_240612_g43beaa1_1.tar.bz2 ./delete-me
cd ./delete-me
cph extract ./ucx-py-0.39.00a1-py39_240612_g43beaa1_1.tar.bz2
cat ucx-py-0.39.00a1-py39_240612_g43beaa1_1/info/about.json | jq .

Unless you can remember the specific reason to prefer .get(), I think we should move away from it in all conda recipes, to turn "silently ignored missing metadata" cases into "got a loud error at build time" cases. I actually just wrote that up in rapidsai/build-planning#72.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool! Go for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jakirkham may remember the specific reason why we used .get(...) in these recipes, I recall discussing it with him at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a related thread: rapidsai/pynvjitlink#33 (comment)

I don't think it changes the fact that this is working and should be used for the specific places I've made this change in this PR, and doesn't need to block this PR. But might mean "never use .get()" is too strong.

I'll post over in the build-planning issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe conda-build has changed. 🤷‍♂️ In any case, I am happy to drop .get() if this is working.

tests/test_version.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

All seems fine once you're comfortable with the state of the conversations from my last review.

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, I agree with @bdice 's suggestion on version test, other than that we're good. Thanks @jameslamb !

tests/test_version.py Outdated Show resolved Hide resolved
@jameslamb jameslamb mentioned this pull request Jun 12, 2024
@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 42c03ef into rapidsai:branch-0.39 Jun 12, 2024
39 checks passed
@jameslamb jameslamb deleted the rapids-build-backend branch June 12, 2024 18:33
rapids-bot bot pushed a commit that referenced this pull request Jul 3, 2024
Follow-up to #1044
Contributes to rapidsai/build-planning#31
Related to #1047

* updates `libucx` build requirements so they can be satisfied by only packages from pypi.org
* removes `--extra-index-url https://pypi.anaconda.org/rapidsai-wheels-nightly/simple/` in `pip install` calls, now that `rapids-build-backend` is on pypi.org (https://pypi.org/project/rapids-build-backend/)
* sets `rapids-build-backend` config `disable-cuda=true` in `pyproject.toml`
  - *this means that now `pip install .` will not require `nvcc` or produce a wheel with a suffix like `-cu12`*
  - *modified CI script used to build wheels to override this, so the published wheels will still have `-cu${ver}` suffixes
* updates docs on source installation to reflect these changes

## Notes for Reviewers

These changes came out of an offline conversation with @pentschev and @vyasr

Authors:
  - James Lamb (https://github.com/jameslamb)
  - Peter Andreas Entschev (https://github.com/pentschev)

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

URL: #1048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants