Skip to content
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

Prepare transition to let cloudpickle.Pickler point to CloudPickler #372

Closed
wants to merge 1 commit into from
Closed

Conversation

twoertwein
Copy link
Contributor

Warn users of a future change that cloudpickle.Pickler (currently pointing to pickle._Pickler) will point to cloudpickle.CloudPickler.

See #366
Replaces #235

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #372 into master will decrease coverage by 6.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   92.95%   86.40%   -6.56%     
==========================================
  Files           2        2              
  Lines         809      809              
  Branches      164      164              
==========================================
- Hits          752      699      -53     
- Misses         29       78      +49     
- Partials       28       32       +4     
Impacted Files Coverage Δ
cloudpickle/cloudpickle.py 82.60% <100.00%> (-9.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0c3509...5365d6d. Read the comment docs.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Could you please add a test that checks that the warning is raised and that the cloudpickle.Pickler attribute is the same as pickle._Pickler for the time being?

Also it seems to not work as expected at the moment:

>>> from cloudpickle import Pickler                                                                                                                                             
Traceback (most recent call last):
  File "<ipython-input-3-9a7c65e34d9c>", line 1, in <module>
    from cloudpickle import Pickler
ImportError: cannot import name 'Pickler' from 'cloudpickle' (/home/ogrisel/code/cloudpickle/cloudpickle/__init__.py)

@twoertwein
Copy link
Contributor Author

thank you for testing!

I will add this test and avoid the f-string for older python versions.

It seems that this code probably only works for python >=3.7 [1]. It works for me on 3.8, which python version are you using?

There might be some 'ugly' tricks to get something like that working on <3.7 " Typical workarounds are assigning __class__ of a module object to a custom subclass of types.ModuleType or replacing the sys.modules item with a custom wrapper instance" [1]. I don't understand that yet but I can look into that, if we need to have a warning for all supported python versions. Otherwise, I will change the code to define __getattr__ only for >=3.7.

[1] https://www.python.org/dev/peps/pep-0562/

@twoertwein
Copy link
Contributor Author

We could raise the FutureWarning directly when cloudpickle is imported for <3.7 and for >=3.7 only when trying to access Pickler (to avoid annoying warnings).

@twoertwein
Copy link
Contributor Author

I don't understand why the test case fails for python >= 3.7

@pierreglaser
Copy link
Member

pierreglaser commented May 24, 2020

The tests are failing because you added a __getattr__ method to the cloudpickle.cloudpickle module while cloudpickle exposes cloudpickle.cloudpickle.Pickler directly from the cloudpickle package (see cloupickle/__init__.py). If you want a warning to be raised when executing the line from cloudpickle import Pickler you need to move your __getattr__ method to cloudpickle/__init__.py.

As for the compatibility issue with Python <= 3.6, it is actually relatively easy to patch cloudpickle.cloudpickle.__class__ to a custom ModuleType subclass to achieve a consistent cross-version behavior:

In [15]: import warnings
    ...: import types
    ...: class CustomModuleType(types.ModuleType):
    ...:     def __getattr__(self, name):
    ...:         if name == 'Pickler':
    ...:             warnings.warn('Pickler will point to Cloudpickler in two releases')
    ...:             return self._Pickler
    ...:         else:
    ...:             raise AttributeError
    ...: import cloudpickle.cloudpickle as cp
    ...: cp.__class__ = CustomModuleType

gives:

In [15]: cp.Pickler
/home/pierreglaser/.virtualenvs/cloudpickle_py37/bin/ipython:6: UserWarning: Pickler will point to Cloudpickler in two releases

Out[15]: pickle._Pickler

I'd rather not have cloudpickle output a warning each time it is imported.

@twoertwein
Copy link
Contributor Author

@pierreglaser thank you! I was importing cloudpickle while being in the ./cloudpickle folder.

The warnings are raised now and I can import Pickler :) but assert_run_python_script fails now to pickle cloudpickle: TypeError: cannot pickle 'CustomModuleType' object

@pierreglaser
Copy link
Member

Ah indeed, CustomModuleType should now have a __reduce__ method, because module objects are not pickleable. Something like

def __reduce__(self):
    return __import__, ("cloudpickle.cloudpickle",)

Should be enough, although I have not checked.

@twoertwein
Copy link
Contributor Author

@pierreglaser it seems that the from cloudpickle.cloudpickle import * doesn't find Pickler for python <3.7. I can directly access cloudpickle.cloudpickle.Pickler but not cloudpickle.Pickler. Do you understand why that is the case?

@twoertwein twoertwein closed this Jul 20, 2020
@ogrisel
Copy link
Contributor

ogrisel commented Oct 12, 2023

@pierreglaser it seems that the from cloudpickle.cloudpickle import * doesn't find Pickler for python <3.7. I can directly access cloudpickle.cloudpickle.Pickler but not cloudpickle.Pickler. Do you understand why that is the case?

Probably because we forgot to define __all__ in cloudpickle.__init__.py. This will be done as part of #517.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants