Restore CloudPickler-scoped globals namespace isolation for pickled functions #240

pierreglaser · 2019-01-29T16:37:47Z

This PR changes the way globals collisions are handled:

before, already existing globals would override globals shipped in a function's pickle string
now, it is the opposite: any global shipped with a function will have the priority over already existing globals in the depickling environment.

The tests are accordingly updated. Note that #216 did not add any new tests, but simply parametrized existing ones.

EDIT: For the record, the previous behavior (silently override globals shipped with the functions) introduced regressions reported by users of downstream projects. Here is the list of issues related to it:

@ogrisel it is a good PR to trigger the new downstream integration builds!

ogrisel · 2019-01-30T10:21:36Z

A bit of background: this PR restores the behavior of cloudpickle from before 0.6.0.

In 0.6.0 and 0.6.1 we changed the dynamic module globals handling behavior as we thought it was a bug (lack of consistency with how globals are handled in non-dynamic modules) but user feedback including #214 revealed that the new behavior is surprising/annoying for most users.

In #216 we explored the possibility of introducing of a switch on the CloudPickler class to control the overriding behavior but in retrospect we felt that there wasn't any realistic use cases that would mandate the override_globals=False case.

So to keep things simple, this PR hardcodes the override_globals=True case to restore a behavior equivalent to what cloudpickle 0.5 and earlier had.

ogrisel · 2019-01-30T10:23:52Z

@ogrisel it is a good PR to trigger the new downstream integration builds!

I agree, please feel free to push an empty commit with the [ci downstream] flag to do so.

pierreglaser · 2019-01-30T10:49:50Z

The failure comes from test_workdir_simple, which at first glance seems unrelated to cloudpickle.
I'll add a [ci distributed] commit to see if this was a random failure.

Edit: the next build passed, so indeed it looks like a random failure.

codecov-io · 2019-01-30T10:50:22Z

Codecov Report

Merging #240 into master will decrease coverage by 0.48%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   85.39%   84.91%   -0.49%     
==========================================
  Files           1        1              
  Lines         589      570      -19     
  Branches      118      112       -6     
==========================================
- Hits          503      484      -19     
  Misses         63       63              
  Partials       23       23

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	`84.91% <66.66%> (-0.49%)`	⬇️

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 7b31ced...c207afc. Read the comment docs.

ogrisel

LGTM. Just a few comments:

CHANGES.md

cloudpickle/cloudpickle.py

mitar · 2019-01-30T16:05:08Z

@suquark, can you check this?

CHANGES.md

suquark · 2019-01-31T07:34:24Z

@mitar I think this PR is nice and it will solve some previous problems. And I will help testing Ray with the latest cloudpickle (#238).

cloudpickle/cloudpickle.py

pcmoritz · 2019-02-06T17:01:19Z

This doesn't seem to fix the issue fully for us. Not sure what is going on, do you have an idea @suquark?

pierreglaser · 2019-02-06T18:19:21Z

@pcmoritz sorry, I still had a couple fixes to do that I thought I already pushed. This PR now reverts cloudpickle's globals handling behavior to 0.5.3. Back then this behavior was something we did not fully understand (and tested). Hopefully, this area will get steadier ☺️ .

pcmoritz · 2019-02-06T22:47:31Z

@pierreglaser Great, I re-ran the Ray test suite on top of your latest commits and it seems to be working now. Thanks a lot for the fix :)

ogrisel

First pass at reviewing the new version of this PR.

cloudpickle/cloudpickle.py

Co-Authored-By: pierreglaser <[email protected]>

pierreglaser · 2019-02-07T11:15:47Z

Here is a gist that loads "old" (e.g created using cloudpickle 0.5, 0.6, 0.7) pickle strings of functions of dynamically created modules using the reversion introduced in this PR. So far so good.

PS: sorry about the ugly force push, i forgot I resolved some conflicts in the browser...

cloudpickle/cloudpickle.py

