spyder-ide / spyder-docs

Documentation for Spyder, the Scientific Python Development Environment

https://docs.spyder-ide.org

MIT License

32 stars 274 forks source link

Use `require_serial` for `sphinx-lint` in `.pre-commit-config.yaml` #353

Closed ezio-melotti closed 9 months ago

ezio-melotti commented 9 months ago

Pull Request

Pull Request Checklist

[x] Read and followed this repo's Contributing Guidelines
[x] Based your PR on the latest version of the correct branch (master or 4.x)
[x] Checked your writing carefully for correct English spelling, grammar, etc
[x] Described your changes and the motivation for them below
[ ] Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

sphinx-lint already uses multiprocessing internally, and this might interfere with precommit, since it will try to use multiple processes too. By setting require_serial we prevent precommit from doing that.

See:

https://github.com/sphinx-contrib/sphinx-lint/pull/95

CAM-Gerlach commented 9 months ago

I'm a little confused—wasn't the purpose of including this in the pre-commit-hooks config in sphinx-contrib/sphinx-lint#95 , released in Sphinx-Lint 0.8.1 which we adopted in #351 , such that each individual project wouldn't need to add this?

Testing locally with time pre-commit run --all sphinx-lint on an old, slow single-threaded by decent multi-threaded with pre-commit 2.20.0 on both master and this branch that are identical except for this setting, I get essentially identical times for running this check, well within the margin of error (3.75 s +/- 0.3 s vs 3.87 s, +/- 0.4 s across 5 trials each), which suggests it has no measurable effect.

Is there something I'm missing here? Thanks!

ezio-melotti commented 9 months ago

Is there something I'm missing here?

Actually I was the one who got confused.

To summarize:

both pre-commit and sphinx-lint use multiple processes
if they both use them at the same time, they interfere with each other, so one of the two should be disabled
for sphinx-lint, this can be achieved by specifying --jobs=1, for pre-commit there is require_serial
both solutions seem to have similar performances on average
by specifying require_serial in .pre-commit-hooks.yaml in the sphinx-lint repo, we are already telling pre-commit not to use multiple processes, and in doing so, we prevent conflicts with projects that explicitly sets a specific number of jobs for sphinx-lint

IOW, this PR is not necessary, and can be closed. Sorry for the noise!

CAM-Gerlach commented 9 months ago

Thanks Ezio!