From 5dd1c79d86206a2b367b761e20332c8edcb34444 Mon Sep 17 00:00:00 2001 From: tomMoral Date: Wed, 15 Aug 2018 16:37:04 +0200 Subject: [PATCH 1/7] FIX globals for functions defined in __main__ --- cloudpickle/cloudpickle.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index 33a431d47..4a6f45b3d 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -431,6 +431,7 @@ def _save_subimports(self, code, top_level_dependencies): Ensure de-pickler imports any package child-modules that are needed by the function """ + # check if any known dependency is an imported package for x in top_level_dependencies: if isinstance(x, types.ModuleType) and hasattr(x, '__package__') and x.__package__: @@ -623,7 +624,15 @@ def extract_func_data(self, func): # save the dict dct = func.__dict__ - base_globals = self.globals_ref.get(id(func.__globals__), {}) + base_globals = self.globals_ref.get(id(func.__globals__), None) + if base_globals is None: + # For functions defined in __main__, use vars(__main__) for + # base_global. This is necessary to share the global variables + # across the multiple multiple functions in the module. + if func.__module__ == "__main__": + base_globals = "__main__" + else: + base_globals = {} self.globals_ref[id(func.__globals__)] = base_globals return (code, f_globals, defaults, closure, dct, base_globals) @@ -1028,7 +1037,11 @@ def _fill_function(*args): else: raise ValueError('Unexpected _fill_value arguments: %r' % (args,)) - func.__globals__.update(state['globals']) + # Only set global variables that do not exist. + for k, v in state['globals'].items(): + if k not in func.__globals__: + func.__globals__[k] = v + func.__defaults__ = state['defaults'] func.__dict__ = state['dict'] if 'annotations' in state: @@ -1067,6 +1080,8 @@ def _make_skel_func(code, cell_count, base_globals=None): """ if base_globals is None: base_globals = {} + elif isinstance(base_globals, str): + base_globals = vars(sys.modules[base_globals]) base_globals['__builtins__'] = __builtins__ closure = ( From 659bf7a6ce2e741621f0db833bbe60611237eb26 Mon Sep 17 00:00:00 2001 From: tomMoral Date: Wed, 15 Aug 2018 16:38:28 +0200 Subject: [PATCH 2/7] TST global variables are shared in __main__ --- tests/cloudpickle_test.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index 8826bac19..d035c4044 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -841,6 +841,39 @@ def f4(x): """.format(protocol=self.protocol) assert_run_python_script(textwrap.dedent(code)) + def test_interactively_global_variable(self): + # Check that callables defined in the __main__ module of a Python + # script (or jupyter kernel) correctly retrieve global variables. + code = """\ + from testutils import subprocess_pickle_echo + from cloudpickle import dumps, loads + + VARIABLE = "uninitialized" + + def f0(): + global VARIABLE + VARIABLE = "initialized" + + def f1(): + return VARIABLE + + reload_f0 = loads(dumps(f0, protocol={protocol})) + reload_f1 = loads(dumps(f1, protocol={protocol})) + pickled_f1 = dumps(f1, protocol={protocol}) + + # Change the value of the global variable + reload_f0() + + # Ensure that the global variable is the same for another function + result_f1 = reload_f1() + assert result_f1 == "initialized", result_f1 + + # Ensure that unpickling the global variable does not change its value + result_pickled_f1 = loads(pickled_f1)() + assert result_pickled_f1 == "initialized", result_pickled_f1 + """.format(protocol=self.protocol) + assert_run_python_script(textwrap.dedent(code)) + @pytest.mark.skipif(sys.version_info >= (3, 0), reason="hardcoded pickle bytes for 2.7") def test_function_pickle_compat_0_4_0(self): From 6d3d86012197d433345bb9cba55dd901f3490916 Mon Sep 17 00:00:00 2001 From: tomMoral Date: Wed, 15 Aug 2018 16:59:07 +0200 Subject: [PATCH 3/7] CI update coverage to start on subprocesses --- .travis.yml | 3 ++- ci/install_coverage_subprocess_pth.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 ci/install_coverage_subprocess_pth.py diff --git a/.travis.yml b/.travis.yml index 4bb9f6174..42e099c80 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,9 +23,10 @@ before_script: - flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide - flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics + - python ci/install_coverage_subprocess_pth.py script: - if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then source activate testenv; fi - - PYTHONPATH='.:tests' py.test -r s --cov-config .coveragerc --cov=cloudpickle + - COVERAGE_PROCESS_START="$TRAVIS_BUILD_DIR/.coveragerc" PYTHONPATH='.:tests' py.test -r s after_success: - if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then source activate testenv; fi - codecov diff --git a/ci/install_coverage_subprocess_pth.py b/ci/install_coverage_subprocess_pth.py new file mode 100644 index 000000000..ec93abcaa --- /dev/null +++ b/ci/install_coverage_subprocess_pth.py @@ -0,0 +1,16 @@ +# Make it possible to enable test coverage reporting for Python +# code run in children processes. +# http://coverage.readthedocs.io/en/latest/subprocess.html + +import os.path as op +from distutils.sysconfig import get_python_lib + +FILE_CONTENT = u"""\ +import coverage; coverage.process_startup() +""" + +filename = op.join(get_python_lib(), 'coverage_subprocess.pth') +with open(filename, 'wb') as f: + f.write(FILE_CONTENT.encode('ascii')) + +print('Installed subprocess coverage support: %s' % filename) From e79dfc6ac66ee89143f248bd5c344732b769d03e Mon Sep 17 00:00:00 2001 From: tomMoral Date: Wed, 15 Aug 2018 17:35:02 +0200 Subject: [PATCH 4/7] CI use coverage for subprocesses --- .coveragerc | 1 + .travis.yml | 1 + tests/testutils.py | 4 ++++ 3 files changed, 6 insertions(+) diff --git a/.coveragerc b/.coveragerc index 3e94abf34..81c8abff8 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,5 +1,6 @@ [run] branch = True +parallel = True source = cloudpickle [report] diff --git a/.travis.yml b/.travis.yml index 42e099c80..cc3c3bd8a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,4 +29,5 @@ script: - COVERAGE_PROCESS_START="$TRAVIS_BUILD_DIR/.coveragerc" PYTHONPATH='.:tests' py.test -r s after_success: - if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then source activate testenv; fi + - coverage combine --append - codecov diff --git a/tests/testutils.py b/tests/testutils.py index a8187baf3..2a14e86c7 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -93,6 +93,10 @@ def assert_run_python_script(source_code, timeout=5): 'stderr': STDOUT, 'env': {'PYTHONPATH': pythonpath}, } + # If coverage is running, pass the config file to the subprocess + coverage_rc = os.environ.get("COVERAGE_PROCESS_START") + if coverage_rc: + kwargs['env']['COVERAGE_PROCESS_START'] = coverage_rc if timeout_supported: kwargs['timeout'] = timeout try: From ad6753863d1d68b94d62f78ca8569564da2adb7d Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Thu, 23 Aug 2018 09:12:52 +0200 Subject: [PATCH 5/7] Phrasing --- cloudpickle/cloudpickle.py | 2 +- tests/cloudpickle_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index 4a6f45b3d..5ec7d0267 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -628,7 +628,7 @@ def extract_func_data(self, func): if base_globals is None: # For functions defined in __main__, use vars(__main__) for # base_global. This is necessary to share the global variables - # across the multiple multiple functions in the module. + # across multiple functions in this module. if func.__module__ == "__main__": base_globals = "__main__" else: diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index d035c4044..03c6f5ad5 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -841,7 +841,7 @@ def f4(x): """.format(protocol=self.protocol) assert_run_python_script(textwrap.dedent(code)) - def test_interactively_global_variable(self): + def test_interactively_defined_global_variable(self): # Check that callables defined in the __main__ module of a Python # script (or jupyter kernel) correctly retrieve global variables. code = """\ From 93e62745ff21e63e9eed26e051ca13b22d2cf829 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Thu, 23 Aug 2018 11:38:28 +0200 Subject: [PATCH 6/7] Also test with subprocess_pickle_echo --- tests/cloudpickle_test.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index 03c6f5ad5..a5be0345d 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -844,10 +844,13 @@ def f4(x): def test_interactively_defined_global_variable(self): # Check that callables defined in the __main__ module of a Python # script (or jupyter kernel) correctly retrieve global variables. - code = """\ + code_template = """\ from testutils import subprocess_pickle_echo from cloudpickle import dumps, loads + def local_clone(obj, protocol=None): + return loads(dumps(obj, protocol=protocol)) + VARIABLE = "uninitialized" def f0(): @@ -857,22 +860,25 @@ def f0(): def f1(): return VARIABLE - reload_f0 = loads(dumps(f0, protocol={protocol})) - reload_f1 = loads(dumps(f1, protocol={protocol})) + cloned_f0 = {clone_func}(f0, protocol={protocol}) + cloned_f1 = {clone_func}(f1, protocol={protocol}) pickled_f1 = dumps(f1, protocol={protocol}) # Change the value of the global variable - reload_f0() + cloned_f0() # Ensure that the global variable is the same for another function - result_f1 = reload_f1() + result_f1 = cloned_f1() assert result_f1 == "initialized", result_f1 # Ensure that unpickling the global variable does not change its value result_pickled_f1 = loads(pickled_f1)() assert result_pickled_f1 == "initialized", result_pickled_f1 - """.format(protocol=self.protocol) - assert_run_python_script(textwrap.dedent(code)) + """ + for clone_func in ['local_clone', 'subprocess_pickle_echo']: + code = code_template.format(protocol=self.protocol, + clone_func=clone_func) + assert_run_python_script(textwrap.dedent(code)) @pytest.mark.skipif(sys.version_info >= (3, 0), reason="hardcoded pickle bytes for 2.7") From d2e879bf0c9f827426c506d1d2bb49d10b5be9e8 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Thu, 23 Aug 2018 11:54:47 +0200 Subject: [PATCH 7/7] Better value names --- tests/cloudpickle_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index a5be0345d..a591997a4 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -851,11 +851,11 @@ def test_interactively_defined_global_variable(self): def local_clone(obj, protocol=None): return loads(dumps(obj, protocol=protocol)) - VARIABLE = "uninitialized" + VARIABLE = "default_value" def f0(): global VARIABLE - VARIABLE = "initialized" + VARIABLE = "changed_by_f0" def f1(): return VARIABLE @@ -869,11 +869,11 @@ def f1(): # Ensure that the global variable is the same for another function result_f1 = cloned_f1() - assert result_f1 == "initialized", result_f1 + assert result_f1 == "changed_by_f0", result_f1 # Ensure that unpickling the global variable does not change its value result_pickled_f1 = loads(pickled_f1)() - assert result_pickled_f1 == "initialized", result_pickled_f1 + assert result_pickled_f1 == "changed_by_f0", result_pickled_f1 """ for clone_func in ['local_clone', 'subprocess_pickle_echo']: code = code_template.format(protocol=self.protocol,