ogrisel · 2019-02-07T13:24:54Z

@pierreglaser please launch downstream CI on this PR to check that we have not broken anything.

ogrisel · 2019-02-07T13:28:23Z

CHANGES.md

+  variables when loaded in their new environment. This restores the (previously
+  untested) behavior of cloudpickle prior to changes done in 0.5.4 for
+  functions defined in the `__main__` module,  and 0.6.0/1 for other dynamic
+  functions.


There is no "override" anymore because we create a new empty namespace each time. May I suggest the following:

- Global variables referenced by functions pickled by cloudpickle are now unpickled in a new and isolated namespace scoped by the CloudPickler instance. This restores the (previously untested) behavior of cloudpickle prior to changes done in 0.5.4 for functions defined in the `__main__` module, and 0.6.0/1 for other dynamic functions.

Yes you are right, this was stale. I'll let the downstream integration test finish before pushing a new commit.

ogrisel · 2019-02-07T14:18:52Z

You can trigger downstream CI again, I merged your PR to fix the broken loky test.

ogrisel

+1 for merge once the downstream CI has completed and the changelog is updated.

ogrisel · 2019-02-07T14:52:20Z

Merged. Thanks for your work @pierreglaser! Thanks for your feedback @pcmoritz!

## What changes were proposed in this pull request? After upgrading cloudpickle to 0.6.1 at #20691, one regression was found. Cloudpickle had a critical cloudpipe/cloudpickle#240 for that. Basically, it currently looks existing globals would override globals shipped in a function's, meaning: **Before:** ```python >>> def hey(): ... return "Hi" ... >>> spark.range(1).rdd.map(lambda _: hey()).collect() ['Hi'] >>> def hey(): ... return "Yeah" ... >>> spark.range(1).rdd.map(lambda _: hey()).collect() ['Hi'] ``` **After:** ```python >>> def hey(): ... return "Hi" ... >>> spark.range(1).rdd.map(lambda _: hey()).collect() ['Hi'] >>> >>> def hey(): ... return "Yeah" ... >>> spark.range(1).rdd.map(lambda _: hey()).collect() ['Yeah'] ``` Therefore, this PR upgrades cloudpickle to 0.8.0. Note that cloudpickle's release cycle is quite short. Between 0.6.1 and 0.7.0, it contains minor bug fixes. I don't see notable changes to double check and/or avoid. There is virtually only this fix between 0.7.0 and 0.8.1 - other fixes are about testing. ## How was this patch tested? Manually tested, tests were added. Verified unit tests were added in cloudpickle. Closes #23904 from HyukjinKwon/SPARK-27000. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

ogrisel mentioned this pull request Jan 30, 2019

ENH Add a switch to override existing global variables #216

Closed

ogrisel approved these changes Jan 30, 2019

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

ogrisel reviewed Jan 30, 2019

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

ogrisel mentioned this pull request Jan 30, 2019

Add test for nested interactively defined functions with code changes joblib/loky#197

Merged

ogrisel mentioned this pull request Jan 31, 2019

Loky cache should be disabled by default joblib/joblib#836

Closed

pierreglaser commented Feb 4, 2019

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

pcmoritz mentioned this pull request Feb 6, 2019

Update cloudpickle to 0.8.0.dev0 ray-project/ray#3964

Merged

ogrisel mentioned this pull request Feb 7, 2019

CLN drop Cloudpickler's globals_ref attribute #229

Closed

ogrisel reviewed Feb 7, 2019

View reviewed changes

pierreglaser added 12 commits February 7, 2019 11:50

globals shipped with func override existing ones

517ca19

TST update tests using globals accordingly

21adb87

DOC update CHANGES.md

2165d40

DOC clearer comments

4f92a84

DOC update CHANGES.md

d07ccb9

DOC typo

0f195ee

DOC mention the 0.5.4 change

0519ee1

specify future backward-compatibilty code

6ab9b9b

flag backward compat code for dynamic modules

3dbf2e1

wip

8ee5ac4

make use of dict.setdefault for globals_ref

f8a0e39

TST remove user-defined dynamic modules tests

75ca9af

pierreglaser and others added 5 commits February 7, 2019 11:51

TST comments rewording and a few more assertions

f4c68ae

Apply suggestions from code review

178b6aa

Co-Authored-By: pierreglaser <[email protected]>

Apply suggestions from code review

9615db0

Co-Authored-By: pierreglaser <[email protected]>

MNT remove code for dynamic modules persistance

8ca8066

MNT, DOC flag backward compatibility code

f4f15e3

pierreglaser force-pushed the force-existing-globals-overriding branch from 13afdd2 to f4f15e3 Compare February 7, 2019 11:15

ogrisel reviewed Feb 7, 2019

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

DOC, MNT update namsespace populating comment

644199e

ogrisel mentioned this pull request Feb 7, 2019

joblib.Parallel overwrites args variable for n_jobs > 1 joblib/joblib#833

Closed

[ci downstream] trigger downstream projects builds

19db7fc

ogrisel reviewed Feb 7, 2019

View reviewed changes

ogrisel changed the title ~~Force existing globals overriding~~ Restore CloudPickler-scoped globals namespace isolation for pickled functions Feb 7, 2019

pierreglaser mentioned this pull request Feb 7, 2019

FIX fix nested function test exit signal joblib/loky#198

Merged

[ci downstream] trigger downstream builds

45ceaed

ogrisel approved these changes Feb 7, 2019

View reviewed changes

DOC, MNT udpate stale changelog

c207afc

ogrisel merged commit 9184a67 into cloudpipe:master Feb 7, 2019

pierreglaser mentioned this pull request Feb 7, 2019

functools.wraps can pollute a function's global namespace #242

Closed

This was referenced Feb 7, 2019

Pickling lambda function that accepts a global as argument is broken on windows #230

Closed

New features in cloudpickle give unexpected results #214

Closed

TheButlah mentioned this pull request Feb 7, 2019

Cloudpickle broken on Windows TheButlah/makrl#28

Closed

nmatthews-asapp mentioned this pull request Feb 13, 2019

Update cloudpickle to 0.6.1 ray-project/ray#3076

Closed

HyukjinKwon mentioned this pull request Feb 27, 2019

[SPARK-27000][PYTHON] Upgrades cloudpickle to v0.8.0 apache/spark#23904

Closed

pierreglaser mentioned this pull request May 29, 2019

References to global objects are broken #275

Closed

pierreglaser mentioned this pull request Feb 20, 2021

Support persistent function.__globals__ #411

Open

Restore CloudPickler-scoped globals namespace isolation for pickled functions #240

Restore CloudPickler-scoped globals namespace isolation for pickled functions #240

Uh oh!

Conversation

pierreglaser commented Jan 29, 2019 • edited by ogrisel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Jan 30, 2019

Uh oh!

pierreglaser commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mitar commented Jan 30, 2019

Uh oh!

Uh oh!

suquark commented Jan 31, 2019

Uh oh!

Uh oh!

pcmoritz commented Feb 6, 2019

Uh oh!

pierreglaser commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcmoritz commented Feb 6, 2019

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierreglaser commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ogrisel commented Feb 7, 2019

Uh oh!

ogrisel Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

pierreglaser Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 7, 2019

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pierreglaser commented Jan 29, 2019 •

edited by ogrisel

Loading

ogrisel commented Jan 30, 2019 •

edited

Loading

pierreglaser commented Jan 30, 2019 •

edited

Loading

codecov-io commented Jan 30, 2019 •

edited

Loading

pierreglaser commented Feb 6, 2019 •

edited

Loading

pierreglaser commented Feb 7, 2019 •

edited

Loading

pierreglaser Feb 7, 2019 •

edited

Loading