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

Add libucxx wheel #260

Merged
merged 18 commits into from
Aug 8, 2024
Merged

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Aug 7, 2024

Copy link
Contributor

@msarahan msarahan left a comment

Choose a reason for hiding this comment

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

These edits might get you on the right track. The argument that is cpp or python is backwards compatible, but if not present, it behaves as just python, and the filenames don't line up.

Edit: for reference, here's where the bash script in action here lives: https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-upload-wheels-to-s3#L11, which eventually leads to the real difference in file naming: https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-package-name#L32

ci/build_wheel.sh Outdated Show resolved Hide resolved
ci/build_wheel.sh Outdated Show resolved Hide resolved
@KyleFromNVIDIA KyleFromNVIDIA marked this pull request as ready for review August 7, 2024 22:18
@KyleFromNVIDIA KyleFromNVIDIA requested review from a team as code owners August 7, 2024 22:18
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, thanks @KyleFromNVIDIA .

dependencies.yaml 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.

A few comments -- overall this looks good!

ci/build_wheel.sh Show resolved Hide resolved
ci/test_wheel_distributed_ucxx.sh Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
dependencies.yaml Show resolved Hide resolved
python/ucxx/pyproject.toml Outdated Show resolved Hide resolved
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner August 8, 2024 18:23
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.

Can you comment on the impact on wheel sizes? LGTM.

@KyleFromNVIDIA
Copy link
Contributor Author

Before this change:

ucxx_cu12-0.40.0a14-cp311-cp311-manylinux_2_28_x86_64.whl - 5.2MB

After this change:

libucxx_cu12-0.40.0a32-py3-none-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl - 512KB
ucxx_cu12-0.40.0a32-cp311-cp311-manylinux_2_27_x86_64.manylinux_2_28_x86_64.whl - 718KB

As you can see, there is a 76% reduction in total wheel size.

@KyleFromNVIDIA
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit cbe4c60 into rapidsai:branch-0.40 Aug 8, 2024
67 checks passed
KyleFromNVIDIA added a commit to KyleFromNVIDIA/ucxx that referenced this pull request Aug 16, 2024
*.pxd files were mistakenly removed in
rapidsai#260. Resume installing them
in the ucxx wheel.
rapids-bot bot pushed a commit that referenced this pull request Aug 16, 2024
`*.pxd` files were mistakenly removed in #260. Resume installing them in the ucxx wheel.

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

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

Successfully merging this pull request may close these issues.

4 participants