-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alias CloudPickler in the cloudpickle namespace as Pickler #235
Conversation
Codecov Report
@@ Coverage Diff @@
## master #235 +/- ##
==========================================
+ Coverage 85.12% 85.15% +0.02%
==========================================
Files 1 1
Lines 585 586 +1
Branches 117 117
==========================================
+ Hits 498 499 +1
Misses 63 63
Partials 24 24
Continue to review full report at Codecov.
|
@ogrisel Can you take a look at this? Thanks! |
I am fine with this. Please add a changelog entry targeting the 0.8.0 release. Before merging I would like to have the opinion of other developers / downstream users of cloudpickle. Pseudo-random ping: |
No objection from me.
…On Thu, Jan 24, 2019 at 5:55 AM Olivier Grisel ***@***.***> wrote:
I am fine with this. Please add a changelog entry targeting the 0.8.0
release.
Before merging I would like to have the opinion of other developers /
downstream users of cloudpickle. Pseudo-random ping:
@rgbkrk <https://github.com/rgbkrk> @pitrou <https://github.com/pitrou>
@HyukjinKwon <https://github.com/HyukjinKwon> @mrocklin
<https://github.com/mrocklin> ...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#235 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASszPAMa_VmZ7aWGPpS5fI5GGR4PbSmks5vGbs3gaJpZM4aP1TI>
.
|
I don't think we should change the public API of the OTOH you could object that
Well PyTorch should perhaps be more flexible. I don't understand the concept of making the module name configurable, rather than the actual serialization functions (or classes). |
+1 for using a |
@@ -64,7 +64,7 @@ | |||
|
|||
|
|||
if sys.version < '3': | |||
from pickle import Pickler | |||
from pickle import Pickler as _Pickler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with this, we better leave a comment as to why we're using _Pickler
here as some future contributor & maintainer may not know the context and end up breaking this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbczumar -- could you add the context around this PR to the import(s) to help our future selves?
Currently, the
Pickler
class in thecloudpickle
namespace refers topickle.Pickler
. Certain libraries, such as PyTorch, leverage thePickler
class of a specified pickling module to serialize objects (see https://github.com/pytorch/pytorch/blob/fc5b79cd1c6020c20640128e43bac43fd636121e/torch/serialization.py#L290). When these libraries attempt reference thecloudpickle.Pickler
, they obtain thepickle.Pickler
class instead ofcloudpickle.CloudPickler
.Working under the assumption that users referring to
cloudpickle.Pickler
desire/expect a reference tocloudpickle.CloudPickler
, this PR aliasescloudpickle.CloudPickler
ascloudpickle.Pickler
and importspickle.Pickler
as_Pickler
to avoid a naming conflict.