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

Add arguments to enable cuDF spilling and set statistics #1362

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

pentschev
Copy link
Member

Add arguments to enable cuDF spilling and set statistics in dask cuda worker/LocalCUDACluster. This is implemented as a Dask plugin, and does not require users anymore to rely on client.run to do that.

Closes #1280

@pentschev pentschev added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Jul 23, 2024
Copy link
Member

@rjzamora rjzamora 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 this @pentschev ! Only one minor comment/question from me.

Side thought: We have a lot of options at this point. Do you think it makes sense to deprecate anything?

Comment on lines +79 to +80
.. warning::
This should NOT be used together with JIT-Unspill.
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to detect if the user is doing this and warn/raise? This doc-string is good, but I'm sure some users will never read it :)

@pentschev
Copy link
Member Author

Warning added in d7ffa0a . @madsbk do you think we should just raise a warning or raise a warning and disable JIT-Unspill regardless of the user enabling both together, i.e., --enable-cudf-spill --enable-jit-unspill would emit a warning saying JIT-Unspill has been disabled and only cuDF spill will be active?

Side thought: We have a lot of options at this point. Do you think it makes sense to deprecate anything?

Probably @madsbk also has a better sense of that as well, do you have thoughts on this?

@madsbk
Copy link
Member

madsbk commented Jul 24, 2024

I think we should raise a warning only.

Side thought: We have a lot of options at this point. Do you think it makes sense to deprecate anything?

I think we should wait a bit with deprecating until we get a good solution from the NO-OOM work.

Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Thanks @pentschev, looks good

@pentschev
Copy link
Member Author

Thanks @rjzamora and @madsbk for reviewing!

@pentschev
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit d6cafc1 into rapidsai:branch-24.08 Jul 24, 2024
29 checks passed
@pentschev pentschev deleted the cudf-spill branch July 24, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change python python code needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to use cudf spilling with dask cuda
3 participants