Introduce incompatible-types and enables spilling of CuPy arrays #856
Conversation
Which replace ignore_types
madsbk
added
2 - In Progress
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #856 +/- ##
=================================================
+ Coverage 64.17% 88.47% +24.30%
=================================================
Files 22 16 -6
Lines 3076 2117 -959
=================================================
- Hits 1974 1873 -101
+ Misses 1102 244 -858
Continue to review full report at Codecov.
|
By maintaining key_containts_incompatible_type, we don't have to call unproxify_device_objects() unnecessary.
madsbk
added
3 - Ready for Review
| incompatibles = dask.config.get("jit-unspill-incompatible", "cupy.ndarray") | ||
| incompatibles = incompatibles.split(",") |
There was a problem hiding this comment.
Thanks for working on this @madsbk .
I am curious, What about stuff like cupyx.scipy.sparse and CumlArray .
Will this work with those too ?
There was a problem hiding this comment.
Currently only cupy.ndarray is on the incompatible list but if you encounter problems with those classes please let me know.
There was a problem hiding this comment.
Okay. So i guess if we set DASK_JIT_UNSPILL_INCOMPATIBLE = cupy.ndarray, CumlArray, cupyx.scipy.sparse.csr_matrix and just try that to see if that fixes the gpu-bdb stuff, ( which potenitally maybe failing with just cupy.ndarrays and see ?.
There was a problem hiding this comment.
Yes, and also try with just cupy.ndarray to see if CumlArray and cupyx.scipy.sparse.csr_matrix works with ProxyObject
dask_cuda/proxify_host_file.py
Outdated
| del self.store[key] | ||
| del self.key_contains_incompatible_type[key] |
There was a problem hiding this comment.
Wouldn't it be simpler to keep a single dict storing a (obj, incompatible_type_found)? Maybe I'm missing some context, but a single dict would potentially make it less error prone on forgetting to update one of them.
There was a problem hiding this comment.
I added a comment explaining exactly what self.store is. Please let me know if you think the comment is accurate.
There was a problem hiding this comment.
Thanks @shwina , I think the changes to combine dicts look good, but I didn't see any comment, any chance you missed pushing the change?
There was a problem hiding this comment.
I'm confused, I still don't see any comments. 😅
Could you paste a link to that diff where the comment is contained?
There was a problem hiding this comment.
I kept pushing to my fork instead of Mads' :( Sorry for the confusion. Should be updated on this PR now.
There was a problem hiding this comment.
No worries Ashwin, I see your comment now and I think it's explaining correctly, thanks for adding that.
Co-authored-by: Peter Andreas Entschev <[email protected]>
Co-authored-by: Peter Andreas Entschev <[email protected]>
|
rerun tests |
|
@gpucibot merge |
|
Thanks @shwina for finishing the PR and thanks @pentschev for the review :) |
This PR replaces
ignore_typesintroduced in #568 withincompatible_types, which is a list of types thatProxifyHostFilewill unproxify on retrieval. This makes it possible to spill types we previously ignored completely such ascupy.ndarray.To mark a type incompatible, add it to the comma separated config value
"jit-unspill-incompatible"or environment variableDASK_JIT_UNSPILL_INCOMPATIBLE.The default value is:
DASK_JIT_UNSPILL_INCOMPATIBLE="cupy.ndarray"Closes #855
Notice, I have marked the PR
breakingbecause theDASK_JIT_UNSPILL_IGNOREoption has been removed.cc. @VibhuJawa