From ed4bdeb180a0dcd87b73af38739664763bd09fa1 Mon Sep 17 00:00:00 2001 From: daogilvie Date: Mon, 17 Oct 2022 15:26:03 +0100 Subject: [PATCH 1/8] Swap plugin cache to pickle-able values when done When the dictionary has served its purpose (making plugin loading pre-and-post init a consistent behaviour), we swap it for bools indicating whether or not a module was loaded. We don't currently use the bools, but it seemed a sensible choice. The main idea is to make the dictionary fully pickle-able, so that when dill pickles the linter for multiprocessing, it doesn't crash horribly. --- doc/whatsnew/fragments/7635.bugfix | 7 +++++++ pylint/lint/pylinter.py | 14 ++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 doc/whatsnew/fragments/7635.bugfix diff --git a/doc/whatsnew/fragments/7635.bugfix b/doc/whatsnew/fragments/7635.bugfix new file mode 100644 index 0000000000..184e509eaf --- /dev/null +++ b/doc/whatsnew/fragments/7635.bugfix @@ -0,0 +1,7 @@ +Fixes a multi-processing crash that prevents using any more than 1 thread on MacOS. + +The returned module objects and errors that were cached by the linter plugin loader +cannot be reliably pickled. This means that ``dill`` would throw an error when +attempting to serialise the linter object for multi-processing use. + +Closes #7635. diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 12726e2780..d0e1b3ea96 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -296,7 +296,7 @@ def __init__( str, list[checkers.BaseChecker] ] = collections.defaultdict(list) """Dictionary of registered and initialized checkers.""" - self._dynamic_plugins: dict[str, ModuleType | ModuleNotFoundError] = {} + self._dynamic_plugins: dict[str, ModuleType | ModuleNotFoundError | bool] = {} """Set of loaded plugin names.""" # Attributes related to registering messages and their handling @@ -402,7 +402,17 @@ def load_plugin_configuration(self) -> None: "bad-plugin-value", args=(modname, module_or_error), line=0 ) elif hasattr(module_or_error, "load_configuration"): - module_or_error.load_configuration(self) + # Mypy is not smart enough to realise that we only call + # this line if the object has the attr we are looking at. + # Hence the one-line type: ignore[union-attr] here. + module_or_error.load_configuration(self) # type: ignore[union-attr] + # We re-set all the dictionary values to True here to make sure the dict + # is pickleable. This is only a problem in multiprocessing/parallel mode. + # (eg invoking pylint -j 2) + self._dynamic_plugins = { + modname: not isinstance(val, ModuleNotFoundError) + for modname, val in self._dynamic_plugins.items() + } def _load_reporters(self, reporter_names: str) -> None: """Load the reporters if they are available on _reporters.""" From d62cb05be71f383c7ec3a3b23929e0924c88a9ec Mon Sep 17 00:00:00 2001 From: daogilvie Date: Mon, 17 Oct 2022 15:35:14 +0100 Subject: [PATCH 2/8] Fix spelling in comment complaints from pylint --- pylint/lint/pylinter.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index d0e1b3ea96..5095b3d970 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -402,13 +402,13 @@ def load_plugin_configuration(self) -> None: "bad-plugin-value", args=(modname, module_or_error), line=0 ) elif hasattr(module_or_error, "load_configuration"): - # Mypy is not smart enough to realise that we only call + # Mypy is not smart enough to realize that we only call # this line if the object has the attr we are looking at. # Hence the one-line type: ignore[union-attr] here. module_or_error.load_configuration(self) # type: ignore[union-attr] # We re-set all the dictionary values to True here to make sure the dict - # is pickleable. This is only a problem in multiprocessing/parallel mode. - # (eg invoking pylint -j 2) + # is pickle-able. This is only a problem in multiprocessing/parallel mode. + # (e.g. invoking pylint -j 2) self._dynamic_plugins = { modname: not isinstance(val, ModuleNotFoundError) for modname, val in self._dynamic_plugins.items() From d982854c5ee788094c93eb42b301a1b0cde026bd Mon Sep 17 00:00:00 2001 From: Drummond Ogilvie Date: Mon, 17 Oct 2022 18:36:52 +0100 Subject: [PATCH 3/8] Correct tense in news fragment 7635.bugfix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> --- doc/whatsnew/fragments/7635.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/whatsnew/fragments/7635.bugfix b/doc/whatsnew/fragments/7635.bugfix index 184e509eaf..72085e0291 100644 --- a/doc/whatsnew/fragments/7635.bugfix +++ b/doc/whatsnew/fragments/7635.bugfix @@ -1,4 +1,4 @@ -Fixes a multi-processing crash that prevents using any more than 1 thread on MacOS. +Fixed a multi-processing crash that prevents using any more than 1 thread on MacOS. The returned module objects and errors that were cached by the linter plugin loader cannot be reliably pickled. This means that ``dill`` would throw an error when From 170d923cfa93b87796a9d160d7ed289d95b20566 Mon Sep 17 00:00:00 2001 From: Drum Ogilvie Date: Mon, 17 Oct 2022 19:13:40 +0100 Subject: [PATCH 4/8] Add a test for linter pickle-ability This should cover the case where plugins are loaded and parallel running is enabled. --- tests/test_check_parallel.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_check_parallel.py b/tests/test_check_parallel.py index 0c872ff8da..1c6a0f57f8 100644 --- a/tests/test_check_parallel.py +++ b/tests/test_check_parallel.py @@ -11,6 +11,8 @@ import argparse import multiprocessing import os +import sys +from pathlib import Path import dill import pytest @@ -231,6 +233,22 @@ def test_worker_check_single_file_no_checkers(self) -> None: assert stats.statement == 18 assert stats.warning == 0 + def test_linter_with_plugins_is_pickleable(self) -> None: + """ + The linter needs to be pickle-able in order to be passed between workers + """ + dummy_plugin_path = Path(__file__).parent / "regrtest_data" / "dummy_plugin" + dummy_plugin_path_str = str(dummy_plugin_path.absolute()) + sys.path.append(dummy_plugin_path_str) + linter = PyLinter(reporter=Reporter()) + print(linter._dynamic_plugins) + try: + dill.dumps(linter) + except dill.PicklingError: + assert False, "Loading plugins caused an un-pickle-able linter" + finally: + sys.path.remove(dummy_plugin_path_str) + def test_worker_check_sequential_checker(self) -> None: """Same as test_worker_check_single_file_no_checkers with SequentialTestChecker.""" linter = PyLinter(reporter=Reporter()) From 1c0653557226bf9e261182338e9b5d93822e8ac7 Mon Sep 17 00:00:00 2001 From: daogilvie Date: Tue, 18 Oct 2022 09:02:51 +0100 Subject: [PATCH 5/8] Fix test to use specifically pickle-unsafe module The test also includes an assert to confirm the module picked is actually unsafe, in case it is made safe in the future. Also include docstring edits suggested by reviews. --- pylint/lint/pylinter.py | 3 --- tests/test_check_parallel.py | 24 ++++++++++-------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 5095b3d970..60fbba67d4 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -402,9 +402,6 @@ def load_plugin_configuration(self) -> None: "bad-plugin-value", args=(modname, module_or_error), line=0 ) elif hasattr(module_or_error, "load_configuration"): - # Mypy is not smart enough to realize that we only call - # this line if the object has the attr we are looking at. - # Hence the one-line type: ignore[union-attr] here. module_or_error.load_configuration(self) # type: ignore[union-attr] # We re-set all the dictionary values to True here to make sure the dict # is pickle-able. This is only a problem in multiprocessing/parallel mode. diff --git a/tests/test_check_parallel.py b/tests/test_check_parallel.py index 1c6a0f57f8..dbac8f9c20 100644 --- a/tests/test_check_parallel.py +++ b/tests/test_check_parallel.py @@ -11,8 +11,6 @@ import argparse import multiprocessing import os -import sys -from pathlib import Path import dill import pytest @@ -233,21 +231,19 @@ def test_worker_check_single_file_no_checkers(self) -> None: assert stats.statement == 18 assert stats.warning == 0 - def test_linter_with_plugins_is_pickleable(self) -> None: - """ - The linter needs to be pickle-able in order to be passed between workers - """ - dummy_plugin_path = Path(__file__).parent / "regrtest_data" / "dummy_plugin" - dummy_plugin_path_str = str(dummy_plugin_path.absolute()) - sys.path.append(dummy_plugin_path_str) + def test_linter_with_unpickleable_plugins_is_pickleable(self) -> None: + """The linter needs to be pickle-able in order to be passed between workers""" linter = PyLinter(reporter=Reporter()) - print(linter._dynamic_plugins) + # We load an extension that we know is not pickle-safe + linter.load_plugin_modules(["pylint.extensions.overlapping_exceptions"]) + with pytest.raises(KeyError): + dill.dumps(linter) + # And expect this call to make it pickle-able + linter.load_plugin_configuration() try: dill.dumps(linter) - except dill.PicklingError: - assert False, "Loading plugins caused an un-pickle-able linter" - finally: - sys.path.remove(dummy_plugin_path_str) + except KeyError: + assert False, "Cannot pickle linter when using non-pickleable plugin" def test_worker_check_sequential_checker(self) -> None: """Same as test_worker_check_single_file_no_checkers with SequentialTestChecker.""" From 695fc1486cd243404d91f99deb7103342f008985 Mon Sep 17 00:00:00 2001 From: daogilvie Date: Tue, 18 Oct 2022 09:39:39 +0100 Subject: [PATCH 6/8] Restructure pickle test to for extra plugins Running in isolation and as part of the full suite cause different default plugins to be loaded, which throw different pickling errors. This will now catch all of them, and still fail the test if all of those modules become pickle safe in future. --- tests/test_check_parallel.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_check_parallel.py b/tests/test_check_parallel.py index dbac8f9c20..e6a7426f06 100644 --- a/tests/test_check_parallel.py +++ b/tests/test_check_parallel.py @@ -236,8 +236,12 @@ def test_linter_with_unpickleable_plugins_is_pickleable(self) -> None: linter = PyLinter(reporter=Reporter()) # We load an extension that we know is not pickle-safe linter.load_plugin_modules(["pylint.extensions.overlapping_exceptions"]) - with pytest.raises(KeyError): + try: dill.dumps(linter) + assert False, "Plugins loaded were pickle-safe! This test needs altering" + except (KeyError, TypeError, dill.PickleError): + pass + # And expect this call to make it pickle-able linter.load_plugin_configuration() try: From e35bd84c63a7113d41306a71276ac9f9faddab3d Mon Sep 17 00:00:00 2001 From: daogilvie Date: Thu, 20 Oct 2022 09:22:42 +0100 Subject: [PATCH 7/8] Address review comments from cdce8p Thanks for the review! --- pylint/lint/pylinter.py | 1 + tests/test_check_parallel.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 60fbba67d4..d337fd32c9 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -403,6 +403,7 @@ def load_plugin_configuration(self) -> None: ) elif hasattr(module_or_error, "load_configuration"): module_or_error.load_configuration(self) # type: ignore[union-attr] + # We re-set all the dictionary values to True here to make sure the dict # is pickle-able. This is only a problem in multiprocessing/parallel mode. # (e.g. invoking pylint -j 2) diff --git a/tests/test_check_parallel.py b/tests/test_check_parallel.py index e6a7426f06..420ed3d935 100644 --- a/tests/test_check_parallel.py +++ b/tests/test_check_parallel.py @@ -11,6 +11,7 @@ import argparse import multiprocessing import os +from pickle import PickleError import dill import pytest @@ -239,7 +240,7 @@ def test_linter_with_unpickleable_plugins_is_pickleable(self) -> None: try: dill.dumps(linter) assert False, "Plugins loaded were pickle-safe! This test needs altering" - except (KeyError, TypeError, dill.PickleError): + except (KeyError, TypeError, PickleError, NotImplementedError): pass # And expect this call to make it pickle-able From 93e7923b175933f6a9b2c13c5bb85f37defedcbe Mon Sep 17 00:00:00 2001 From: daogilvie Date: Thu, 20 Oct 2022 09:57:45 +0100 Subject: [PATCH 8/8] Shorten my name in the contributor file --- script/.contributors_aliases.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/.contributors_aliases.json b/script/.contributors_aliases.json index 95808f651c..ebd0aa76c3 100644 --- a/script/.contributors_aliases.json +++ b/script/.contributors_aliases.json @@ -438,7 +438,7 @@ }, "me@daogilvie.com": { "mails": ["me@daogilvie.com", "drum.ogilvie@ovo.com"], - "name": "Drummond Ogilvie" + "name": "Drum Ogilvie" }, "me@the-compiler.org": { "mails": [