feat: add new dask-behavior protocol #409
Conversation
src/dask_awkward/lib/core.py
Outdated
| _dask_set: Callable | None = None | ||
| _dask_del: Callable | None = None |
There was a problem hiding this comment.
I defined __selattr__ and __delattr__ although these wouldn't make sense in a functional API. We could come up with a dak.setattr and dak.delattr function that always returns the input array, i.e. _dask_set actually returns self, but I doubt we actually need __delattr__ support.
As such, we probably want to remove these other descriptor methods.
tests/test_behavior.py
Outdated
| return "this is a non-dask property" | ||
|
|
||
| def non_dask_method(self, _dask_array_=None): | ||
| return _dask_array_ | ||
| @some_property.dask_getter |
There was a problem hiding this comment.
If we drop __setattr__ support, then this can become @some_property.dask
|
I'll need to poke at this in coffea to make sure it covers what's needed. |
|
I'm fine with making a change like this (and I'll add that beyond behaviors "just working", I'd consider @lgray's opinion a more important one than mine w.r.t. the ergonomics, since he originally wrote this as a coffea need) |
|
should I wait until all this is green to give it a check? |
|
@lgray should be ready to go now! |
|
OK - there's something I need to make work first (the multi-return stuff that @iasonkrom found) and then I'll give this a proper looking over. |
|
I also noticed this! It's unrelated (main) c.f. #413 |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
=======================================
Coverage 93.81% 93.82%
=======================================
Files 23 23
Lines 3153 3156 +3
=======================================
+ Hits 2958 2961 +3
Misses 195 195 ☔ View full report in Codecov by Sentry. |
|
@lgray any thoughts on this? :) |
… of github.com:dask-contrib/dask-awkward into agoose77/refactor-dask-behavior-interface
|
@lgray I fixed a bug that caused your regression. Would you be happy to test this PR again? |
|
This passes in coffea, so we can look to merging. @lgray would you be happy to pin |
|
Yes - I've already pinned that PR to the next release of dask-awkward. Please go ahead and merge / release! |
|
@douglasdavis I already pinged you for review but realised I didn't add context: this is good to go, are you happy to sign off on it? |
This PR is a suggestion for replacing
_dask_array_with an extension to the descriptor protocol, namely_dask_get,_dask_set, etc.The motivations for this are to avoid reserving a name within the function signatures, keep property descriptors looking like property descriptors, and to avoid the need to compute the function signature.
Unfortunately, if the fallback case of using
map_partitionswere implemented naively, we'd need to support a collection type that represents anAnytype. As this isn't feasible, this PR uses a heuristic (as we already do) to determine whether the descriptor represents a method (in which case, delaying the call to map_partitions). The heuristic that I chose is to test whether the resolved attribute iscallable.If we are interested in this PR, then I'd probably want to add a fallback for coffea, unless they want to sync up again with coffea's latest release.
@lgray @douglasdavis