[MRG] Regression on pickling classes from the __main__ module by ogrisel · Pull Request #132 · cloudpipe/cloudpickle

ogrisel · 2017-11-13T15:32:16Z

This PR aims to fix the regression reported in #131 and that affects 0.4.2 and later. Right now there is is just a non-regression test to reproduce the problem.

I believe the regression was silently introduced by @pitrou's cleanups in #122 although I am not 100% sure yet because this code is quite complex.

pitrou · 2017-11-13T15:50:25Z

+from subprocess import check_output
 from subprocess import PIPE
+from subprocess import STDOUT
+from subprocess import CalledProcessError


Can you import all those names at once? :-)

pitrou · 2017-11-13T15:53:10Z

+ f.write(source_code.encode('utf-8'))
+
+ cmd = [sys.executable, source_file]
+ pythonpath = "{cwd}/tests:{cwd}".format(cwd=os.getcwd())


Can't you use __file__ or something? getcwd sounds fragile.

codecov-io · 2017-11-13T22:15:38Z

Codecov Report

Merging #132 into master will decrease coverage by 0.31%.
The diff coverage is 33.33%.

@@ Coverage Diff @@## master #132 +/- ##
==========================================
- Coverage 84.08% 83.76% -0.32% 
==========================================
Files 2 2 Lines 534 536 +2 Branches 97 98 +1 ==========================================
Hits 449 449 - Misses 63 64 +1 - Partials 22 23 +1

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	`83.67% <33.33%> (-0.32%)`	⬇️

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 07cf3a8...748e7d2. Read the comment docs.

ogrisel · 2017-11-13T22:15:57Z

@pitrou I have tried to make a minimal change to save_global to fix the tests but it does not feel right to me. Do you have a better suggestion?

ogrisel · 2017-11-13T22:17:50Z

The coverage decrease is caused by the new branch that can only be executed in subprocess calls and therefore not accounted in the report.

pitrou · 2017-11-13T22:29:56Z

I have tried to make a minimal change to save_global to fix the tests but it does not feel right to me.

I don't understand the fix. How does it work?

ogrisel · 2017-11-14T07:29:25Z

Apparently instances of (type, types.ClassType) from the __main__ module need to go through self.save_dynamic_class(obj) instead of Pickler.save_global(...).

ogrisel · 2017-11-14T07:38:58Z

I have pushed a simpler fix.

pitrou · 2017-11-14T11:03:53Z

 dispatched here.
 """
+ if obj.__module__ == "__main__":
+ return self.save_dynamic_class(obj)


Does this mean save_dynamic_class is also used for functions? If so, could you rename that method and update its docstring?

I don't think functions get dispatched here. The dispatch entries for save_global are:
dispatch[type] =save_globaldispatch[types.ClassType] =save_global
whereas function-likes are dispatched via:
dispatch[types.FunctionType] =save_function ... dispatch[types.MethodType] =save_instancemethod ... dispatch[types.BuiltinFunctionType] =save_builtin_function ... dispatch[classmethod] =save_classmethoddispatch[staticmethod] =save_classmethod
I believe the name save_global is a holdover from the base Pickler class, which calls self.save_global in some base class methods that we call into via super, so renaming this isn't straightforward.

I have pushed a new commit to simplify that function even further and all tests still pass. However, I am not sure we are not breaking edge cases in third party libraries and applications.

I have run the test suite of both loky and joblib against this branch all tests pass as well so I am pretty confident that the changes are fine.

Can you check whether save_dynamic_class really gets a function as input and, if so, change the name or docstring?

@pitrou I'm pretty confident save_dynamic_class can never be called with a function as input. It accesses obj.__bases__ unconditionally, which doesn't exist on function objects.

pitrou · 2017-11-14T20:27:29Z

 dispatched here.
 """
+ if obj.__module__ == "__main__":
+ return self.save_dynamic_class(obj)


Can you check whether save_dynamic_class really gets a function as input and, if so, change the name or docstring?

ogrisel · 2017-11-14T23:13:52Z

