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

Update GpuRunningWindowExec to use OOM retry framework #8170

Merged
merged 31 commits into from
May 8, 2023
Merged

Update GpuRunningWindowExec to use OOM retry framework #8170

merged 31 commits into from
May 8, 2023

Conversation

andygrove
Copy link
Contributor

@andygrove andygrove commented Apr 21, 2023 •
edited
Loading

Closes #7809

This PR introduces a retry block in GpuRunningWindowIterator.computeRunning around the fixUp code. See comments in code for more information.

@andygrove andygrove changed the title Implement CheckpointRestore for BatchedRunningWindowFixer Update GpuRunningWindowExec to use OOM retry framework Apr 21, 2023
@andygrove andygrove self-assigned this Apr 21, 2023
@andygrove andygrove added the reliability Features to improve reliability or bugs that severly impact the reliability of the plugin label Apr 21, 2023
@andygrove andygrove marked this pull request as ready for review April 24, 2023 19:31
@andygrove andygrove marked this pull request as draft April 25, 2023 19:06
revans2
revans2 reviewed May 1, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 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. Now in GpuWindowIterator we need to add in a retry. That means that the WindowRetrySuite tests will need to change, but I think it would just mean that we need to setup an iterator and call next on it.

Also could you file a follow on issue to fix GpuCachedDoublePassWindowIterator I forgot that I added that in.

@andygrove
Copy link
Contributor Author

Also could you file a follow on issue to fix GpuCachedDoublePassWindowIterator I forgot that I added that in.

I filed #8217 for this.

revans2
revans2 reviewed May 3, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Looking good. No new comments to add

@andygrove andygrove marked this pull request as ready for review May 5, 2023 22:25
@andygrove
Copy link
Contributor Author

I've marked this PR as ready for review, but I am running into a segfault after the first unit test passes and I am still trying to track that down. The seg fault occurs in Java_ai_rapids_cudf_ColumnVector_deleteCudfColumn.

jbrennan333
jbrennan333 previously approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@jbrennan333 jbrennan333 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 good to me.

revans2
revans2 previously approved these changes May 8, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Just a nit

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuWindowExec.scala Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuWindowExec.scala Outdated
numOutputRows += ret.numRows()
ret
}
// TODO maybe should create spillable batch here before calling computeRunning
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Remove the TODO, what we have it fine, especially once the spill code changes are in so that making something spillable is super cheap.

@andygrove andygrove dismissed stale reviews from revans2 and jbrennan333 via 02e2c47 May 8, 2023 18:44
@andygrove
Copy link
Contributor Author

build

@andygrove andygrove merged commit bf58d90 into NVIDIA:branch-23.06 May 8, 2023
@andygrove andygrove deleted the windows-reliability-1 branch May 8, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrennan333 jbrennan333 jbrennan333 approved these changes

@revans2 revans2 revans2 approved these changes

Assignees

@andygrove andygrove

Labels
reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Update GpuRunningWindowExec to use OOM retry framework
3 participants
@andygrove @revans2 @jbrennan333