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

Port all conda recipes to rattler-build #18054

Merged
merged 54 commits into from
Mar 4, 2025

Conversation

gforsyth
Copy link
Contributor

@gforsyth gforsyth commented Feb 20, 2025

Description

Port all condabuild recipes over to use rattler-build instead.

Contributes to rapidsai/build-planning#47

  • To satisfy rattler, this changes all the licenses in the pyproject.toml files to the SPDX-compliant Apache-2.0 instead of Apache 2.0

Checklist

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

@gforsyth gforsyth added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 20, 2025
@gforsyth gforsyth requested review from a team as code owners February 20, 2025 16:40
@gforsyth gforsyth requested a review from msarahan February 20, 2025 16:40
@github-actions github-actions bot added Python Affects Python cuDF API. cudf.polars Issues specific to cudf.polars pylibcudf Issues specific to the pylibcudf package labels Feb 20, 2025
@gforsyth gforsyth changed the title Add rattler-build recipe for libcudf Port all conda recipes to rattler-build Feb 20, 2025
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.

Initial comments attached. Please also see my review on UCXX and apply any relevant requests from that PR to this one, too.

@gforsyth
Copy link
Contributor Author

Computers are the absolute worst.

Despite RAPIDS_CUDA_VERSION being a string, apparently in the process of splitting and extracting the major version, we're getting an integer instead:

       "context": {
        "version": "25.04.00a223",
        "minor_version": "25.04",
        "cuda_version": "11.8",
        "cuda_major": 11,
        "date_string": 250221,
        "head_rev": "1708a669"
      },

rattler-build has a filter for converting from string to int, but not from int to string.
I also can't reproduce this locally.

@gforsyth
Copy link
Contributor Author

rattler-build has a filter for converting from string to int, but not from int to string.
I also can't reproduce this locally.

Ok, wrapping in quotation marks works. It's very strange that this happens in only some repos, but I'll add "wrap these in strings all the time" to the rattler playbook

Copy link

copy-pr-bot bot commented Feb 25, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@gforsyth
Copy link
Contributor Author

/ok to test

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.

I have only a few leftover comments. However, I think we should merge this soon and work out the remaining items in a follow-up PR wherever we can. Getting this in will help us see any problems, and it'll help with reducing merge conflicts with other work that is going on.

@gforsyth
Copy link
Contributor Author

/ok to test

@gforsyth gforsyth force-pushed the rattler-build branch 2 times, most recently from bec4719 to e27c1f3 Compare February 26, 2025 16:36
@vyasr
Copy link
Contributor

vyasr commented Feb 26, 2025

rattler-build has a filter for converting from string to int, but not from int to string.
I also can't reproduce this locally.

Ok, wrapping in quotation marks works. It's very strange that this happens in only some repos, but I'll add "wrap these in strings all the time" to the rattler playbook

Is there a minimal "expected" behavior here that we could capture and write up in a rattler issue?

@gforsyth
Copy link
Contributor Author

Is there a minimal "expected" behavior here that we could capture and write up in a rattler issue?

I think it might be:

Given that shells have horrendously inconsistent behavior around types, make everything a string unless someone explicitly calls | int

@vyasr
Copy link
Contributor

vyasr commented Feb 26, 2025

Is there a minimal "expected" behavior here that we could capture and write up in a rattler issue?

I think it might be:

Given that shells have horrendously inconsistent behavior around types, make everything a string unless someone explicitly calls | int

That seems sensible. Want to write that up as a rattler-build issue?

@gforsyth
Copy link
Contributor Author

That seems sensible. Want to write that up as a rattler-build issue?

will do

@gforsyth
Copy link
Contributor Author

rattler-build issue for context types: prefix-dev/rattler-build#1451

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.

This looks great, thanks Gil! I have some open requests, but nothing worth blocking on so I'm approving now so that you can move ahead. Three requests beyond what I put in-line:

  1. Can you go through and resolve all the open discussion threads on this PR? There are a lot, and I want to make sure that we don't miss anything.
  2. For any changes that we've decided to punt on for this PR but that we do want to make, could we start documenting them somewhere? I'm fine if we just use something like a google doc (issues feel like extra overhead for how tiny many of the requests are) but it would be good to not lose track of things.
  3. In future PRs, could you please avoid force-pushing? We try to avoid those because they decouple the discussion history from the source since the commits vanish.

@gforsyth
Copy link
Contributor Author

gforsyth commented Mar 4, 2025

@vyasr thanks for the review -- I'm tracking all of the remaining open requests here: rapidsai/build-planning#47 (comment)

in re: force-pushing, it was a necessary evil because of some unverified commits from one of the lab machines that was continuously blocking CI from running, but I'll avoid it in the future.

@gforsyth
Copy link
Contributor Author

gforsyth commented Mar 4, 2025

/merge

@rapids-bot rapids-bot bot merged commit 45bd05d into rapidsai:branch-25.04 Mar 4, 2025
108 checks passed
@gforsyth gforsyth deleted the rattler-build branch March 4, 2025 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf.polars Issues specific to cudf.polars improvement Improvement / enhancement to an existing function non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants