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

Fix pylint parallel execution #577

Merged

Conversation

koniiiik
Copy link
Contributor

@koniiiik koniiiik commented Jan 12, 2023

Description

This patch prevents closing over the entire ProspectorConfig in the filter generated by make_exclusion_filter to avoid pickling the entire object when pylint forks subprocesses for its parallel execution.

Related Issue

#573

Motivation and Context

Full context explained in #573 (comment)

This branch also includes a slight bump of the lower bound on Python in pyproject.toml. The reason is that allowing Python 3.7.1 locks pylint to versions below 2.14, and pylint 2.13 has an even more catastrophic failure mode than 2.15 – instead of crashing, it livelocks in an endless loop of spawning new subprocesses.

Lifting the Python lower bound to 3.7.2 allows poetry to upgrade pylint to 2.15, which handles the failure state in a slightly more graceful way.

How Has This Been Tested?

Ran pytest on Python 3.10. Also makes it possible for prospector to run successfully on our code base at $DAYJOB.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My change requires a change to the dependencies
  • I have updated the dependencies accordingly
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

pyproject.toml Show resolved Hide resolved
@carlio
Copy link
Member

carlio commented Jan 12, 2023

One minor question but otherwise LGTM, thanks for investigating and finding this!

Another question, is the two.py test file intentionally blank? I wasn't sure.

@koniiiik
Copy link
Contributor Author

Another question, is the two.py test file intentionally blank? I wasn't sure.

Oh, yeah, that's rather arbitrary, I created multiple files right off the bat, because I wasn't sure if parallel execution would fully trigger without them, but I might be able to clean it up.

@carlio
Copy link
Member

carlio commented Jan 12, 2023

Oh, yeah, that's rather arbitrary, I created multiple files right off the bat, because I wasn't sure if parallel execution would fully trigger without them, but I might be able to clean it up.

It's not an issue to leave it, it just seemed an anomaly so wanted to check!

@koniiiik koniiiik force-pushed the fix-pylint-parallel-execution branch from 1e4a180 to e577824 Compare January 12, 2023 14:49
@koniiiik
Copy link
Contributor Author

It's not an issue to leave it, it just seemed an anomaly so wanted to check!

Turns out it's not necessary, so in the interest of avoiding any potential confusion, I cleaned it up.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 1.9.0 milestone Jan 12, 2023
Copy link
Collaborator

@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.

I think upgrading to python 3.7.2 is not that big of a deal, some tool like flake8 already dropped python 3.7. And the EOL for the latest python 3.7 is very soon anyway.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 223f04c into landscapeio:master Jan 12, 2023
@koniiiik koniiiik deleted the fix-pylint-parallel-execution branch January 13, 2023 00:11
rpatterson added a commit to rpatterson/project-structure that referenced this pull request Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants