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

Add --workers CLI parameter (fixes #2513) #2514

Merged
merged 2 commits into from Sep 29, 2021

Conversation

FHTMitchell
Copy link
Contributor

@FHTMitchell FHTMitchell commented Sep 27, 2021

Description

Fixes Issue #2513. This does introduce a new CLI parameter but it fixes a pretty core issue. See the issue for more details.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary? -- Unsure if this needs testing.
  • Add new / update outdated documentation? -- I checked this and the --help is automatically inserted into the docs.

@felix-hilden
Copy link
Collaborator

I think you meant to link issue #2513 👍

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

As this approach was okayed in the linked issue, I think we can start polishing this.

src/black/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

I feel and agree this is the way to go as our main dislike for CLI args is around formatting changes.

I feel we could also use more click features to simplify the code + actually show how many workers the default detection would run in your environment in --help

But I won't block on requiring these changes if others disagree.

Comment on lines 324 to 325
type=int,
help="Number of parallel workers [default: os.cpu_count()]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type=int,
help="Number of parallel workers [default: os.cpu_count()]",
default=os.cpu_count(),
type=int,
show_default=True,
help="Number of parallel workers",

Maybe we could just show the cpu_count in the help output ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be confusing from one machine to another and on our documentation. Probably not that big of a deal though, and it is convenient to see the value right away 🤔

src/black/__init__.py Outdated Show resolved Hide resolved
src/black/__init__.py Show resolved Hide resolved
@ichard26 ichard26 linked an issue Sep 28, 2021 that may be closed by this pull request
@FHTMitchell
Copy link
Contributor Author

FHTMitchell commented Sep 29, 2021

I added your changes (use os.cpu_count() as the default and error on non-positive)

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

LGTM - If no one objects I'll merge later today.

@click.option(
"-W",
"--workers",
type=click.IntRange(min=1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice touch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice, didn't know this was also a thing 👌

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

LGTM as well, thanks for the updates!

@click.option(
"-W",
"--workers",
type=click.IntRange(min=1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice, didn't know this was also a thing 👌

@JelleZijlstra JelleZijlstra merged commit 0fd353f into psf:main Sep 29, 2021
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.

Black Freezes in Containers on Nodes with a Large os.cpu_count()
4 participants