No it does not. It's only for classes that cannot be looked up as attributes from global modules. In our tests those are:

Classes that are defined in the local namespace of another function, method or class,
Classes that are defined in the __main__ module.

ogrisel · 2017-11-14T23:16:23Z

The name of the PR is misleading: the new non-regression tests call interactively defined functions from the __main__ module but the actual problem was caused by the fact that interactively defined classes used by those functions could not be unpickled by attribute from module lookup.

pitrou · 2017-11-15T12:01:46Z

Ok, thank you for clarifying :-)

pitrou · 2017-11-15T12:03:08Z

+ _builtin_type, (_BUILTIN_TYPE_NAMES[obj],), obj=obj)

-raise
+return self.save_dynamic_class(obj)


Does save_dynamic_class raise a readable exception or do we want to keep the if above?

ogrisel · 2017-11-15T15:40:25Z

Maybe it's good idea to keep the protection:

>>>importcloudpickle>>>importio>>>p=cloudpickle.CloudPickler(io.BytesIO)
>>>p.save_dynamic_class(lambdax: x)
FatalPythonerror: Cannotrecoverfromstackoverflow.
Thread0x00007fc32d281700 (mostrecentcallfirst):
File"/home/ogrisel/.virtualenvs/py36/lib/python3.6/site-packages/IPython/core/history.py", line764in_writeout_input_cacheFile"/home/ogrisel/.virtualenvs/py36/lib/python3.6/site-packages/IPython/core/history.py", line780inwriteout_cacheFile"/home/ogrisel/.virtualenvs/py36/lib/python3.6/site-packages/IPython/core/history.py", line58inneeds_sqliteFile"<decorator-gen-23>", line2inwriteout_cacheFile"/home/ogrisel/.virtualenvs/py36/lib/python3.6/site-packages/IPython/core/history.py", line834inrunFile"/home/ogrisel/.virtualenvs/py36/lib/python3.6/site-packages/IPython/core/history.py", line58inneeds_sqliteFile"<decorator-gen-24>", line2inrunFile"/usr/lib/python3.6/threading.py", line916in_bootstrap_innerFile"/usr/lib/python3.6/threading.py", line884in_bootstrapCurrentthread0x00007fc33466b700 (mostrecentcallfirst):
File"/usr/lib/python3.6/pickle.py", line264in_getattributeFile"/usr/lib/python3.6/pickle.py", line918insave_globalFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line635insave_globalFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line375insave_functionFile"/usr/lib/python3.6/pickle.py", line476insaveFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line480insave_dynamic_classFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line642insave_globalFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line375insave_functionFile"/usr/lib/python3.6/pickle.py", line476insaveFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line480insave_dynamic_classFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line642insave_globalFile"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line375insave_function
...
Aborted (coredumped)

ogrisel · 2017-11-15T15:48:13Z

Actually the snippet from the previous comment is misleading, I passed the BytesIO class instead of an instance to build the pickler. With a correct pickler, the error message is fine I think:

>>>importcloudpickle>>>importio>>>p=cloudpickle.CloudPickler(io.BytesIO())
>>>p.save_dynamic_class(lambdax: x)
Traceback (mostrecentcalllast):
File"<ipython-input-8-4356e554f31b>", line1, in<module>p.save_dynamic_class(lambdax: x)
File"/home/ogrisel/code/cloudpickle/cloudpickle/cloudpickle.py", line490, insave_dynamic_class# encountered while saving will point to the skeleton class.AttributeError: 'function'objecthasnoattribute'__bases__'

pitrou · 2017-11-15T15:52:03Z

But what was the previous message? It seems to me that Pickler generally raises PicklingError.

ogrisel · 2017-11-15T16:11:07Z

p.save_dynamic_classes(lambda x: x) would raise the same Attribute error in 0.4.1 (as this part of the code has not changed).
p.save_global(lambda x: x) would raise a PicklingError: Can't pickle <function <lambda> at 0x7ff4d2ff97b8> while it now raises the AttributeError.

