Repository navigation
Conversation
github-actions
Bot
added
CI/CD
leofang
added
the
PR review get-together
|
btw IIRC @carterbox moved nvmath-python to use reno. My impression (Daniel can correct me if I am mistaken) is that there's mixed feeling. The time goes from "making sure we don't miss anything" to "fighting against the tooling to get the last mile." |
|
mdboom
removed
the
PR review get-together
|
Yes, nvmath currently uses reno. Yes, our sentiment is mixed, but not because reno is a bad choice per se. The main functionality which is release note aggregation works well, but this functionality is not unique to reno. I was really excited to have fully autogenerated release notes in our documentation, but when trying to get our documentation out the door we had unexpected difficulty getting the documentation rendered correctly. The root of problem is that our development workflow doesn't match OpenStack's strict branching/tagging model, so the git-awareness wasn't the convenient feature that I wanted it to be. There are two issues specifically that we had/have:
I still have a month to decide whether we're going to try again with reno or if we will switch to towncrier, which is not git-aware. |
|
Thanks, Daniel! Yeah git-awareness is no longer a useful feature to me. For example our CI design was forced to change because setuptools-scm requires it, and I really don't like it especially for useless CI re-runs that it must trigger at tagging time (to generate the correct version). I like automation tools but I really don't want them to be more opinionated than I am (and I am already super opinionated) 🙂 @mdboom is this something we want to push through quickly? I see the PR Review Together label was removed. |
mdboom
added
the
PR review get-together
To be clear, this PR doesn't use reno -- it uses a reno-like approach. The behavior we need is different enough that it was fewer lines of more straightforward code to implement from scratch than it was to workaround reno's differences.
This is all in service of being able to do proper patch release management (bugfixes are backported to maintenance branches, and patch releases are made from there to avoid patch releases containing new, half-baked work). That's all very git/branch aware, and having the release notes managed automatically makes backporting much lighter weight and easier to manage.
Yes, but the old approach was full of bugs and pitfalls, IMHO. It was possible to release something under any name we wanted, with no way to reverse engineer the tag that created it.
No need to rush -- I thought that label was auto-assigned so I removed it. But I'm happy to discuss in the meeting. |
There was a problem hiding this comment.
Q: Is this file used during doc build or CI? If so, let's move it to ci/tools/ or ci/.
There was a problem hiding this comment.
No, this file is used for local development when writing a note.
📝 Summary
Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · 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: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
91e12037-92b8-425e-82f7-821421b86bff
📒 Files selected for processing: 98
.github/PULL_REQUEST_TEMPLATE.md.github/RELEASE-core.md.github/actions/fetch-release-history/action.yml.github/workflows/build-docs.yml.github/workflows/ci-nightly.yml.github/workflows/pr-metadata-check.yml.github/workflows/release-cuda-pathfinder.yml.github/workflows/release.yml.pre-commit-config.yamlCONTRIBUTING.mdci/tools/check_pr_release_notes.pyci/tools/check_release_notes.pyci/tools/lint_release_notes.pyci/tools/tests/requirements.txtci/tools/tests/test_add_note.pyci/tools/tests/test_check_pr_release_notes.pyci/tools/tests/test_check_release_notes.pyci/tools/tests/test_lint_release_notes.pyci/tools/tests/test_pixi_lockfile_freshness_workflow.pyci/tools/tests/test_release_date.pyci/tools/tests/test_release_notes.pycuda_bindings/AGENTS.mdcuda_bindings/docs/source/conf.pycuda_bindings/docs/source/release.rstcuda_bindings/docs/source/release/.gitignorecuda_bindings/releasenotes/feature-cuda-header-version-check-c8eaa96d6d5ad554.rstcuda_bindings/releasenotes/feature-faster-instance-creation-ef8fc4969e39029f.rstcuda_bindings/releasenotes/feature-full-ctk-docstrings-d590a2c344c1e362.rstcuda_bindings/releasenotes/feature-none-for-buffer-pointers-51f32eceb1de8bc2.rstcuda_bindings/releasenotes/feature-nvrtc-v2-experimental-d5df4ca8d6654ae8.rstcuda_bindings/releasenotes/feature-struct-wrappers-direct-f525a3c7ed913e3a.rstcuda_bindings/releasenotes/fix-destructor-use-after-free-9ec9fa7be805904f.rstcuda_bindings/releasenotes/fix-docstring-corrections-d8ff2a3c627df05a.rstcuda_bindings/releasenotes/fix-launch-host-func-unchanged-b4e17c0a92d3586f.rstcuda_bindings/releasenotes/fix-nvml-device-get-samples-e9a7a9c65a48d5db.rstcuda_bindings/releasenotes/fix-nvml-empty-instance-arrays-c95161364856ba28.rstcuda_bindings/releasenotes/fix-nvml-vgpu-getters-9e335e78aaa033d8.rstcuda_bindings/releasenotes/fix-self-assignment-crash-ad099eeee3424651.rstcuda_bindings/releasenotes/issue-pip-upgrade-from-old-versions-0cef57de3b6140bd.rstcuda_bindings/releasenotes/issue-wsl-process-name-df799a038280be7e.rstcuda_bindings/releasenotes/prelude-python-310-dropped-5a1f0c7e2b9d4a63.rstcuda_bindings/releasenotes/upgrade-enum-member-renames-e611ece8f9177c5b.rstcuda_bindings/releasenotes/upgrade-nvml-nvlink-firmware-info-9724bd9a0d08fb5a.rstcuda_bindings/releasenotes/upgrade-nvml-vgpu-process-utilization-5e4d031c0f7a6501.rstcuda_bindings/releasenotes/upgrade-nvml-vgpu-utilization-4ac700ba248397ac.rstcuda_bindings/releasenotes/upgrade-nvvm-none-raises-0c0349a51ae2eb38.rstcuda_core/AGENTS.mdcuda_core/docs/source/conf.pycuda_core/docs/source/release/.gitignorecuda_core/docs/source/release/1.3.0-notes.rstcuda_core/releasenotes/feature-cuda-error-top-level-b1aa0da2621e49f6.rstcuda_core/releasenotes/feature-cuda-warning-7623597db0b2e2d6.rstcuda_core/releasenotes/feature-graph-getitem-overloads-d39c63769136be19.rstcuda_core/releasenotes/feature-linker-incremental-5d1fe2b0a24cee39.rstcuda_core/releasenotes/feature-managed-buffer-last-prefetch-location-96cc2bedbdaf0c87.rstcuda_core/releasenotes/feature-virtualmemorybuffer-class-5baa60b7446e63aa.rstcuda_core/releasenotes/feature-virtualmemoryresource-ipc-e60c4a8d3b17f259.rstcuda_core/releasenotes/fix-context-restore-failure-note-3bdf84a0cb1ef5bd.rstcuda_core/releasenotes/fix-device-methods-bound-context-68680878c62dbe17.rstcuda_core/releasenotes/fix-dlpack-preserve-exception-96ac0894ec880fda.rstcuda_core/releasenotes/fix-dmr-close-revokes-peer-access-c6fdad418c6a673b.rstcuda_core/releasenotes/fix-dmr-default-pool-in-capture-51f3baf092d23a79.rstcuda_core/releasenotes/fix-graph-builder-invalidated-capture-053511cf302cb705.rstcuda_core/releasenotes/fix-graph-node-discard-noop-2b6c4d6601993078.rstcuda_core/releasenotes/fix-graph-node-update-context-restore-15c7910882ae0a1b.rstcuda_core/releasenotes/fix-graph-rollback-failure-note-40598aa74bf43efc.rstcuda_core/releasenotes/fix-legacy-pinned-device-id-0cdbd4699d5d3806.rstcuda_core/releasenotes/fix-memcpy-node-update-captured-8f2b6d1e4c07a395.rstcuda_core/releasenotes/fix-orin-uuid-without-prefix-c11eb4c949ed67f2.rstcuda_core/releasenotes/fix-pch-heap-retry-typeerror-3a9d5e7c1b04f862.rstcuda_core/releasenotes/fix-program-cycle-destructor-d111b62063cd1513.rstcuda_core/releasenotes/fix-set-current-single-call-a2fb40af8e2d1dee.rstcuda_core/releasenotes/fix-stream-device-context-restore-d08f1046789930a3.rstcuda_core/releasenotes/fix-stream-wait-other-device-ae307899cad418d7.rstcuda_core/releasenotes/fix-virtualmemoryresource-handle-layer-a4130dca601d51f6.rstcuda_core/releasenotes/prelude-python-310-dropped-7c3e9b1d0a5f2e84.rstcuda_core/releasenotes/upgrade-bindings-version-floor-b17a649521e708d0.rstcuda_core/releasenotes/upgrade-cleanup-failures-as-warnings-75c52b61a448ad64.rstcuda_core/releasenotes/upgrade-cpp-layer-driver-entry-points-d9ba7302b7570c9b.rstcuda_core/releasenotes/upgrade-feature-availability-from-driver-5c84637adb9aca36.rstcuda_core/releasenotes/upgrade-source-build-header-match-0704cac6c0c72756.rstcuda_core/releasenotes/upgrade-virtualmemoryresource-handle-layer-a4130dca601d51f6.rstcuda_pathfinder/AGENTS.mdcuda_pathfinder/docs/source/conf.pycuda_pathfinder/docs/source/release.rstcuda_pathfinder/docs/source/release/.gitignorecuda_python/AGENTS.mdcuda_python/docs/exts/release_date.pycuda_python/docs/exts/release_notes.pycuda_python/docs/exts/release_ranges.pycuda_python/docs/exts/release_toc.pycuda_python/docs/source/conf.pycuda_python/docs/source/release.rstcuda_python/docs/source/release/.gitignorecuda_python/docs/source/release/13.5.0-notes.rstcuda_python/releasenotes/issue-pip-upgrade-from-old-versions-5b1e0c7a93d24f68.rstcuda_python/releasenotes/prelude-python-310-dropped-2d8b6f4a1c9e7035.rsttoolshed/add_note.py
💤 Files with no reviewable changes: 2
- cuda_python/docs/source/release/13.5.0-notes.rst
- cuda_core/docs/source/release/1.3.0-notes.rst
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| # The release notes are found from the git history between release tags. | ||
| - name: Fetch release history | ||
| uses: ./.github/actions/fetch-release-history | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- release.yml relevant blocks ---'
nl -ba .github/workflows/release.yml | sed -n '120,205p'
printf '%s\n' '--- build-docs.yml relevant block ---'
nl -ba .github/workflows/build-docs.yml | sed -n '35,90p'
printf '%s\n' '--- release-cuda-pathfinder.yml relevant block ---'
nl -ba .github/workflows/release-cuda-pathfinder.yml | sed -n '40,95p'
printf '%s\n' '--- local action files ---'
find .github/actions/fetch-release-history -maxdepth 2 -type f -print -exec nl -ba {} \;
printf '%s\n' '--- PR diff for the workflows and action ---'
git diff --stat 9d903cdcaee82731618dd56e8c76bcd528918592 c99c5bd07318359595e5f76b0722b91b6855b3b4 -- .github/workflows .github/actions/fetch-release-history
git diff --unified=20 9d903cdcaee82731618dd56e8c76bcd528918592 c99c5bd07318359595e5f76b0722b91b6855b3b4 -- .github/workflows/release.yml .github/workflows/build-docs.yml .github/workflows/release-cuda-pathfinder.yml .github/actions/fetch-release-history
printf '%s\n' '--- release routing references ---'
rg -n -F -e 'backport_branch' -e 'validate_backport_decision' -e 'release_ranges' -e 'git-tag' ci .github | head -200 || trueRepository: NVIDIA/cuda-python
Length of output: 34102
🏁 Script executed:
set -eu
printf '%s\n' '--- base and head file presence ---'
for rev in 9d903cdcaee82731618dd56e8c76bcd528918592 c99c5bd07318359595e5f76b0722b91b6855b3b4; do
echo "REV $rev"
for path in .github/actions/fetch-release-history/action.yml ci/tools/check_release_notes.py ci/tools/release_ranges.py cuda_python/docs/exts/release_notes.py .github/workflows/release.yml .github/workflows/build-docs.yml .github/workflows/release-cuda-pathfinder.yml; do
if git cat-file -e "$rev:$path" 2>/dev/null; then echo " present $path"; else echo " absent $path"; fi
done
done
printf '%s\n' '--- relevant release workflow header and inputs ---'
nl -ba .github/workflows/release.yml | sed -n '1,120p'
printf '%s\n' '--- release ranges and versions ---'
nl -ba ci/release_ranges.py 2>/dev/null | sed -n '1,180p' || true
nl -ba ci/versions.yml | sed -n '1,40p'
printf '%s\n' '--- tags and action presence for representative release tags ---'
git tag --list | grep -E '^(cuda-bindings-v|cuda-core-v|cuda-pathfinder-v|v)' | tail -80 || true
for tag in $(git tag --list | grep -E '^(cuda-bindings-v|cuda-core-v|cuda-pathfinder-v|v)' | tail -20); do
if git cat-file -e "$tag:.github/actions/fetch-release-history/action.yml" 2>/dev/null; then state=present; else state=absent; fi
echo "$tag action=$state"
done
printf '%s\n' '--- changed files outside workflow scope relevant to compatibility ---'
git diff --name-status 9d903cdcaee82731618dd56e8c76bcd528918592 c99c5bd07318359595e5f76b0722b91b6855b3b4 | sed -n '1,160p'Repository: NVIDIA/cuda-python
Length of output: 12970
🏁 Script executed:
set -eu
printf '%s\n' '--- release package ranges ---'
rg -n -F -e 'cuda-pathfinder' -e '1.8' -e 'PACKAGES' cuda_python/docs/exts/release_ranges.py
nl -ba cuda_python/docs/exts/release_ranges.py | sed -n '1,180p'
printf '%s\n' '--- backport validation and release workflow routing ---'
nl -ba ci/tools/check_release_notes.py | sed -n '210,285p'
nl -ba .github/workflows/backport.yml | sed -n '1,90p'Repository: NVIDIA/cuda-python
Length of output: 13451
important: Keep release-history fetching independent of the checked-out release tag.
Each workflow checks out the release tag before it invokes ./.github/actions/fetch-release-history. Tags created before this PR do not contain action.yml, so the release can stop with Can't find 'action.yml' before the release-note check. This affects the configured maintenance-branch backport flow and older cuda-pathfinder 1.8.x tags.
Inline the fetch command in all three workflows:
- - name: Fetch release history
- uses: ./.github/actions/fetch-release-history
+ - name: Fetch release history
+ run: |
+ refspecs=("$(git rev-parse HEAD)" '+refs/tags/*:refs/tags/*')
+ if [[ "$(git rev-parse --is-shallow-repository)" == "true" ]]; then
+ git fetch --unshallow --no-tags origin "${refspecs[@]}"
+ else
+ git fetch --no-tags origin "${refspecs[@]}"
+ fiApply this change to .github/workflows/release.yml, .github/workflows/build-docs.yml, and .github/workflows/release-cuda-pathfinder.yml.
🧰 Tools
🪛 zizmor (1.30.1)
[warning] 162-167: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 5-285: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 158-181: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 169-169: use GitHub's dedicated self-repository syntax (self-repository): use '$/...' instead of './...'
(self-repository)
There was a problem hiding this comment.
I'm not sure I follow. The action should be available from every commit it is called from. Can you describe the scenario in which this would fail?
There was a problem hiding this comment.
@mdboom important: The workflow definition and the checked-out files can come from different commits.
The failure scenario is:
- This PR merges into
main. - A maintainer dispatches the release workflow from
main, withgit-tagset to a tag created before this PR. actions/checkoutchecks out that tag because the step setsref: ${{ inputs.git-tag }}.- The next step invokes
./.github/actions/fetch-release-history.
GitHub resolves a ./ action from the checked-out workspace, not from the commit that supplied the workflow definition. If the selected tag lacks .github/actions/fetch-release-history/action.yml, the action cannot load.
The action is available in the commit that defines the workflow, as you describe. The preceding checkout replaces the workspace with a different commit.
If releases only target tags that contain this action, this failure does not occur. My concern is running the updated workflow against older tags or maintenance-branch tags that do not contain it.
You are interacting with an AI system.
| Only plain `vX.Y.Z` tags count as releases. Pre-release tags (`rc`, `a`, `b`) | ||
| are ignored, and their notes roll into the final release. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
suggestion: Fix the tag format in the release-tag rule. Line 328 says only plain vX.Y.Z tags count as releases. That is true only for cuda-bindings and cuda-python. release_ranges.PACKAGES uses cuda-core-v and cuda-pathfinder-v as the tag prefixes for the other two packages. A cuda-core release manager who reads this line can conclude that cuda-core-v1.3.0 tags are ignored. State the rule as <prefix>X.Y.Z, and list the per-package prefixes.
--- "a/CONTRIBUTING.md"
+++ "b/CONTRIBUTING.md"
@@ -325,7 +325,8 @@
The docs build generates a page for every release from the notes in git,
with a final "In development" page for notes that are not in any release yet.
-Only plain `vX.Y.Z` tags count as releases. Pre-release tags (`rc`, `a`, `b`)
+Only plain `<prefix>X.Y.Z` release tags count (`v` for `cuda-bindings` and
+`cuda-python`, `cuda-core-v` and `cuda-pathfinder-v` for the others). Pre-release tags (`rc`, `a`, `b`)
are ignored, and their notes roll into the final release.
* A minor release (`X.Y.0`) lists the notes added since the previous minor📝 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.
| Only plain `vX.Y.Z` tags count as releases. Pre-release tags (`rc`, `a`, `b`) | |
| are ignored, and their notes roll into the final release. | |
| Only plain `<prefix>X.Y.Z` release tags count (`v` for `cuda-bindings` and | |
| `cuda-python`, `cuda-core-v` and `cuda-pathfinder-v` for the others). Pre-release tags (`rc`, `a`, `b`) | |
| are ignored, and their notes roll into the final release. |
| # that is both in a page's range and in its known issues is looked up for the same ref twice. | ||
| @functools.cache | ||
| def _pr_number(repo_root: Path, ref: str, path: str) -> str | None: | ||
| subjects = _git(repo_root, "log", "--diff-filter=A", "--format=%s", ref, "--", path).strip().split("\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
suggestion: Add --no-renames to the PR-number lookup so it handles renamed notes the same way as _notes_in_range.
_notes_in_range (Line 273) uses --no-renames, so a renamed note counts as a new note. _pr_number does not use that flag. Rename detection is on by default for git log, so the rename commit shows as R and the --diff-filter=A filter drops it. Without --follow, the history of the new path then has no A commit. In that case subjects is [""], the note gets no PR link, and a logger.warning is emitted for tag refs. test_a_renamed_note_counts_as_a_new_note_in_the_release_that_contains_the_rename covers this path but does not check the link.
--- "a/cuda_python/docs/exts/release_notes.py"
+++ "b/cuda_python/docs/exts/release_notes.py"
@@ -283,7 +283,9 @@
# that is both in a page's range and in its known issues is looked up for the same ref twice.
@functools.cache
def _pr_number(repo_root: Path, ref: str, path: str) -> str | None:
- subjects = _git(repo_root, "log", "--diff-filter=A", "--format=%s", ref, "--", path).strip().split("\n")
+ subjects = _git(
+ repo_root, "log", "--no-renames", "--diff-filter=A", "--format=%s", ref, "--", path
+ ).strip().split("\n")
# The oldest commit that added the file is the last one listed.
m = _PR_SUFFIX_RE.search(subjects[-1])
if m is not None:📝 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.
| subjects = _git(repo_root, "log", "--diff-filter=A", "--format=%s", ref, "--", path).strip().split("\n") | |
| subjects = _git( | |
| repo_root, "log", "--no-renames", "--diff-filter=A", "--format=%s", ref, "--", path | |
| ).strip().split("\n") |
This branch has not been deployed
3 participants
This moves us to snippet-based release note management. This is a prerequisite for proper use of maintenance branches, since release note pages for patch releases can be automatically created by backporting the release note snippet.
For a good description of this approach in general see reno's doc page. Note that CPython also uses a very similar approach to great success.
My initial implementation of this used
renoas a dependency, but I ultimately pulled back from that becausereno's behavior varied in important ways from what we need. We spent more lines of code "working around" reno's behavior that it took to just write our own implementation here. The conventions used still closely hew to what reno does to avoid re-solving problems they have already solved.The differences from
renoare:Known issues had to be sticky. Reno attributes a note to the release that first contains it, so a known issue would have appeared on one page only.
cuda-python's long-standing policy is that known issues are listed on every release until resolved.Notes had to stay editable after release. Reno reads each note as of the commit that added it, so editing an already-released note changed no page. Since we publish docs from main and want corrections to old release notes to show up, each note's current text is fetched from
main.Review recommendation
I recommend starting with
CONTRIBUTING.mdto understand how this is intended to work, and then working out from there.What the change does
Release notes move from one hand-written
release/<version>-notes.rstper release to one small YAMLfile per change. The design is inspired by reno but is a custom
Sphinx extension, not reno itself.
python toolshed/add_note.py <package> <short-description>creates<package>/releasenotes/<short-description>-<16 hex>.yamlfrom a template. A note maps section keys(
features,fixes,issues,prelude, ...) to lists of reStructuredText entries. Merged notesare never renamed.
cuda_python/docs/exts/release_notes.pybuildsrelease/<version>-notes.rstfor every release tag at or after the package's first note-basedversion, plus an "In development" page (
unreleased-notes.rst). Which notes go on which page comesfrom git tag ranges (
release_ranges.py), not from reno-style version labels.issuesentries are listed on every page whose tag still contains them, with thetext they had at that tag. Resolving one means deleting the entry.
(#N)suffix of the commit that added the note file, orfrom explicit
(#N)markers in the entry.release/.gitignore), and theextension refuses to overwrite a file without the marker.
release-notes-lint(ci/tools/lint_release_notes.py): file name, YAML shape, knownkeys written plainly (
fixes:at line start), no leftoverTODO.pr-metadata-check.ymlrunsci/tools/check_pr_release_notes.py: a PR that touches a package'ssource_pathsneeds a note added or modified, unless it has theskip-release-notelabel.ci/tools/check_release_notes.py: hand-written page before the firstnote-based version, otherwise at least one note added in the release's range.
13.5.0(cuda-bindings) and1.3.0(cuda-core) pages are deletedand replaced by note files. Earlier pages stay hand-written.