Skip to content

Use numba-cuda>=0.0.13 - #16474

Merged
rapids-bot[bot] merged 11 commits into
NVIDIA:branch-24.12from
gmarkall:use-numba-cuda
Sep 27, 2024
Merged

Use numba-cuda>=0.0.13#16474
rapids-bot[bot] merged 11 commits into
NVIDIA:branch-24.12from
gmarkall:use-numba-cuda

Conversation

@gmarkall

@gmarkall gmarkall commented Aug 2, 2024

Copy link
Copy Markdown
Contributor

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 NVIDIA/cudf#16474) ensuring
numba-cuda is present.
@msarahan

msarahan commented Aug 2, 2024

Copy link
Copy Markdown
Member

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

gmarkall commented Aug 2, 2024

Copy link
Copy Markdown
Contributor Author

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

gmarkall commented Aug 2, 2024

Copy link
Copy Markdown
Contributor Author

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

Comment thread conda/recipes/cudf/meta.yaml Outdated
Co-authored-by: Mike Sarahan <msarahan@gmail.com>
Comment thread dependencies.yaml Outdated
Comment thread dependencies.yaml Outdated
@gmarkall gmarkall added the DO NOT MERGE Hold off on merging; see PR for details label Aug 5, 2024
@gmarkall

gmarkall commented Aug 5, 2024

Copy link
Copy Markdown
Contributor Author

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 DO NOT MERGE Hold off on merging; see PR for details label Aug 30, 2024
@gmarkall

Copy link
Copy Markdown
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

vyasr commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

@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

gmarkall commented Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

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

vyasr commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

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

@vyasr

vyasr commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

/merge

@bdice

bdice commented Sep 19, 2024

Copy link
Copy Markdown
Contributor

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 DO NOT MERGE Hold off on merging; see PR for details label Sep 19, 2024
@vyasr

vyasr commented Sep 25, 2024

Copy link
Copy Markdown
Contributor

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
Comment thread dependencies.yaml
- matrix: {dependencies: "oldest"}
packages:
- numba==0.57.*
- *numba-cuda-dep

Copy link
Copy Markdown
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 DO NOT MERGE Hold off on merging; see PR for details label Sep 26, 2024
@vyasr

vyasr commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot
rapids-bot Bot merged commit 0632538 into NVIDIA:branch-24.12 Sep 27, 2024
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 NVIDIA/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