Repository navigation
ci: use pre-commit for documentation Lychee checks - #3142
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
rwgk
added
enhancement
ChatGPT Codex Connector
|
/ok to test bdb3299 |
|
/ok to test f87a871 |
|
f87a871 to
9a24782
Compare
|
/ok to test 9a24782 |
|
/ok to test |
rwgk
changed the title
rwgk
added
the
PR review get-together
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Feature Merge Risk: ⚪ Minimal · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
1993dd35-a388-4bbb-8736-2bd9cca57c52
📒 Files selected for processing: 17
.github/actions/check-doc-links/action.yml.github/dependabot.yml.github/workflows/build-docs.yml.github/workflows/lychee.yml.github/workflows/pre-commit.yml.gitignore.pre-commit-config.yamlci/README.mdci/tools/lychee_cache.pyci/tools/retry_lychee.pyci/tools/run_lychee.pyci/tools/run_lychee_precommit.pyci/tools/tests/test_lychee_cache.pyci/tools/tests/test_retry_lychee.pyci/tools/tests/test_run_lychee.pyci/tools/tests/test_run_lychee_precommit.pylychee.toml
💤 Files with no reviewable changes: 2
- .github/dependabot.yml
- .github/actions/check-doc-links/action.yml
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| cache = tmp_path / "rendered cache" | ||
| monkeypatch.setenv("LYCHEE_CACHE_DIR", str(cache)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
suggestion: The expected cache path in this test is not resolved, so the test can fail on macOS and some Windows hosts.
run_lychee.main resolves LYCHEE_CACHE_DIR with Path(...).resolve() (run_lychee.py Line 41). This resolved path is used as cwd and as the parent of inputs.txt. The test compares both values against the unresolved tmp_path / "rendered cache". On macOS, tmp_path sits under the /var → /private/var symlink, so resolve() returns a different path. In that case the run.assert_called_once_with(...) check fails, and --files-from gets a different string than the test expects. Linux CI does not show the problem, but local runs on macOS fail. The fix is to resolve the expected path in the test.
--- "a/ci/tools/tests/test_run_lychee.py"
+++ "b/ci/tools/tests/test_run_lychee.py"
@@ -51,8 +51,8 @@
(docs / "guide.html").write_text('<h1 id="section">Guide</h1>')
(docs / "_static").mkdir()
(docs / "_static/theme.html").write_text("ignored theme")
- cache = tmp_path / "rendered cache"
+ cache = (tmp_path / "rendered cache").resolve()
monkeypatch.setenv("LYCHEE_CACHE_DIR", str(cache))
monkeypatch.setattr(run_lychee, "ROOT", tmp_path)
monkeypatch.delenv("LYCHEE_REPORT_DIR", raising=False)
with (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cache = tmp_path / "rendered cache" | |
| monkeypatch.setenv("LYCHEE_CACHE_DIR", str(cache)) | |
| cache = (tmp_path / "rendered cache").resolve() | |
| monkeypatch.setenv("LYCHEE_CACHE_DIR", str(cache)) |
|
Nice work consolidating link checking onto the pre-commit hooks. One behavior change I'd like to flag before this lands, plus an optional follow-up. Request: give the authored gate the same 429 tolerance as the rendered path Moving authored link checks into That's asymmetric with the rendered docs build, which already tolerates up to 10 distinct 429s through
- name: Check authored documentation links
env:
GITHUB_TOKEN: ${{ github.token }}
run: >-
python ci/tools/run_lychee_precommit.py authored
--policy check --max-attempts 3
--report "${RUNNER_TEMP}/lychee-reports/lychee-authored.json"Net behavior would match the rendered path: permanent failures (404, DNS) still fail; up to 10 distinct 429s warn and pass; more than that still fails. The Optional follow-up: a maintainer bypass for the rare non-429 case For the narrow residual (e.g. >10 429s, or a site returning 503/403 at merge time), a maintainer escape hatch would help. A |
d7d7bd8 to
7625aad
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · important: Apply the authored Lychee policy in the PR gate · pre-commit.yml:165-168
.github/workflows/pre-commit.yml:165-168
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winimportant: Apply the authored Lychee policy in the PR gate
pre-commit run --all-filesruns the authoredlycheehook with its raw exit code. The preceding cache warm-up does not make this step tolerant of an unresolved HTTP 429. Therefore, ten or fewer distinct 429 responses can still fail Linux and Windows PR jobs.Skip only
lycheein this strict step, then run the authored check through the existingcheckpolicy. Permanent link failures and more than ten distinct 429 responses will still fail.Suggested fix
- name: Run pre-commit env: GITHUB_TOKEN: ${{ github.token }} + SKIP: check-precommit-installed,lychee run: pre-commit run --all-files --show-diff-on-failure + + - name: Check authored links + env: + GITHUB_TOKEN: ${{ github.token }} + run: python ci/tools/run_lychee_precommit.py authored --policy check --max-attempts 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
9fa469c8-f3e9-47df-9a9b-dc271999308f
📒 Files selected for processing: 1
ci/tools/tests/test_run_lychee.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
I adopted @juenglin 's commit 1d7b804, based on this codex review: I favor Ralf’s reduction. I found no blocking correctness issue, and it preserves the main design of #3142. His branch is one commit directly on the current #3142 head, so this is a focused simplification. What improves
The important safeguards remain: collecting every pre-commit chunk, checking report/exit-code consistency, rejecting malformed or empty reports, retrying only transient failures, and keeping authored/rendered caches separate. The report collector itself is unchanged. Changes worth accepting explicitlyNightly becomes the sole URL-cache producer. Default-branch pre-commit runs stop publishing authored snapshots. This simplifies ownership, while removing a backup producer during nightly outages. Given the three-day lifetime and working cold-cache fallback, I think that is a reasonable tradeoff. Proposed cache policy. Two smaller changes also seem reasonable:
The new cache namespace will require an initial cold generation. Testing and dependent PRsThe 96 retained focused tests pass. Most deleted tests cover removed helpers or duplicate policy cases; I found no coverage blocker. However, the new composite action still needs hosted Linux/Windows execution and a nightly producer/consumer rehearsal. Adopting it requires deliberate adjustments downstream:
My recommendation: adopt the reduction in #3142 before merging, accepting nightly-only cache publication explicitly, then validate and update the dependent PRs. |
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Reduce the code behind the pre-commit-based link checks without changing which checks run, how they fail, or how results are cached and retried. - Replace ci/tools/lychee_cache.py and its tests with a small composite action, .github/actions/lychee-links. It hashes the policy files with hashFiles(), restores and publishes the URL cache, runs the driver, and uploads reports, so the pre-commit, nightly, and docs-build workflows each make one call instead of repeating five to eight steps. - Drop LYCHEE_CACHE_DIR. The rendered hook always uses the gitignored .lychee-rendered directory. - Make nightly the only producer of the shared baselines. Linux pre-commit runs on the default branch no longer publish the authored cache. - Remove retry branches for native exit codes other than 0 and 2. The driver raises on those before retrying, so they could not run. - Fold the two helpers still in use from prepare_lychee_inputs.py into run_lychee.py and delete the module, whose authored-file selection has been done by the hook configuration since the Lychee action was removed. - Trim tests that duplicated each other or exercised removed paths, and shorten the README section. Signed-off-by: Ralf Juengling <rjuengling@nvidia.com> Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
1d7b804 to
411b3a7
Compare
|
/ok to test 411b3a7 |
rwgk
removed
the
PR review get-together
|
@rwgk Thanks for adopting the trim. I re-checked the description against Suggested description edits (no CI impact)
Cleanup:
|
2 participants
Highlights
Bring documentation Lychee checks into the same pre-commit workflow used during development, and catch authored-link failures during ordinary PR updates.
lychee-bin==0.24.2dependency. Replacelycheeverse/lychee-action, its separate binary pin, and the obsolete upstream-hook Dependabot exception.Ralf Juengling's simplification is incorporated: one shared composite action handles URL-cache restoration, hook execution, report uploads, and nightly cache publication for all three workflows. This removes repeated workflow steps, redundant Python helpers, and unreachable retry paths.
Tradeoffs
Authored links are checked more frequently because pre-commit runs on pushes that may not trigger full CI. Cache reuse should keep the usual overhead modest, but new URLs, expired or missing caches, and slow or failing sites still cost requests and time. Each pre-commit job can make up to three Lychee warm-up passes, followed by one strict full pre-commit run; successful external checks are reused between passes. PRs consume the shared baseline without publishing their own snapshots, so newly introduced URLs can be checked again on later pushes. URLs newly added to
mainalso remain uncached in the shared baseline until the next nightly refresh.Successful external checks expire after three days rather than one, providing breathing room when nightly refreshes fail or are missed. Nightly still checks links afresh every day, but there is no automatic backup cache producer if it fails. PR checks run cold once those cached successes expire. When fresh results are unavailable, the tradeoff is that a cached success can conceal a newly broken external link for up to 72 hours.
The new
hashFiles-based cache keys do not match existing baselines. After merging, PR checks initially run cold until the first nightly refresh publishes the new baselines.Authored and rendered URL caches remain separate even when their URLs overlap. Authored checks leave fragments unchecked; rendered checks validate them. Lychee's cache does not record which validation occurred, so sharing an authored success can falsely accept a missing anchor. This was reproduced with pinned Lychee 0.24.2 against a local server. Keeping the caches separate prevents that false pass, at the cost of some duplicate requests.
How it works
In the documentation-build job, rendered link checks now use Python 3.14 supplied by
actions/setup-python, replacing the conda interpreter for that step. The documentation itself is still built in conda.Repository URLs retain their existing targets in this PR. Maintainer control of HTTP 429 failures is a separate follow-up in #3155, followed by checkout-aware repository links in #3143 and the source-root rename in #3147. Standalone CI-tool unit tests remain in nightly CI.
Implementation details and suggested review order
Retries and diagnostics
The CI driver invokes the actual configured hook and collects every native invocation's JSON report. Pre-commit may split long input lists, so each invocation writes its own report and exit status before the driver validates and combines them.
Additional hook attempts run only when every remaining failure is recognized as transient HTTP trouble: timeouts, 408, 429, or 5xx. CI allows three total attempts, with 60/120-second cooldowns. Permanent failures stop additional attempts. The full pre-commit suite runs only once, so formatter changes remain failures.
Missing, malformed, or inconsistent reports fail explicitly, as does a combined scan that checks no links. Per-attempt reports and a final summary are retained as CI artifacts for seven days.
Shared caches
Nightly refreshes are the only producers of the shared authored and rendered baselines. PR and ordinary default-branch runs restore them without publishing replacements. Only successful external checks are persisted, with their original timestamps and a three-day expiration. Reusing a result does not extend its lifetime. Local filesystem targets are checked afresh. A missing baseline starts a normal cold check.
URL results can be reused across operating systems. Installed pre-commit environments retain their separate OS/Python-specific caches. A local composite action restores the URL cache, invokes the configured hook through the CI driver, uploads reports, and publishes a new baseline when requested by nightly. It derives compatible cache names with GitHub Actions
hashFiles, covering the hook configuration, native wheel pin, checker configuration, and implementation files. Hashing whole files is conservative: even a comment or display-name change in a listed file can invalidate the cache.Suggested review order
The CI link-checking guide collects the maintenance and failure policies in one place. Focused tests cover report aggregation, retry decisions, and rendered invocation.
Validation
After incorporating the shared composite action and rebasing on current
main, this revision passes all 203 CI-tool tests and 36 subtests, pluspre-commit run --all-fileslocally.Full CI requested for this revision; the earlier hosted results below predate the composite-action refactoring.
Earlier validation of this implementation
Before this rebase and the final review refinements:
Checklist