Skip to content

Rely on cuDF's Serializable in CumlArray#2281

Merged
dantegd merged 1 commit intorapidsai:branch-0.14from
jakirkham:use_serializable
May 20, 2020
Merged

Rely on cuDF's Serializable in CumlArray#2281
dantegd merged 1 commit intorapidsai:branch-0.14from
jakirkham:use_serializable

Conversation

@jakirkham
Copy link
Member

Depends on PR ( rapidsai/cudf#5139 ) and nightlies with that change.

Update CumlArray to rely on cuDF's Serializable, which cuDF's Buffer inherits from. In particular Serializable already provides pickling support through a __reduce_ex__ method of its own, which just builds off of the serialize and deserialize methods that all Serializable subclasses must implement and CumlArray already does (either by directly implementing them or relying on cuDF Buffer's implementations). So no extra work is needed to just drop this.

Update `CumlArray` to rely on cuDF's `Serializable`, which cuDF's
`Buffer` inherits from. In particular `Serializable` already provides
pickling support through a `__reduce_ex__` method of its own, which just
builds off of the `serialize` and `deserialize` methods that all
`Serializable` subclasses must implement and `CumlArray` already does
(either by directly implementing them or relying on cuDF `Buffer`'s
implementations). So no extra work is needed to just drop this.
@jakirkham
Copy link
Member Author

cc @dantegd @cjnolet

@dantegd
Copy link
Member

dantegd commented May 20, 2020

@jakirkham the PR fails seem to indicate me that an issue with serializing/pickling the data inheritted as is from Serializable is serializing the data as a Buffer and losing the datatype information (which causes all that havoc), perhaps we do need to override methods? The errors in CI are very similar to the ones I saw when the new reduce_ex in Buffer broke CumlArray

@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review Cython / Python Cython or Python issue labels May 20, 2020
@jakirkham
Copy link
Member Author

rerun tests

(cudf nightlies with the change are now available)

@jakirkham
Copy link
Member Author

jakirkham commented May 20, 2020

Sorry for the confusion. I think the failure is was just because we didn't have the Conda packages with the change. Rerunning CI to confirms.

Should add here's Serializable's __reduce_ex__ method for context. We've dropped all other __reduce__/__reduce_ex__ methods of cuDF objects inheriting from Serializable. There are already quite a few tests of pickling in cuDF, which were happy with the change. So I think we should be good.

That all being said, if I'm wrong about this, happy to take a closer look tomorrow 🙂

@dantegd dantegd merged commit b25c436 into rapidsai:branch-0.14 May 20, 2020
@jakirkham jakirkham deleted the use_serializable branch May 21, 2020 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 - Waiting on Author Waiting for author to respond to review Cython / Python Cython or Python issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants