Add option to limit the number of mp workers

BlankSpruce / gersemi

A formatter to make your CMake code the real treasure

Mozilla Public License 2.0

156 stars 8 forks source link

Add option to limit the number of mp workers #14

Closed ThadHouse closed 10 months ago

ThadHouse commented 10 months ago

Due to https://github.com/python/cpython/issues/89240, any use of mp.Pool with a number of workers greater than 60 fails. This means that by using cpu_count(), any system with more than 60 logical cores will crash when attempting to run.

Solve this by adding a flag to allow limiting the number of workers for users with systems with that many cores.

BlankSpruce commented 10 months ago

I appreciate the attempt. I've got two questions though:

I acknowledge that due to that referred issue number of workers should be limited. Wouldn't it be enough to provide such limitation just in the code?
Assuming that you have a good case for number of workers to be manually tuned by the user, would it be ok with you if I introduced -j/--jobs flag similar to how tools like make and ninja do it?

ThadHouse commented 10 months ago

Limiting in code to a maximum of 60 would be fine. Thats actually what our project does. I just didn't know which you'd prefer.

I'm fine with making the flags jobs instead of workers as well. Not set on any name. But I don't have any compelling reason not to just limit the number of runners and be done with it.

BlankSpruce commented 10 months ago

I prefer to not introduce new flags or options unless they accomplish something user might be interested in to have a concious choice. I'll research the topic further and probably today, as in CET zone, I'll release a new version with that change.

ThadHouse commented 10 months ago

@BlankSpruce it needs to be 60, not 61. You’re allowed 63 handles, and mp itself takes 3 of them. 61 will still fail. The issue says 61, but I think one more has been added since then which drops it to 60.

BlankSpruce commented 10 months ago

Indeed, python bug tracker also mentions it here: https://bugs.python.org/issue26903 as well as black's solution uses 60: https://github.com/psf/black/blob/main/src/black/concurrency.py#L88

BlankSpruce commented 10 months ago

https://github.com/BlankSpruce/gersemi/releases/tag/0.11.0 Thank you and enjoy.