Repository navigation
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. |
|
/ok to test 8934e74 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Feature Merge Risk: ⚪ Minimal · up to
|
|
Has the lychee pre-commit step been tested on both Windows and Linux? |
juenglin
left a comment
There was a problem hiding this comment.
Nice approach. The --remap quoting is right: the embedded \" survive eval in the lychee-action entrypoint and shlex.split in retry_lychee.py, so both passes give lychee one "pattern replacement" argument per --remap. The 16 new tests pass for me locally. Nothing blocking, but a few things to consider:
1. Windows pre-commit (follow-up to @juenglin's question)
CI skips lychee in pre-commit on both Linux and Windows (SKIP: lychee), so nothing exercises --preprocess=./ci/tools/preprocess_repo_links.py on Windows. Lychee launches that path directly rather than through a shell, so it depends on the shebang and exec bit. Windows doesn't honor shebangs. I haven't verified how lychee handles this on Windows. Has anyone run the hook there? If Windows local runs are supported, it may need an explicit interpreter or a note in ci/README.md.
2. Backport-branch PRs
check-doc-links runs for every pull-request/N ref, not just PRs against main. For a PR against 12.9.x, 13.4.x, 12.2.x or 11.8.x, HEAD is the backport commit. A tree/main/<path> link is then checked against that branch, so a path that exists only on main (for example cuda_bindings_13 after #3116) would fail. A path that was removed from main but still exists on the branch would pass. I haven't checked whether those branches contain such links today. Is it worth gating the remap on the base branch, or documenting the behavior in ci/README.md?
3. CI and local patterns differ
CI uses an anchored regex (^https://github[.]com/...). preprocess_repo_links.py does an unanchored str.replace, so it would also rewrite the prefix inside another URL, such as a redirect query string. This doesn't happen in today's docs. It might be worth a comment, or defining the prefixes once so the two paths can't drift. (I considered using --remap locally too, but pre-commit args are static and the checkout root isn't, so --preprocess is the right mechanism.)
4. Smaller notes
- The remapped URLs embed the commit SHA, and lychee keys its cache on the remapped URI. So the nightly baseline cache can't serve these links on PRs, and about 21 authored links (more in rendered HTML) are refetched on every run. That is probably fine for the rate-limit budget, but it's worth knowing.
test_repo_link_check_action.pyandtest_preprocess_repo_links.pyrun only in the nightlyci/tools/testsjob. Onlytest_compute_ci_plan.pyruns per PR. Consider adding the action test toci.yml, since it's the one most likely to catch breakage in the remap step.- The description's "missing targets still fail" and "17 regression probes" aren't backed by anything in the diff. If those probes are cheap to keep, a Lychee-driven fixture test would cover them.
Tracking a codex analysis & recommendations at this stageI fetched the latest feedback and read the Slack thread. I agree with adding limited Lychee coverage to pre-commit CI on Linux and Windows. The review exposes a Windows integration risk that I would address before merging #3118. Windows: the history mattersMike’s original PR was #2327, “Make pre-commit work on Windows”. Its initial description says Lychee does not run on Windows, but the discussion refined that conclusion: Lychee itself supports Windows, and Mike confirmed that the installed Git hook works. Git Bash was also documented as a supported approach. That gives us existing Windows behavior to preserve. PR #3118 introduces a separate problem: Lychee launches the Python preprocessor directly, without an interpreter or shell. The pinned implementation uses My source-based conclusion is that the new preprocessing command is likely broken on Windows, including when the outer hook starts successfully through Git Bash. I have not reproduced this on Windows. Our earlier validation established Linux behavior. Windows CI skipped Lychee, and the Python tests launch the helper through The small CI check I recommendAdd a separate step to the existing Linux and Windows pre-commit jobs that invokes the actual configured Lychee hook, with Lychee removed from Generate tiny Markdown and reStructuredText fixtures during the job, and verify:
Generating fixtures avoids committing intentionally broken links that nightly checks would discover. With no external links, this should be quick and deterministic after installation. I would also run the new Python tests on every PR. Currently they run automatically only in the nightly CI-tools suite. Mike’s follow-up #2729 adds another useful lesson: cached environments hid an installation failure. Changes to the hook’s installation or pin should therefore receive a fresh-install check too. A promising implementation simplificationI would investigate a small Python hook wrapper that computes the checkout path and passes dynamic, anchored That could resolve both the Windows subprocess problem and the reviewer’s concern about unanchored text replacement. The official Would the nightly cache make broader coverage quick?It should help substantially with unchanged external URLs, but reuse needs explicit wiring. The current pre-commit jobs restore hook environments; they do not restore the nightly Lychee response cache. A broader Linux hook run could reuse the nightly authored cache. Windows reuse would require additional cache portability work. There are also two expected limits:
For #3118, I favor the small fixture check on both platforms. Broader cached documentation coverage can follow without making this fix depend on external-site reliability. The remaining feedbackRalf’s review supports the approach and raises useful refinements:
My recommended scope for #3118 is: fix Windows invocation, align URL matching, add small real-hook checks on both platforms, enable the focused Python tests per PR, and clarify branch scope. That keeps the work focused while closing the gaps identified by review. |
|
/ok to test cc6fa67 |
Rename the CUDA 13 bindings source root and update its build, test, CI, documentation, and tooling paths. This is the same file tree as the validated review branch, with linear history above PR #3118. Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Rename the CUDA 13 bindings source root and update its build, test, CI, documentation, and tooling paths. This is the same file tree as the validated review branch, with linear history above PR #3118. Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
cc6fa67 to
200be8b
Compare
0bed5a9 to
3019f24
Compare
5d7f177 to
de7c722
Compare
Build on the portable native Lychee hook to remap canonical main source links to the checkout or exact CI commit. Preserve source URLs, check missing targets, and retain the existing Linux/Windows hook smoke coverage and rendered-fragment policy. Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
de7c722 to
fad2ab1
Compare
Lychee currently checks repository links against remote
main. When a PR adds or moves a path and updates documentation to link to its new location onmain, that target does not exist yet. This creates a chicken-and-egg problem: the link check needs the PR to merge before it can pass, while the PR needs a passing link check before merge.For example, #3116 renames
cuda_bindings/tocuda_bindings_13/. Its intended repository link is:That link should be checked against the proposed directory layout before it reaches
main.Solution
Builds on #3135, which extracts the native hook installation, launcher, and Windows contributor guidance for separate review.
Validate these repository links against the checkout being tested, while keeping the intended
mainURLs in source files and published documentation.lycheeverse/lychee-actiondirectly, using Lychee's native--remapto change the validation target to the actual checked-out commit in the CI repository. Authored and rendered documentation checks, including retries, use the same remapping. GitHub's file and directory views are preserved, so rendered checks retain their existing fragment validation.--remaprules for the working tree. This also covers uncommitted or unpushed paths. Source files are not rewritten, and local checks retain their existing policy of not checking fragments. CI exercises the configured hook against bounded local fixtures on Linux and Windows; full documentation link scans run through the dedicated CI action.For the example above, CI checks
tree/<checked-commit>/cuda_bindings_13; pre-commit checks the checkout'scuda_bindings_13/directory. Missing targets still fail.Scope
Remapping applies only to canonical
https://github.com/NVIDIA/cuda-python/blob/main/andtree/main/links. Links to tags, explicit commits, other branches, and other repositories keep their existing targets.These rules validate the proposed
maintree. Maintenance branches do not currently contain this action; adopting it there requires an explicit policy for links that intentionally point tomainrather than the maintenance checkout.This PR prepares the link-checking infrastructure for #3116. The source-directory rename remains in that separate PR.
Validation
After extraction: 246 CI-tool tests plus 36 subtests, full pre-commit, and the installed-hook smoke checks pass locally. Hosted CI is deferred for web review; the rewritten commit includes
[skip ci].Earlier validation before this extraction: