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

ensure raft-dask wheel tests install pylibraft wheel from the same CI run, fix wheel dependencies #2349

Merged

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented May 31, 2024

Description

Fixes #2348

#2331 introduced rapids-build-backend (https://github.com/rapidsai/rapids-build-backend) as the build backend for pylibraft, raft-dask, and raft-ann-bench.

That library handles automatically modifying a wheel's dependencies based on the target CUDA version. Unfortunately, we missed a few cases in #2331, and as a result the last few days of nightly raft-dask wheels had the following issues:

  • depending on pylibraft
    • (does not exist, it's called pylibraft-cu12)
  • depending on ucx-py==0.39.*
    • (does not exist, it's called ucx-py-cu12)
  • depending on distributed-ucxx-cu11==0.39.* instead of distributed-ucxx-cu11==0.39.*,>=0.0.0a0
    • (without that alpha specifier, pip install --pre is required to install pre-release versions)

This wasn't caught in raft's CI, but in downstream CI like cuml and cugraph, with errors like this:

ERROR: ResolutionImpossible:
  
The conflict is caused by:
    raft-dask-cu12 24.8.0a20 depends on pylibraft==24.8.* and >=0.0.0a0
    raft-dask-cu12 24.8.0a19 depends on pylibraft==24.8.* and >=0.0.0a0

(example cugraph build)

This PR:

  • fixes those dependency issues
  • modifies raft's CI so that similar issues would be caught here in the future, before publishing wheels

Notes for Reviewers

What was the root cause of CI missing this, and how does this PR fix it?

The raft-dask test CI jobs use this pattern to install the raft-dask wheel built earlier in the CI pipeline.

pip install "raft_dask-cu12[test]>=0.0.0a0" --find-links dist/

As described in the pip docs (link), --find-links just adds a directory to the list of other places pip searches for packages. Because the wheel there had unsatisfiable constraints (e.g. pylibraft==24.8.* does not exist anywhere), pip install silently disregarded that locally-downloaded raft_dask wheel and backtracked (i.e. downloaded older and older wheels from https://pypi.anaconda.org/rapidsai-wheels-nightly/simple/) until it found one that wasn't problematic.

This PR ensures that won't happen by telling pip to install exactly that locally-downloaded file, like this

pip install "$(echo ./dist/raft_dask_cu12*.whl)[test]"

If that file is uninstallable, pip install fails and you find out via a CI failure.

How I tested this

Initially pushed a commit with just the changes to the test script. Saw the wheel-tests-raft-dask CI jobs fail in the expected way, instead of silently falling back to an older wheel and passing 🎉 .

ERROR: Could not find a version that satisfies the requirement ucx-py-cu12==0.39.* (from raft-dask-cu12[test]) (from versions: 0.32.0, 0.33.0, 0.34.0, 0.35.0, 0.36.0, 0.37.0, 0.38.0a4, 0.38.0a5, 0.38.0a6, 0.39.0a0)
ERROR: No matching distribution found for ucx-py-cu12==0.39.*

(build link)

@jameslamb jameslamb added the 2 - In Progress Currenty a work in progress label May 31, 2024
@github-actions github-actions bot added the ci label May 31, 2024
@jameslamb jameslamb added bug Something isn't working non-breaking Non-breaking change labels May 31, 2024
@@ -11,7 +11,8 @@ RAPIDS_PY_WHEEL_NAME="raft_dask_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels
RAPIDS_PY_WHEEL_NAME="pylibraft_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 ./local-pylibraft-dep
python -m pip install --no-deps ./local-pylibraft-dep/pylibraft*.whl

python -m pip install "raft_dask-${RAPIDS_PY_CUDA_SUFFIX}[test]>=0.0.0a0" --find-links dist/
# echo to expand wildcard before adding `[extra]` requires for pip
python -m pip install -v "$(echo ./dist/raft_dask_${RAPIDS_PY_CUDA_SUFFIX}*.whl)[test]"
Copy link
Member Author

Choose a reason for hiding this comment

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

copied this commend and pattern from

# echo to expand wildcard before adding `[extra]` requires for pip
python -m pip install $(echo ./dist/pylibraft*.whl)[test]

@jameslamb jameslamb changed the title WIP: ensure raft-dask wheel tests install pylibraft wheel from the same CI run ensure raft-dask wheel tests install pylibraft wheel from the same CI run May 31, 2024
@jameslamb jameslamb added 3 - Ready for Review and removed 2 - In Progress Currenty a work in progress labels May 31, 2024
@jameslamb jameslamb marked this pull request as ready for review May 31, 2024 21:17
@jameslamb jameslamb requested review from a team as code owners May 31, 2024 21:17
@jameslamb jameslamb requested a review from bdice May 31, 2024 21:17
@jameslamb jameslamb changed the title ensure raft-dask wheel tests install pylibraft wheel from the same CI run ensure raft-dask wheel tests install pylibraft wheel from the same CI run, fix wheel dependencies May 31, 2024
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.

Approved with one small request.

dependencies.yaml Outdated Show resolved Hide resolved
Co-authored-by: Bradley Dice <[email protected]>
@dantegd
Copy link
Member

dantegd commented Jun 3, 2024

@jameslamb @bdice This PR should unblock CI for downstream repos, and ready to merge, right?

@bdice
Copy link
Contributor

bdice commented Jun 3, 2024

@dantegd @jameslamb Yes, I think so. I’ll go ahead and merge so we can unblock downstream work.

@bdice
Copy link
Contributor

bdice commented Jun 3, 2024

/merge

@rapids-bot rapids-bot bot merged commit be812e4 into rapidsai:branch-24.08 Jun 3, 2024
69 checks passed
@jameslamb
Copy link
Member Author

Yes it should, thanks for merging it @bdice

@jameslamb jameslamb deleted the fix/raft-dask-wheel-test-install branch June 3, 2024 13:22
@nv-rliu
Copy link

nv-rliu commented Jun 3, 2024

Thanks everyone! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review bug Something isn't working ci non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] wheel tests do not fail when raft-dask wheel has unsatisfiable dependency requirements
4 participants