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 ArrowDeviceArray interface to pass address of event #16058

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Fix ArrowDeviceArray interface to pass address of event #16058

merged 3 commits into from
Jul 11, 2024

Conversation

zeroshade
Copy link
Contributor

Description

the sync_event member of ArrowDeviceArray needs to be a pointer to a cudaEvent_t, currently we're returning the cudaEvent_t directly. We need to be passing the address of the event. Thankfully this is a single line change, plus adding a test to confirm.

Checklist

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

@zeroshade zeroshade requested a review from a team as a code owner June 18, 2024 20:10
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 18, 2024

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.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 18, 2024
@zeroshade
Copy link
Contributor Author

CC @vyasr

@davidwendt davidwendt added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Jun 24, 2024
@davidwendt
Copy link
Contributor

/ok to test

vyasr
vyasr reviewed Jun 24, 2024
View reviewed changes
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.

Thanks for the fix! I think you're missing cudaEventSynchronize calls currently. Could we also add these checks to the other (non-empty table) tests?

@zeroshade
Copy link
Contributor Author

Added calls to cudaEventSynchronize

@vyasr
Copy link
Contributor

vyasr commented Jun 28, 2024

/ok to test

@zeroshade
Copy link
Contributor Author

@vyasr anything else needed for this to get merged?

@davidwendt
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit dddeb12 into rapidsai:branch-24.08 Jul 11, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr left review comments

@davidwendt davidwendt davidwendt approved these changes

@hyperbolic2346 hyperbolic2346 hyperbolic2346 approved these changes

@pmattione-nvidia pmattione-nvidia Awaiting requested review from pmattione-nvidia pmattione-nvidia is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zeroshade @davidwendt @vyasr @hyperbolic2346