Swap plugin cache to pickle-able values when done #7640
Conversation
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.
Pull Request Test Coverage Report for Build 3288211080
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks for the quick fix @daogilvie! Verified that it does resolve the issue. Left a small comment. Besides that the fix is fine IMO. Will leave the actual approval for someone with more experience with multitasking in pylint.
Co-authored-by: Daniël van Noord <[email protected]>
I honestly did not, no, because I wasn't sure how to do that beyond what the existing tests are doing in test_check_parallel. I'll look into it this evening; I'm sure there will be a way! |
Yeah, I think just linting with |
This should cover the case where plugins are loaded and parallel running is enabled.
It looks like the reason existing parallel tests didn't catch this is that they directly register the checkers on the linter, so they bypass the plugin loading code completely. I have added a test that doesn't actually lint a file — it just loads the dummy plugin into a linter object and verifies the linter can then be pickled. In addition to not doubling-up the content of other tests, the only time this test would need re-working is if parallel running was revamped so heavily as not to relying on pickling, I think — does it sound like something that will suit? |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
added
the
Needs backport
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.
daogilvie
requested review from
cdce8p and
DanielNoord
and removed request for
cdce8p
|
I've modified the test to use a non-pickle-safe module explicitly, and to verify the module cannot be pickled (in case it is made pickle-safe in the future independently). |
|
Ok I am genuinely mortified and confused, sorry, I seem to have caused another failure in parallel tests. Working on it. There are two pre-existing failures in the test_functional file also, I assume they are known. EDIT: Oh, it seems that the set of default plugins loaded is different depending on whether you run the full test suite or just the pickle test in isolation, and catching the error that happens with the specific plugin I chose is actually not enough when the extras are loaded. Addressing now. |
This comment has been minimized.
This comment has been minimized.
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.
|
Oh, huh, actually do you mind if I put myself in the |
I believe Pierre added you already. |
Oh that's great! Thank you again @Pierre-Sassoulas for covering for my laziness 👀 — I might just shorten my name though if that's ok 😄 |
Please do if you like! |
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]>
cdce8p
added
Backported
and removed
Needs backport
Type of Changes
Description
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.
Closes #7635