Skip to content

ENH Add a switch to override existing global variables - #216

Closed
pierreglaser wants to merge 8 commits into
cloudpipe:masterfrom
pierreglaser:set-custom-globals-overriding-behavior
Closed

ENH Add a switch to override existing global variables#216
pierreglaser wants to merge 8 commits into
cloudpipe:masterfrom
pierreglaser:set-custom-globals-overriding-behavior

Conversation

@pierreglaser

Copy link
Copy Markdown
Member

Following #214.
This PR implements a switch called override_existing_globals, available from CloudPickler.__init__, cloudpickle.dumps and cloudpickle.dump.

  • If set to True, the globals from a function originally created in the __main__ module, dynamic modules or in a nested scope override the globals of the function's module at unpickling time.
  • If set to False, the existing variables in the function's module overrides the globals of those functions at unpickling time.

@codecov-io

codecov-io commented Nov 14, 2018

Copy link
Copy Markdown

Codecov Report

Merging #216 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #216 +/- ##
========================================
+ Coverage 84.92% 85% +0.07% 
========================================
Files 1 1 Lines 577 580 +3 Branches 114 115 +1 ========================================
+ Hits 490 493 +3 
Misses 63 63 Partials 24 24
Impacted FilesCoverage Δ
cloudpickle/cloudpickle.py85% <100%> (+0.07%)⬆️

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 7fd15c2...3f32518. Read the comment docs.

Comment threadcloudpickle/cloudpickle.py Outdated

@tomMoraltomMoral left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall, this looks good.

I agree that override_existing_globals should be passed through the state to ensure compatibility.
Also, some nitpicks on the tests and comments.

Comment threadcloudpickle/cloudpickle.py Outdated
Comment threadcloudpickle/cloudpickle.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
@pierreglaser

Copy link
Copy Markdown
MemberAuthor

@suquarkoverride_existing_globals is now in state.
@tomMoral I now loop over override_existing_globals in the test. Because of that, the diff looks messy. You can refer to this commit for a cleaner diff.

@tomMoraltomMoral left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. just a typo.

Comment threadcloudpickle/cloudpickle.py Outdated
@pierreglaser
pierreglaserforce-pushed the set-custom-globals-overriding-behavior branch from 0b818ac to 0f49713CompareNovember 15, 2018 13:06

@ogriselogrisel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 on my side besides the following comments.

I think we need a changelog entry targeting the 0.7 version.

Comment threadcloudpickle/cloudpickle.py Outdated
if state.get('override_existing_globals', True):
func.__globals__.update(state['globals'])
else:
for k, v in state['globals'].items():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would re-add the original comment for this block of the condition:

# Only set global variables that do not exist.

Comment threadcloudpickle/cloudpickle.py Outdated
Comment threadcloudpickle/cloudpickle.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
Comment threadtests/cloudpickle_test.py Outdated
@pierreglaser
pierreglaserforce-pushed the set-custom-globals-overriding-behavior branch from 06f38ba to f6d6337CompareJanuary 14, 2019 17:21

@ogriselogrisel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, please add an entry to the change log (for 0.7).

@pierreglaser
pierreglaserforce-pushed the set-custom-globals-overriding-behavior branch from 07f8a3e to 3f32518CompareJanuary 15, 2019 16:01
@mitar

Copy link
Copy Markdown

@suquark Does this address fully #214?

@suquark

Copy link
Copy Markdown
Contributor

@mitar It only partly fixes #214. Fixing another part is tricky. It is related to recursive functions and only fails in CI tests. We still don't know how it happens; it is about how Python deals with global variables of a created function.

@ogrisel

Copy link
Copy Markdown
Contributor

@suquark could you please provide more details? Do you have a standalone example of one such problematic recursive function?

@ogrisel

Copy link
Copy Markdown
Contributor

Closing in favor of #240.

Sign up for freeto 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.

6 participants

@pierreglaser@codecov-io@mitar@suquark@ogrisel@tomMoral