Skip to content

[PyROOT] Add __reduce__ method to ROOTFacade to help serialization tools#7886

Merged
vepadulano merged 1 commit intoroot-project:masterfrom
vepadulano:auto-import-facade
Apr 20, 2021
Merged

[PyROOT] Add __reduce__ method to ROOTFacade to help serialization tools#7886
vepadulano merged 1 commit intoroot-project:masterfrom
vepadulano:auto-import-facade

Conversation

@vepadulano
Copy link
Copy Markdown
Member

@vepadulano vepadulano commented Apr 15, 2021

Fixes #6764 , see the comment with explanation there

@vepadulano vepadulano self-assigned this Apr 15, 2021
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 15, 2021

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@eguiraud
Copy link
Copy Markdown
Contributor

eguiraud commented Apr 15, 2021

Both the commit message and the code might use some comments, otherwise well done finding this out!

@vepadulano vepadulano changed the title [PyROOT] Automatically import ROOT facade when serializing [skip-ci][PyROOT] Automatically import ROOT facade when serializing Apr 16, 2021
@vepadulano
Copy link
Copy Markdown
Member Author

vepadulano commented Apr 16, 2021

Alright, I added docstrings to the functions in _facade.py . I also added some unittests, we need to discuss whether we want to include the tests with cloudpickle which were the origin of the linked issue, but we would need to install the packages on the nodes and probably add another ROOTTEST envvar to avoid running them in certain nodes

@vepadulano
Copy link
Copy Markdown
Member Author

vepadulano commented Apr 16, 2021 •
edited
Loading

Thought it was worth for our reference to keep track also of the behaviour of the dill module in the tests (stemming from #6370)

@vepadulano vepadulano changed the title [skip-ci][PyROOT] Automatically import ROOT facade when serializing [PyROOT] Add __reduce__ method to ROOTFacade to help serialization tools Apr 19, 2021
@vepadulano
Copy link
Copy Markdown
Member Author

vepadulano commented Apr 19, 2021

After discussion, we decided to only keep pickle tests, whereas cloudpickle usecase is implicitly tested with distributed RDataFrame. See the sibling PR in rottest for the additional test of the serialization of the facade

@vepadulano vepadulano marked this pull request as ready for review April 19, 2021 11:35
@vepadulano vepadulano requested a review from etejedor as a code owner April 19, 2021 11:35
@vepadulano vepadulano mentioned this pull request Apr 19, 2021
Merged
etejedor
etejedor approved these changes Apr 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@etejedor etejedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating this @vepadulano !

@vepadulano vepadulano merged commit 7768b4e into root-project:master Apr 20, 2021
@vepadulano vepadulano deleted the auto-import-facade branch October 25, 2021 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@etejedor etejedor etejedor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vepadulano vepadulano

Labels

improvement in:Python Interface

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pickling functions using the ROOT module with cloudpickle breaks

4 participants

@vepadulano @phsft-bot @eguiraud @etejedor