Repository navigation
Conversation
…mmit config change Signed-off-by: Tahsin Tunan <tahsintunan@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
khluu
left a comment
khluu
left a comment
There was a problem hiding this comment.
Thanks, this is a good fix. The 502/503 failures from these hooks have been painful. I checked how it works and it holds up: all five hooks share the pip-compile id, so SKIP and pre-commit run pip-compile cover all of them. On PRs, HEAD^1 of the merge ref is the base, so the diff is right. And every -r include sits inside the hooks' files: filters. Your own run shows the skip path working.
One gap I'd like closed before merging: uv pip compile also reads [tool.uv] from pyproject.toml, which this doesn't account for. Details are inline. The other comments are smaller.
Could you also link runs where the hooks actually execute? For example, a throwaway commit touching requirements/common.txt and one touching .pre-commit-config.yaml. The current run only shows the skip path, and a broken diff would look the same ("no files to check").
| SKIP: "" | ||
| run: | | ||
| if git diff --quiet HEAD^1 HEAD -- .pre-commit-config.yaml; then | ||
| pre-commit run pip-compile --hook-stage manual --show-diff-on-failure --color=always --from-ref HEAD^1 --to-ref HEAD |
There was a problem hiding this comment.
On push to main, HEAD^1 only covers the last commit, so if several commits land in one push, the earlier ones aren't checked. Using ${{ github.event.before }} as the from-ref for push events (with a deeper fetch) would cover the whole push. Low priority since we squash-merge.
There was a problem hiding this comment.
Yeah, decided to leave this out. HEAD^1 covers the whole push in practice, since every merge is a squash.
…and even if other hooks fail Signed-off-by: Tahsin Tunan <tahsintunan@gmail.com>
Signed-off-by: Tahsin Tunan <tahsintunan@gmail.com>
Done.
Description updated with throwaway test runs. |
| - run: python -m pip install pre-commit | ||
| - name: Re-resolve all lockfiles (retries index errors) | ||
| run: | | ||
| for attempt in 1 2 3; do | ||
| if pre-commit run pip-compile --hook-stage manual --all-files --show-diff-on-failure --color=always; then | ||
| exit 0 | ||
| fi | ||
| # A failed attempt may have rewritten the lockfiles; reset them so a | ||
| # retry cannot pass on files the previous attempt fixed up. | ||
| git checkout -- requirements | ||
| if [ "$attempt" -lt 3 ]; then | ||
| sleep 60 | ||
| fi | ||
| done | ||
| exit 1 |
There was a problem hiding this comment.
Same comment here, why not use the hook that already exists and specify only pip-compile?
There was a problem hiding this comment.
The loop is for retries. It reruns on a PyPI 502 and resets the lockfiles between attempts, so a nightly blip doesn't go red. The action has no built-in retry, so I kept the loop.
Signed-off-by: Tahsin Tunan <tahsintunan@gmail.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Tahsin Tunan <tahsintunan@gmail.com>
khluu
left a comment
khluu
left a comment
There was a problem hiding this comment.
Kevin's review agent: approve for CI changes only
Summary
--all-files, so the 5pip-compilehooks re-resolve every lockfile against PyPI anddownload.pytorch.orgon every run, even when nothing inrequirements/changed. When those indexes return a 502/503, pre-commit fails for changes that have nothing to do with dependencies.files.pythonhosted.org(Aug 15–19), 57 PR pre-commit runs failed inside these hooks. Main went red 18 times from May to Sep for the same reason (see Examples).pip-compilein the--all-filesrun, and run it separately on only the files the commit changed. The hooks' existingfiles:filters then decide whether they need to run, so they only touch the network when requirements change..pre-commit-config.yamlchanged, they still run on all files, because a uv bump or a hook-args edit can change the lockfiles without touchingrequirements/(as in Updatepre-commithook versions #26591).Examples
pip-compile-xpu, 503 from download.pytorch.org: run 28826483690pip-compile-xpu, 503 from download.pytorch.org: run 28834611183pip-compile-xpu, 503 from download.pytorch.org: run 28840983587pip-compile-xpu, 503 from download.pytorch.org: run 28901713650pip-compile-xpu, 503 from download.pytorch.org: run 28977448720pip-compile-cpu, 502 from files.pythonhosted.org: run 31888326977pip-compile-xpuandpip-compile-docs, 502 from files.pythonhosted.org: run 31935561992pip-compile-rocm, 502 from files.pythonhosted.org: run 31938637047pip-compile,pip-compile-rocmandpip-compile-xpu, 502 from files.pythonhosted.org: run 32012766971pip-compile,pip-compile-xpu,pip-compile-cpuandpip-compile-docs, 502 from files.pythonhosted.org: run 32021711554pip-compile-cpu, 502 from files.pythonhosted.org: run 32047339080pip-compile-cpu, 502 from files.pythonhosted.org: run 32073834879pip-compileandpip-compile-rocm, 502 from files.pythonhosted.org: run 32170751177pip-compile-cpu, 502 from files.pythonhosted.org: run 32172635973pip-compile-xpu, 502 from files.pythonhosted.org: run 32187315071pip-compile-cpu, 502 from files.pythonhosted.org: run 32194198346pip-compileandpip-compile-xpu, 502 from files.pythonhosted.org: run 32208700153pip-compile-xpu, 503 from download.pytorch.org: run 36288536065Tests
Real GitHub Actions runs on my fork. "Network blocked" means a dead proxy on the pip-compile step to simulate a 502 burst. Look at the
pip-compile (only if its inputs changed)step: the jobs themselves are red because actionlint flags a line in the test harness, or because pre-commit hit a 504 while installing hook environments.--all-filesbehavior with network blocked: all 5 fail with "Connection refused" (run)requirements/test/xpu.in: onlypip-compile-xpuruns and fails with+toolz(PR, push)pip-compile-xpupasses (run)[tool.uv]-only change inpyproject.toml: cuda, rocm, xpu, cpu fail with+openai-harmony(PR, push)pyproject.tomlchange outside[tool.uv]: all 5 run and pass (run).pre-commit-config.yamlchange: all 5 run and pass (run)+toolz(run)