However, I am not sure we should try to preserve the behavior of the private API on wrong inputs. What really matters is that it should raise a meaningful exception when calling the public API with wrong inputs and calling loads(dumps(arbitrary_func)) should work on 0.5.2 as it used to work in 0.4.1 and not yield an exception.

ogrisel · 2017-11-15T16:13:34Z

@robertnishihara @iaroslav-ai could you please try this branch with your code to confirm that it fixes all your problems?

ogrisel · 2017-11-15T16:34:31Z

@pitrou it's true that the upstreamsave_global raises PicklingError when it cannot lookup such an object definition. I will restore this behavior in this branch.

However this case should never happen in cloudpickle as far as I know because cloudpickle has been written precisely to be able to pickle dynamic class and function definitions. I am not sure how to write a test for this.

This reverts commit d9e02fb.

robertnishihara · 2017-11-15T20:06:26Z

@ogrisel Thanks, I just tried it out and this seems to fix my problem.

iaroslav-ai · 2017-11-15T23:33:18Z

Yup that seems to solve it. Thanks @ogrisel !

…lasses from the __main__ module

New tests for interactively defined functions
d3c1f46

pitrou reviewed Nov 13, 2017
View reviewed changes

ogrisel added 6 commits November 13, 2017 19:09

More robust non-regression test
8d7c003

Use __file__ as reference instead of os.getcwd()
5d2ae3e

Nicer indentation handling for inline code snippet
9443907

cleanup
b7014de

More comprehensive interactive function tests
99bfb95

Fix interactive function pickling
acf5edd

ogrisel changed the title ~~[WIP] Regression on pickling functions from __main__ module~~[MRG] Regression on pickling functions from __main__ moduleNov 13, 2017

Simpler fix
7da07a3

pitrou reviewed Nov 14, 2017
View reviewed changes

Further simplification of save_global
d9e02fb

pitrou requested changes Nov 14, 2017
View reviewed changes

ogrisel changed the title ~~[MRG] Regression on pickling functions from __main__ module~~[MRG] Regression on pickling classes from the __main__ moduleNov 14, 2017

pitrou reviewed Nov 15, 2017
View reviewed changes

pitrou approved these changes Nov 15, 2017
View reviewed changes

Add entry to CHANGES.md
20feec2

Revert "Further simplification of save_global"
748e7d2
This reverts commit d9e02fb.

rgbkrk merged commit fa4b1ce into cloudpipe:masterNov 16, 2017

ogrisel deleted the fix-interactive-func branch November 16, 2017 10:03

ueshin mentioned this pull request Jan 29, 2018
[SPARK-23159][PYTHON] Update cloudpickle to v0.4.3 apache/spark#20373
Closed

BryanCutler added a commit to BryanCutler/spark that referenced this pull request Jan 29, 2018
backport ofcloudpipe/cloudpickle#132, [MRG] Regression on pickling c…
e7eabe3
…lasses from the __main__ module

HyukjinKwon mentioned this pull request Feb 5, 2018
[MRG][BRANCH-0.4.x] Regression on pickling classes from the __main__ module #149
Merged

Uh oh!

Conversation

ogrisel commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ogrisel commented Nov 13, 2017

Uh oh!

ogrisel commented Nov 13, 2017

Uh oh!

pitrou commented Nov 13, 2017

Uh oh!

ogrisel commented Nov 14, 2017

Uh oh!

ogrisel commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Nov 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Nov 14, 2017

Uh oh!

pitrou commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Nov 15, 2017

Uh oh!

ogrisel commented Nov 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Nov 15, 2017

Uh oh!

ogrisel commented Nov 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Nov 15, 2017

Uh oh!

ogrisel commented Nov 15, 2017

Uh oh!

robertnishihara commented Nov 15, 2017

Uh oh!

iaroslav-ai commented Nov 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Nov 13, 2017•
edited
Loading

ogrisel commented Nov 14, 2017•
edited
Loading

ogrisel commented Nov 15, 2017•
edited
Loading

ogrisel commented Nov 15, 2017•
edited
Loading