Use elif in Dask deserialize check
#4537
Conversation
jakirkham
added
3 - Ready for Review
| @@ -44,7 +44,7 @@ def deserialize_cudf_object(header, frames): | |||
| # some frames are empty -- meta/empty partitions/etc | |||
| if len(f) > 0: | |||
| assert hasattr(f, "__cuda_array_interface__") | |||
| if header["serializer"] == "dask": | |||
| elif header["serializer"] == "dask": | |||
There was a problem hiding this comment.
Moving discussion to here. I understand that this should be elif, but how could both conditions evaluate to True here regardless?
There was a problem hiding this comment.
It wouldn't be possible.
There was a problem hiding this comment.
@kkraus14 are you suggesting this should be else and not elif ?
There was a problem hiding this comment.
No, I wasn't aware that header['serializer'] could be a list of elements, but I'm still not clear on how both conditions could trigger here because it's ==.
There was a problem hiding this comment.
I think that is incorrect. header["serializer"] should only be a single str with a serializer named.
Ben was mentioning that multiple serializers could be used to transform the data (and its various components), which is different from the value seen here.
There was a problem hiding this comment.
@jakirkham So then is this a fix for something or just preventing an additional conditional check?
There was a problem hiding this comment.
Just preventing an additional check. Something I stumbled across recently when looking at the serialization code.
|
rerun tests |
kkraus14
added
5 - Ready to Merge
Codecov Report
@@ Coverage Diff @@
## branch-0.13 #4537 +/- ##
===============================================
+ Coverage 88.68% 88.70% +0.01%
===============================================
Files 50 50
Lines 9800 9850 +50
===============================================
+ Hits 8691 8737 +46
- Misses 1109 1113 +4
Continue to review full report at Codecov.
|
Slight tweak to the checks added in PR ( #4423 ).