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

Simplify wheel build scripts and allow alphas of RAPIDS dependencies #13963

Merged
merged 11 commits into from
Aug 31, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Aug 25, 2023

Description

This PR makes a handful of changes aimed at simplifying the CI pipeline for building wheels as a precursor to switching RAPIDS nightlies to using proper alpha versions:

  • Inlines apply_wheel_modifications.sh in build_wheel.sh. Now that the build doesn't rely excessively on logic in shared workflows, there's no real benefit to having a separate script (previously apply_wheel_modification.sh was a special script that the shared workflow knew to execute i.e. it was a hook into an externally controlled workflow).
  • Consolidates the textual replacements using for loops and makes the replacements more targeted by only modifying the Python package being built in a given script. For instance, python/dask_cudf/pyproject.toml is no longer overwritten when building cudf.
  • Modifies dependency specs for RAPIDS packages to include a >=0.0.0a0 component. This is the key change that will allow alpha dependencies to be discovered.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 25, 2023
@vyasr vyasr self-assigned this Aug 25, 2023
@vyasr vyasr requested a review from a team as a code owner August 25, 2023 16:00
@github-actions github-actions bot added the ci label Aug 25, 2023
@vyasr vyasr requested a review from bdice August 29, 2023 00:56
fi

if [[ $PACKAGE_CUDA_SUFFIX == "-cu12" ]]; then
sed -i "s/cuda-python[<=>\.,0-9a]*/cuda-python>=12.0,<13.0a0/g" ${pyproject_file}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if upper bound pinnings like this (<13.0a0) affect whether alphas are allowed, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, but you're right to find that strange. For example:

(main) dt08% pip download 'numpy>=1.25,<2.0.0a'
Collecting numpy<2.0.0a,>=1.25
  Obtaining dependency information for numpy<2.0.0a,>=1.25 from https://files.pythonhosted.org/packages/32/6a/65dbc57a89078af9ff8bfcd4c0761a50172d90192eaeb1b6f56e5fbf1c3d/numpy-1.25.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata
  Downloading numpy-1.25.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (5.6 kB)
Downloading numpy-1.25.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (18.2 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 18.2/18.2 MB 7.5 MB/s eta 0:00:00
Saved ./numpy-1.25.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
Successfully downloaded numpy
(main) dt08% pip download 'numpy>=1.25.0a0,<2.0.0a'
Collecting numpy<2.0.0a,>=1.25.0a0
  Obtaining dependency information for numpy<2.0.0a,>=1.25.0a0 from https://files.pythonhosted.org/packages/35/8b/b669836be53d7b6697bc290abcdda701fd924a22c702713bbdf2c6c5bef5/numpy-1.26.0b1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata
  Using cached numpy-1.26.0b1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (58 kB)
Using cached numpy-1.26.0b1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (19.2 MB)
Saved ./numpy-1.26.0b1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
Successfully downloaded numpy

So there's definitely a degree of depending on pip implementation details here. My guess is that nobody ever wanted the upper bound alpha to trigger this so they didn't notice this oversight. I'm not sure we can improve on my solution at present, but our testing should catch if we run into issues fairly quickly since we won't get the necessary dependencies, so I'm not too worried. I'm confident pip won't break the >=a0 behavior.

ci/build_wheel.sh Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from bdice August 31, 2023 17:34
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.

LGTM.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@vyasr
Copy link
Contributor Author

vyasr commented Aug 31, 2023

/merge

@rapids-bot rapids-bot bot merged commit eeb7613 into rapidsai:branch-23.10 Aug 31, 2023
@vyasr vyasr deleted the feat/simplify_wheel_builds branch October 5, 2023 23:56
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 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