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

Avoid hangs on many-core Windows machines #7035

Merged
merged 8 commits into from
Jun 25, 2022

Conversation

randomascii
Copy link
Contributor

@randomascii randomascii commented Jun 24, 2022

Creating a multiprocessing Pool with too many processes can hit
ValueError exceptions or hangs or both. The number that counts as "too
many" depends on the Python version so this change uses 56 as a
guaranteed safe limit.

Note that this only avoids the issue if an explicit jobs count is not
passed in.

| ✓ | 🐛 Bug fix |

Closes #6965

Creating a multiprocessing Pool with too many processes can hit
ValueError exceptions or hangs or both. The number that counts as "too
many" depends on the Python version so this change uses 56 as a
guaranteed safe limit.

Details in issue 6965

Note that this only avoids the issue if an explicit jobs count is not
passed in.
@Pierre-Sassoulas Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons Crash 💥 A bug that makes pylint crash multiprocessing labels Jun 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.0, 2.14.4 Jun 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Jun 24, 2022
Comment on lines 97 to 99

* Fixed an issue where many-core Windows machines (>~60 logical processors) would hang when
using the default jobs count.
Copy link
Member

Choose a reason for hiding this comment

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

We're going to backport that in 2.14.4 :)

@Pierre-Sassoulas
Copy link
Member

Thank you for the research you've done @randomascii, much appreciated !

@randomascii
Copy link
Contributor Author

Thank you for the research you've done @randomascii, much appreciated !

You're welcome. It was interesting, and the discussion on the bug was helpful for me.

@coveralls
Copy link

coveralls commented Jun 24, 2022

Pull Request Test Coverage Report for Build 2559891737

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 49 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 95.323%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/utils.py 3 95.14%
pylint/checkers/typecheck.py 46 95.29%
Totals Coverage Status
Change from base Build 2558044584: -0.02%
Covered Lines: 16653
Relevant Lines: 17470

💛 - Coveralls

pylint/lint/run.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Only need to move the changelog entry to 2.14.4.

Thanks very much for all the work here @randomascii! Much appreciated!

pylint/lint/run.py Outdated Show resolved Hide resolved
@randomascii
Copy link
Contributor Author

I updated 2.15/index.rst and put the same content in 2.14/index.rst - I hope that is as desired.

I also made the requested comment change (I didn't see the button to do it from the pull request until I'd already done it on my machine).

@github-actions

This comment has been minimized.

@DanielNoord DanielNoord removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jun 25, 2022
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

I move the changelog to the 2.14.4 section as that will be the release this will be available in. Should have been clearer about that!

Thanks a lot @randomascii 😄

@github-actions

This comment has been minimized.

@DanielNoord DanielNoord merged commit 8a86414 into pylint-dev:main Jun 25, 2022
@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 7c709da

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Jun 29, 2022
Pierre-Sassoulas pushed a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 29, 2022
Creating a multiprocessing Pool with too many processes can hit
ValueError exceptions or hangs or both. The number that counts as "too
many" depends on the Python version so this change uses 56 as a
guaranteed safe limit.

Details in pylint-dev#6965

Note that this only avoids the issue if an explicit jobs count is not
passed in.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Pierre-Sassoulas pushed a commit that referenced this pull request Jun 29, 2022
Creating a multiprocessing Pool with too many processes can hit
ValueError exceptions or hangs or both. The number that counts as "too
many" depends on the Python version so this change uses 56 as a
guaranteed safe limit.

Details in #6965

Note that this only avoids the issue if an explicit jobs count is not
passed in.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported Crash 💥 A bug that makes pylint crash multiprocessing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint 2.7 hangs on many-core Windows machines
4 participants