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.

Sign up for GitHub

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

Jump to bottom

fix: Upstream API changes in cuDF 24.10 #3309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Upstream API changes in cuDF 24.10 #3309

wants to merge 1 commit into from

Conversation

martindurant
Copy link
Contributor

No description provided.

@martindurant martindurant mentioned this pull request Nov 19, 2024
Open
@martindurant martindurant changed the title Upstream API changes in cuDF 24.10 fix: Upstream API changes in cuDF 24.10 Nov 19, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.06%. Comparing base (b749e49) to head (e100461).
Report is 202 commits behind head on main.

Files with missing lines Patch % Lines
src/awkward/operations/ak_to_cudf.py 0.00% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/contents/listoffsetarray.py 81.25% <ø> (-1.62%) ⬇️
src/awkward/operations/ak_to_cudf.py 75.00% <0.00%> (ø)

... and 159 files with indirect coverage changes

---- 🚨 Try these New Features:

jpivarski
jpivarski reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Does this need to be coordinated with a cuDF release? If we merge this, would we gain the ability to work with cuDF >= 24.10 and lose the ability to work with cuDF < 24.10?

Also, I should be careful not to merge this PR without resetting the rapidjson submodule.

image

src/awkward/operations/ak_to_cudf.py
@@ -18,4 +18,4 @@ def to_cudf(
"""
import cudf

return cudf.Series(array.layout._to_cudf(cudf, None, len(array)))
return cudf.Series._from_column(array.layout._to_cudf(cudf, None, len(array)))
Copy link
Member

Choose a reason for hiding this comment

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

This functionality has become private. Is it something we're not supposed to do anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be worth finding the git blame on this, and asking on their repo

@martindurant
Copy link
Contributor Author

I should fix this to check the existence of the class method, so that it works for both new and old versions. I am using this branch for the moment for making my akimbo talk :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpivarski jpivarski jpivarski left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martindurant @jpivarski