Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load custom plugins when linting in parallel #8683

Merged

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented May 13, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Load custom plugins when processing in parallel.

The current implementation of multiprocessing launches a pool, and unpickles a PyLinter object that contains all the configuration information. The workers in the pool will not have access to the AstroidManager that had the configuration options (custom plugins, init hooks) applied to it.

The solution here is to just re-register the custom plugins during the initialization of the pool. It's inelegant to do this multiple times, but I'm suggesting not over-optimizing a multiprocessing design that's flawed in other ways. (See #2525.) We can optimize this infelicity out later if it's still even relevant after the overhaul for #2525.

(It's possible some custom code in --init-hook still might not execute, but the main use case for --init-hook is for manipulating sys.path so that custom plugins will work, and they do work now, so I removed the caveat in the docs. No one on the issue board has presented another use case for --init-hook and --jobs.)

Closes #4874
Closes #3232

The MRE in #4874 passes now πŸŽ‰

@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a7 milestone May 13, 2023
@jacobtylerwalls jacobtylerwalls changed the title Load dynamic plugins when linting in parallel Load custom plugins when linting in parallel May 13, 2023
@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Merging #8683 (cb7804c) into main (0d878dd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8683      +/-   ##
==========================================
- Coverage   95.80%   95.80%   -0.01%     
==========================================
  Files         172      172              
  Lines       18301    18300       -1     
==========================================
- Hits        17534    17533       -1     
  Misses        767      767              
Impacted Files Coverage Ξ”
pylint/lint/parallel.py 92.85% <100.00%> (+0.21%) ⬆️
pylint/lint/pylinter.py 96.42% <100.00%> (ΓΈ)

... and 1 file with indirect coverage changes

@github-actions
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit cb7804c

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Great !

There is one known limitation with running checks in parallel as currently
implemented. Since the division of files into worker processes is indeterminate,
checkers that depend on comparing multiple files (e.g. ``cyclic-import``
and ``duplicate-code``) can produce indeterminate results.
Copy link
Member

Choose a reason for hiding this comment

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

We have a map reduce function for that I think. But some are not implemented (they should). For duplicate code I don't even know if it's possible to parralelize. The whole hashmap of lines should be shared.

@@ -30,6 +31,9 @@
from pylint.typing import FileItem
from pylint.utils import LinterStats, ModuleStats

if TYPE_CHECKING:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this helps much? If you are running this test then nodes will have probably been imported somewhere right?

Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't reject change because it's only a small improvments. It's better, let's ship it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, I just prefer this conceptually, when it comes in handy in more substantial cases. We can probably trace this to the opposite views we expressed on #8111 :-)

@jacobtylerwalls
Copy link
Member Author

Thanks for the reviews!

@jacobtylerwalls jacobtylerwalls merged commit 4e48d46 into pylint-dev:main May 14, 2023
34 checks passed
@jacobtylerwalls jacobtylerwalls deleted the multiprocessing-plugins branch May 14, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants