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

Build wheels alongside conda CI #1182

Merged
merged 19 commits into from
Jan 6, 2023
Merged

Build wheels alongside conda CI #1182

merged 19 commits into from
Jan 6, 2023

Conversation

sevagh
Copy link
Contributor

@sevagh sevagh commented Dec 20, 2022

Description

This PR adds pip wheel CI to the Conda CI, instead of having them work separately.

Checklist

@sevagh sevagh requested a review from a team as a code owner December 20, 2022 15:47
@sevagh sevagh added non-breaking Non-breaking change ci Python Related to RMM Python API improvement Improvement / enhancement to an existing function and removed Python Related to RMM Python API labels Dec 20, 2022
@github-actions github-actions bot removed the ci label Dec 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (d76979d) compared to base (0c16507).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##           branch-23.02   #1182   +/-   ##
============================================
  Coverage          0.00%   0.00%           
============================================
  Files                 6       6           
  Lines               421     421           
============================================
  Misses              421     421           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vyasr vyasr mentioned this pull request Dec 21, 2022
3 tasks
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.

🔥 🔥 🔥 🔥

@github-actions github-actions bot added the ci label Dec 21, 2022
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.

All the shared action branches need to get repointed once the associated PR is merged, but aside from that just a couple of minor comments. Excited to see this change!

Comment on lines 6 to 8
print(buf.size)
print(buf.ptr)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the prints.

Suggested change
print(buf.size)
print(buf.ptr)

build_type: pull-request
package-dir: python
package-name: rmm
skbuild-configure-options: "-DRMM_BUILD_WHEELS=ON"
Copy link
Contributor

Choose a reason for hiding this comment

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

It annoys me that we have to specify this in every repo, but I don't see a great WAR at this stage. For now, just making a note that this is one more bit of boilerplate that it would be nice to get rid of somehow.

@vyasr
Copy link
Contributor

vyasr commented Dec 21, 2022

Would also recommend following Bradley's naming suggestion on cudf.

@sevagh
Copy link
Contributor Author

sevagh commented Jan 6, 2023

Would also recommend following rapidsai/cudf#12427 (comment).

That addressed the multiple wheel blocks for cudf:

cudf-wheel-build
dask-cudf-wheel-build

With Bradley's suggestion, that became:

wheel-build-cudf
wheel-build-dask-cudf

In RMM's case, there's only one wheel, so the wheel blocks never had a leading rmm-; unless you'd prefer I add a trailing -rmm:

wheel-build-rmm

@vyasr
Copy link
Contributor

vyasr commented Jan 6, 2023

You're right, I commented that a bit blindly and it doesn't entirely apply here. The current job naming is fine with me.

@sevagh sevagh merged commit 3732b31 into rapidsai:branch-23.02 Jan 6, 2023
rapids-bot bot pushed a commit that referenced this pull request Jan 9, 2023
An unnecessary parameter (`repo: rapidsai/rmm`) was left behind after the wheel PR was merged in #1182 

This PR cleans it up

Authors:
  - Sevag H (https://github.com/sevagh)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #1187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci 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.

4 participants