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

Allow users to provide an explicit process count #78

Merged
merged 7 commits into from
Oct 11, 2023

Conversation

rtobar
Copy link
Contributor

@rtobar rtobar commented Oct 10, 2023

Useful for pre-commit hooks, as read in the discussion in #76, but probably in other situations too.

Instead of always defaulting to using as many processes are CPUs (or no sub-processes if checking less than 8 files), a new -p/--processes flag allows users to customise how many sub-processes will be used to perform the checks. Defaults to the old behavior (i.e., os.cpu_count() processes), caps values on the lower end to 1 automatically, and prevents subprocess spawning if requiring only one process.

sphinxlint/__main__.py Outdated Show resolved Hide resolved
@rtobar rtobar force-pushed the explicit-process-count branch from 0266266 to 0b45b4f Compare October 10, 2023 23:55
@AA-Turner
Copy link
Member

AA-Turner commented Oct 11, 2023

Useful for pre-commit hooks

I agree that this PR has merit on its own, but I'm unsure it would be useful for pre-commit hooks -- the reporting mechanism of sphinx-lint collates, sorts, and reports errors after running; I worry that limiting the parallelism here but enabling pre-commit to parallise produce a preponderance of output with multiple sphinx-lint instances attempting to display reports.

Hence I think the better solution in this specific case is to enforce, or strongly reccomend, the serial mode of operation.

A

Instead of always defaulting to using as many processes are CPUs (or no
sub-processes if checking less than 8 files), a new -j/--jobs flag
allows users to customise how many sub-processes will be used to perform
the checks. Defaults to the old behavior (i.e., os.cpu_count()
processes), caps values on the lower end to 1 automatically, prevents
subprocess spawning if requiring only one process, and aligns with the
sphiinx-build semantics.

Signed-off-by: Rodrigo Tobar <[email protected]>
@rtobar rtobar force-pushed the explicit-process-count branch from 0b45b4f to da8c971 Compare October 11, 2023 00:04
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Please avoid force-pushing when working on a PR, it makes reviewers' lives much harder. In general, commits are sqiash-merged, so history in the PR need not be tidied.

A

sphinxlint/__main__.py Outdated Show resolved Hide resolved
sphinxlint/__main__.py Outdated Show resolved Hide resolved
sphinxlint/__main__.py Outdated Show resolved Hide resolved
rtobar and others added 3 commits October 11, 2023 10:19
@rtobar
Copy link
Contributor Author

rtobar commented Oct 11, 2023

Please avoid force-pushing when working on a PR, it makes reviewers' lives much harder. In general, commits are sqiash-merged, so history in the PR need not be tidied.

Thanks, I hadn't realised that was the policy for this project (different people gravitate towards different processes), but TBH I didn't check if there were contribution guidelines either. I've applied your suggestions now as-is since they seemed more than reasonable.

@hugovk
Copy link
Collaborator

hugovk commented Oct 11, 2023

"No force pushes" is policy for https://github.com/python/ repos, but we don't actually have a policy either way for this repo/org.

And we have all merge options available here:

image

sphinxlint/__main__.py Outdated Show resolved Hide resolved
sphinxlint/__main__.py Outdated Show resolved Hide resolved
sphinxlint/__main__.py Outdated Show resolved Hide resolved
Comment on lines 127 to 129
help="Run in parallle with N processes, defaults to 'auto', "
"which sets N to the number of logical CPUs."
"Values <= 1 are all considered 1.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same behaviour as sphinx-build for N <= 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't really know, but I thought it'd be the less error prone.

@hugovk
Copy link
Collaborator

hugovk commented Oct 11, 2023

Useful for pre-commit hooks, as read in the discussion in #76, but probably in other situations too.

I agree with Adam that it doesn't sound so useful for pre-commit hooks. Can you suggest some of the other situations?

rtobar and others added 3 commits October 11, 2023 21:47
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
@rtobar
Copy link
Contributor Author

rtobar commented Oct 11, 2023

Useful for pre-commit hooks, as read in the discussion in #76, but probably in other situations too.

I agree with Adam that it doesn't sound so useful for pre-commit hooks. Can you suggest some of the other situations?

Benchmarking and profiling, for one, which currently is limited to having to do it with up to 8 files, lest you spawn subprocesses. But giving full flexibility to users in general for situations where they might wnat to use a different solution between 1 and cpu-count. I admit it might not be heavily used, but I don't think it's totally unworthy to have it.

Copy link
Collaborator

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks!

@hugovk hugovk merged commit 1e8c3f9 into sphinx-contrib:main Oct 11, 2023
@rtobar rtobar deleted the explicit-process-count branch October 11, 2023 15:20
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.

3 participants