Avoid deprecated jit_unspill in dask tests#5456
Avoid deprecated jit_unspill in dask tests#5456rapids-bot[bot] merged 2 commits intorapidsai:mainfrom
Conversation
rapidsai/dask-cuda#1631 deprecated jit-unspilling in dask-cuda in favor of cudf's native spilling. For now we match dask-cuda's default of None. Next release, when the deprecation is enforced, we'll need to remove they keyword.
| rmm_pool_size=None, | ||
| dask_worker_devices=None, | ||
| jit_unspill=False, | ||
| jit_unspill=None, |
There was a problem hiding this comment.
I wasn't 100% sure if cugraph.testing was part of the public API. If our tests are the only ones using this then we can just remove it.
There was a problem hiding this comment.
I'm not aware of it being used elsewhere (@rlratzel is probably the main person who can say definitively), so yeah it's probably safer to keep it as is for now. Perhaps leave a breadcrumb note such as:
| jit_unspill=None, | |
| jit_unspill=None, # Unused (deprecated). Safe to remove if not used elsewhere |
Also, I would suggest to similarly update jit_unspill in the docstring.
There was a problem hiding this comment.
Thanks, Tom and Erik. It is indeed safe to remove so I just did that.
|
Thanks @TomAugspurger. Would you recommend setting (or being able to set) Also, it looks like the mention of JIT-unspill in the dask-cudf best practices doc could also be updated: https://docs.rapids.ai/api/dask-cudf/stable/best_practices/#enable-cudf-spilling |
|
Thanks for the link to the outdated docs. I'll get that fixed.
I personally don't have a recommendation, so using whatever the default is is probably for the best. API-design wise, I'd say that unless cugraph has a strong reason to prefer otherwise (because it knows something about the problem / domain that dask-cuda doesn't), I'd leave cluster creation up to the user / dask-cuda. But I'm not familiar with the history of cugraph so maybe there's a reason for the way things are now. |
jameslamb
added
improvement
Signed-off-by: rlratzel <rratzel@nvidia.com>
| rmm_pool_size=None, | ||
| dask_worker_devices=None, | ||
| jit_unspill=False, | ||
| jit_unspill=None, |
There was a problem hiding this comment.
Thanks, Tom and Erik. It is indeed safe to remove so I just did that.
|
/merge |
5 participants
rapidsai/dask-cuda#1631 deprecated jit-unspilling in dask-cuda in favor of cudf's native spilling.
For now we match dask-cuda's default of None, which will avoid any warnings.
Next release, when the deprecation is enforced, we'll need to remove they keyword.