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

Use numba-cuda>=0.0.13 #16474

Merged
merged 11 commits into from
Sep 27, 2024
Merged

Conversation

gmarkall
Copy link
Contributor

@gmarkall gmarkall commented Aug 2, 2024

Description

Testing with https://github.com/NVIDIA/numba-cuda on CI.

I am not sure if edits in other repos are required (e.g. I used to have to change an "integration" repo) - any pointers appreciated!

Checklist

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

@gmarkall gmarkall requested a review from a team as a code owner August 2, 2024 09:22
@gmarkall gmarkall requested a review from msarahan August 2, 2024 09:22
@gmarkall gmarkall added non-breaking Non-breaking change numba Numba issue improvement Improvement / enhancement to an existing function labels Aug 2, 2024
@github-actions github-actions bot added the Python Affects Python cuDF API. label Aug 2, 2024
copy-pr-bot bot pushed a commit to rapidsai/integration that referenced this pull request Aug 2, 2024
numba-cuda is the NVIDIA-maintained CUDA target for Numba, which depends
on the numba package.

This PR replaces the numba dependency with numba-cuda. However, similar to
@bdice's comment in #648, I don't know if we strictly need this here any
more due to the cudf dependency (from rapidsai/cudf#16474) ensuring
numba-cuda is present.
@msarahan
Copy link
Contributor

msarahan commented Aug 2, 2024

This looks right from the recipe edit side. You should only have to manually edit dependencies.yaml, and all the other files should then be generated.

I don't understand the failing builds. I'm having a hard time finding the error that kills the build. Are you seeing this locally?

@gmarkall
Copy link
Contributor Author

gmarkall commented Aug 2, 2024

Thanks @msarahan - unfortunately I edited the PR before I saw your comment - should I revert that last change?

I haven't run this locally because I'm not really set up to build / test cudf at the moment (it's not what I usually work on), but will give it a try on Monday.

@gmarkall
Copy link
Contributor Author

gmarkall commented Aug 2, 2024

Just noticed the problem, I introduced a space that shouldn't have been there.

dependencies.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
@gmarkall gmarkall added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 5, 2024
@gmarkall
Copy link
Contributor Author

gmarkall commented Aug 5, 2024

Thanks for looking at this @msarahan - I've just marked as "DO NOT MERGE" for now until I speak to Ben about this later in the Numba weekly sync - I want to first check whether there should be some discussion about the implications of this with other developers - will update here later.

@gmarkall gmarkall removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 30, 2024
@gmarkall
Copy link
Contributor Author

@msarahan Following the discussion in the cudf meeting last week the consensus seemed to be that this was OK to proceed with, so I've removed the "DO NOT MERGE" label, fixed up the conflicts, and I'm just waiting on CI.

@vyasr
Copy link
Contributor

vyasr commented Sep 3, 2024

@gmarkall what else do we want to see on this PR before we merge? If we're happy with it, I'd encourage moving forward ASAP so we have some decent runway to test it before the 24.10 release. CC @bdice @brandon-b-miller

@gmarkall
Copy link
Contributor Author

gmarkall commented Sep 3, 2024

Thanks Vyas - I don't think there's anything else we want to see in it, I just wanted to have made sure to have discussed it with the cuDF team before merging.

@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

Cool. Going to queue this up for merge then, the dep changes look fine.

@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

/merge

@bdice
Copy link
Contributor

bdice commented Sep 19, 2024

Let's wait to adopt this until numba-cuda is available on conda-forge: conda-forge/staged-recipes#27600

Then let's set the minimum version to the first version available on conda-forge, to avoid cross-channel constraints from causing problems.

@bdice bdice added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 19, 2024
@vyasr
Copy link
Contributor

vyasr commented Sep 25, 2024

Given the delay with the conda-forge recipe and there now being conflicts on this branch, let's push this upgrade to 24.12. That will give us more time to iron out any kinks if anything goes unexpectedly wrong with the switch (particularly if it happens downstream of cudf).

@vyasr vyasr changed the base branch from branch-24.10 to branch-24.12 September 26, 2024 22:20
@@ -720,7 +720,7 @@ dependencies:
matrices:
- matrix: {dependencies: "oldest"}
packages:
- numba==0.57.*
- *numba-cuda-dep
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I changed this so that we now require numba-cuda even to test the oldest supported dependency. If at some point we choose to support a range of versions then we should remove the alias here and insert a real version.

@vyasr vyasr removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 26, 2024
@vyasr
Copy link
Contributor

vyasr commented Sep 26, 2024

/merge

@rapids-bot rapids-bot bot merged commit 0632538 into rapidsai:branch-24.12 Sep 27, 2024
99 checks passed
copy-pr-bot bot pushed a commit that referenced this pull request Sep 28, 2024
Testing with https://github.com/NVIDIA/numba-cuda on CI.

I am not sure if edits in other repos are required (e.g. I used to have to change an "integration" repo) - any pointers appreciated!

Authors:
  - Graham Markall (https://github.com/gmarkall)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

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

URL: #16474
rapids-bot bot pushed a commit to rapidsai/integration that referenced this pull request Oct 22, 2024
**EDIT:** This now removes the numba dependency, which comes transitively from cuDF.

**Original description:**

numba-cuda is the NVIDIA-maintained CUDA target for Numba, which depends on the numba package.

This PR replaces the numba dependency with numba-cuda. However, similar to @bdice's comment in #648, I don't know if we strictly need this here any more due to the cudf dependency (from rapidsai/cudf#16474) ensuring numba-cuda is present.

Authors:
  - Graham Markall (https://github.com/gmarkall)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change numba Numba issue Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants