Skip to content

Commit

Permalink
Swap plugin cache to pickle-able values when done (#7640)
Browse files Browse the repository at this point in the history
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.

Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Marc Mueller <[email protected]>
  • Loading branch information
3 people authored Oct 20, 2022
1 parent e8ba3eb commit 015f340
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 3 deletions.
7 changes: 7 additions & 0 deletions doc/whatsnew/fragments/7635.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
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
attempting to serialise the linter object for multi-processing use.

Closes #7635.
12 changes: 10 additions & 2 deletions pylint/lint/pylinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -402,7 +402,15 @@ 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)
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)
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."""
Expand Down
2 changes: 1 addition & 1 deletion script/.contributors_aliases.json
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@
},
"[email protected]": {
"mails": ["[email protected]", "[email protected]"],
"name": "Drummond Ogilvie"
"name": "Drum Ogilvie"
},
"[email protected]": {
"mails": [
Expand Down
19 changes: 19 additions & 0 deletions tests/test_check_parallel.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import argparse
import multiprocessing
import os
from pickle import PickleError

import dill
import pytest
Expand Down Expand Up @@ -231,6 +232,24 @@ def test_worker_check_single_file_no_checkers(self) -> None:
assert stats.statement == 18
assert stats.warning == 0

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())
# We load an extension that we know is not pickle-safe
linter.load_plugin_modules(["pylint.extensions.overlapping_exceptions"])
try:
dill.dumps(linter)
assert False, "Plugins loaded were pickle-safe! This test needs altering"
except (KeyError, TypeError, PickleError, NotImplementedError):
pass

# And expect this call to make it pickle-able
linter.load_plugin_configuration()
try:
dill.dumps(linter)
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."""
linter = PyLinter(reporter=Reporter())
Expand Down

0 comments on commit 015f340

Please sign in to comment.