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

Publish nightly wheels to NVIDIA index instead of PyPI #1294

Merged

Conversation

pentschev
Copy link
Member

Nightly wheels also require rapids-dask-dependency which is only available in NVIDIA's PyPI index and cannot be published to PyPI as it installs Dask/Distributed from GitHub, which is forbidden by PyPI. Therefore, we're switching to publishing nightlies only to NVIDIA index as it doesn't seem external projects currently rely on nightlies. Release packages will continue to be published to PyPI.

@pentschev pentschev requested a review from a team as a code owner December 5, 2023 11:13
@github-actions github-actions bot added the ci label Dec 5, 2023
@pentschev pentschev added bug Something isn't working 3 - Ready for Review Ready for review by team labels Dec 5, 2023
@pentschev pentschev force-pushed the update-nightly-wheels-publishing branch from d4cb930 to 72072c9 Compare December 5, 2023 11:15
Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Looks like we also need to switch to build_wheel.sh in the PR workflow?

run: ci/build_python_pypi.sh

@pentschev pentschev added the breaking Breaking change label Dec 5, 2023
@pentschev
Copy link
Member Author

Good catch @charlesbluca , updated.

@pentschev
Copy link
Member Author

cc @vyasr @raydouglass

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Looks like the wheel build is still failing - my guess is that this is because the provided nightly version of 24.02.00a22 isn't PEP440 compliant? Perhaps we need to do something like #1279 here?

@pentschev
Copy link
Member Author

Looks like the wheel build is still failing - my guess is that this is because the provided nightly version of 24.02.00a22 isn't PEP440 compliant? Perhaps we need to do something like #1279 here?

You're right, but honestly I don't know what's the "right way" to fix this. The version being generated 24.02.00a22 seems in accordance to the version defined by rapids-dask-dependency where I took these changes from. Maybe @vyasr can point us to how it should be handled here, I believe we may also need to be compliant with release versions (which won't include a0 suffix).

@charlesbluca
Copy link
Member

charlesbluca commented Dec 5, 2023

On second thought, looks like maybe what we want to do here is bump the version in dask_cuda/VERSION rather than in the pyproject file, similar to what's happening here:

https://github.com/rapidsai/cudf/blob/8f7cbe69d4c2f670b97decc63e73b08e0eef7329/ci/build_wheel.sh#L25-L26

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks @pentschev!

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Almost there.

ci/build_python_pypi.sh Show resolved Hide resolved
ci/build_wheel.sh Outdated Show resolved Hide resolved
@charlesbluca
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 5b739af into rapidsai:branch-24.02 Dec 6, 2023
24 checks passed
Comment on lines +69 to +70
# Package is pure Python and only ever requires one build.
matrix_filter: map(select(.ARCH == "amd64" and .PY_VER == "3.10" and .CUDA_VER == "12.0.1"))
Copy link
Member

@ajschmidt8 ajschmidt8 Dec 6, 2023

Choose a reason for hiding this comment

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

@pentschev, you can generalize this with the following:

matrix_filter: map(select(.ARCH=="amd64")) | sort_by(.CUDA_VER, (.PY_VER | split(".") | map(tonumber))) | [.[-1]]

That will filter the matrix to only include amd64 machines, sort those machines by CUDA_VER and PY_VER, and then select the last element (which will have the latest CUDA and Python version).

You can optionally take this a step further to switch from a centos7/rockylinux image to an Ubuntu image with this minor amendment:

matrix_filter: map(select(.ARCH=="amd64")) | sort_by(.CUDA_VER, (.PY_VER | split(".") | map(tonumber))) | [.[-1] + {LINUX_VER: "ubuntu22.04"}]

Though this will require the Ubuntu version to be updated periodically as RAPIDS rotates CI image versions.

Copy link
Member

@ajschmidt8 ajschmidt8 Dec 6, 2023

Choose a reason for hiding this comment

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

Also, if you don't care about the CUDA_VER, then this can be simplified to:

matrix_filter: map(select(.ARCH=="amd64")) | sort_by((.PY_VER | split(".") | map(tonumber))) | [.[-1]]

and

matrix_filter: map(select(.ARCH=="amd64")) | sort_by((.PY_VER | split(".") | map(tonumber))) | [.[-1] + {LINUX_VER: "ubuntu22.04"}]

Here's one final variation that can be used too, which sorts the entries by PY_VER, gets the last entry, and then overrides the ARCH and LINUX_VER values:

matrix_filter: sort_by((.PY_VER | split(".") | map(tonumber))) | [.[-1] + {ARCH: "amd64", LINUX_VER: "ubuntu22.04"}]

Copy link
Member

Choose a reason for hiding this comment

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

As Dask-CUDA is a pure Python package (agnostic to OS, Python version, CUDA version, etc.), think this is just selecting one build arbitrarily

Is the idea of these suggestions just to avoid hard-coding version information in the selector?

Copy link
Member

Choose a reason for hiding this comment

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

yes. the hardcoded Python and CUDA versions will eventually be invalid as we update to newer versions. these generalized snippets prevent us from having to upgrade these versions in the future

Copy link
Member

Choose a reason for hiding this comment

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

younseojava pushed a commit to ROCm/dask-cuda-rocm that referenced this pull request Apr 16, 2024
Nightly wheels also require `rapids-dask-dependency` which is only available in NVIDIA's PyPI index and cannot be published to PyPI as it installs Dask/Distributed from GitHub, which is forbidden by PyPI. Therefore, we're switching to publishing nightlies only to NVIDIA index as it doesn't seem external projects currently rely on nightlies. Release packages will continue to be published to PyPI.

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

Approvers:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)
  - Ray Douglass (https://github.com/raydouglass)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#1294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team breaking Breaking change bug Something isn't working ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants