-
Notifications
You must be signed in to change notification settings - Fork 305
Add griffe API checking to cuda_core #2300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: griffe API check | ||
|
|
||
| description: >- | ||
| Check a package's public API (as defined by `__all__`) for changes using | ||
| griffe, comparing the current code against both the PR's merge-base and the | ||
| package's latest matching release tag. This action itself always succeeds; | ||
| it reports findings via the `has-findings` output and a job summary so the | ||
| caller can decide whether/how to fail its own job. | ||
|
|
||
| inputs: | ||
| package-name: | ||
| description: "Importable package name to check, e.g. cuda.core" | ||
| required: true | ||
| package-dir: | ||
| description: "Directory to search for the package sources, e.g. cuda_core" | ||
| required: true | ||
| merge-base: | ||
| description: >- | ||
| Git ref/sha to compare the current code against, typically the PR's | ||
| merge-base with its target branch. | ||
| required: true | ||
| tag-pattern: | ||
| description: >- | ||
| Glob pattern (as passed to `git describe --match`) matching the | ||
| package's release tags, e.g. cuda-core-v* | ||
| required: true | ||
|
|
||
| outputs: | ||
| has-findings: | ||
| description: "true if griffe reported API differences in either comparison" | ||
| value: ${{ steps.summarize.outputs.has-findings }} | ||
|
|
||
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 | ||
| with: | ||
| enable-cache: false | ||
|
|
||
| - name: Check API vs. PR base branch | ||
| id: vs-base | ||
| shell: bash --noprofile --norc -euo pipefail {0} | ||
| continue-on-error: true | ||
| env: | ||
| PACKAGE_NAME: ${{ inputs.package-name }} | ||
| PACKAGE_DIR: ${{ inputs.package-dir }} | ||
| MERGE_BASE: ${{ inputs.merge-base }} | ||
| run: | | ||
| # griffe writes its `::warning::`/`::error::` annotations to stderr, | ||
| # so stdout and stderr must both be captured for the summary file | ||
| # below to contain anything. | ||
| uvx griffe check "$PACKAGE_NAME" \ | ||
| --search "$PACKAGE_DIR" \ | ||
| --find-stubs-packages \ | ||
| --against "$MERGE_BASE" \ | ||
| --format github \ | ||
| 2>&1 | tee griffe-vs-base.txt | ||
|
|
||
| - name: Find latest release tag | ||
| id: latest-tag | ||
| shell: bash --noprofile --norc -euo pipefail {0} | ||
| env: | ||
| TAG_PATTERN: ${{ inputs.tag-pattern }} | ||
| run: | | ||
| # No matching tag (e.g. a brand-new package) is not an error: the | ||
| # release-compatibility comparison is simply skipped below. | ||
| tag="$(git describe --tags --match "$TAG_PATTERN" --abbrev=0 2>/dev/null || true)" | ||
| echo "tag=${tag}" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Check API vs. latest release tag | ||
| id: vs-tag | ||
| if: ${{ steps.latest-tag.outputs.tag != '' }} | ||
| shell: bash --noprofile --norc -euo pipefail {0} | ||
| continue-on-error: true | ||
| env: | ||
| PACKAGE_NAME: ${{ inputs.package-name }} | ||
| PACKAGE_DIR: ${{ inputs.package-dir }} | ||
| LATEST_TAG: ${{ steps.latest-tag.outputs.tag }} | ||
| run: | | ||
| # See the comment in the "Check API vs. PR base branch" step: griffe's | ||
| # annotations go to stderr, so redirect it into the captured stream. | ||
| uvx griffe check "$PACKAGE_NAME" \ | ||
| --search "$PACKAGE_DIR" \ | ||
| --find-stubs-packages \ | ||
| --against "$LATEST_TAG" \ | ||
| --format github \ | ||
| 2>&1 | tee griffe-vs-tag.txt | ||
|
|
||
| - name: Summarize results | ||
| id: summarize | ||
| if: always() | ||
| shell: bash --noprofile --norc -euo pipefail {0} | ||
| env: | ||
| PACKAGE_NAME: ${{ inputs.package-name }} | ||
| VS_BASE_OUTCOME: ${{ steps.vs-base.outcome }} | ||
| VS_TAG_OUTCOME: ${{ steps.vs-tag.outcome }} | ||
| LATEST_TAG: ${{ steps.latest-tag.outputs.tag }} | ||
| run: | | ||
| has_findings=false | ||
|
|
||
| { | ||
| echo "### API check: \`${PACKAGE_NAME}\`" | ||
| echo "" | ||
| echo "_This check is informational; it does not block the PR. Review findings and confirm any API changes are intentional._" | ||
| echo "" | ||
|
|
||
| if [[ "$VS_BASE_OUTCOME" == "failure" ]]; then | ||
| has_findings=true | ||
| echo "#### :warning: Changes vs. PR base branch" | ||
| echo '```' | ||
| cat griffe-vs-base.txt | ||
| echo '```' | ||
| else | ||
| echo "#### :white_check_mark: No changes vs. PR base branch" | ||
| fi | ||
| echo "" | ||
|
|
||
| if [[ -z "$LATEST_TAG" ]]; then | ||
| echo "#### No release tag matched; skipped release-compatibility check." | ||
| elif [[ "$VS_TAG_OUTCOME" == "failure" ]]; then | ||
| has_findings=true | ||
| echo "#### :warning: Changes vs. latest release (\`${LATEST_TAG}\`)" | ||
| echo '```' | ||
| cat griffe-vs-tag.txt | ||
| echo '```' | ||
| else | ||
| echo "#### :white_check_mark: No changes vs. latest release (\`${LATEST_TAG}\`)" | ||
| fi | ||
| } >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| echo "has-findings=${has_findings}" >> "$GITHUB_OUTPUT" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,48 @@ class _PatchedProperty(metaclass=_PatchedPropMeta): | |
| del _patch_rlcompleter_for_cython_properties | ||
|
|
||
|
|
||
| __all__ = [ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: If this is a must to make griffe happy, should we just unify the style and always use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just needed in the public ones. I think that's a reasonable convention. |
||
| "LEGACY_DEFAULT_STREAM", | ||
| "PER_THREAD_DEFAULT_STREAM", | ||
| "Buffer", | ||
| "Context", | ||
| "ContextOptions", | ||
| "Device", | ||
| "DeviceMemoryResource", | ||
| "DeviceMemoryResourceOptions", | ||
| "DeviceResources", | ||
| "Event", | ||
| "EventOptions", | ||
| "GraphMemoryResource", | ||
| "GraphicsResource", | ||
| "Host", | ||
| "Kernel", | ||
| "LaunchConfig", | ||
| "LegacyPinnedMemoryResource", | ||
| "Linker", | ||
| "LinkerOptions", | ||
| "ManagedBuffer", | ||
| "ManagedMemoryResource", | ||
| "ManagedMemoryResourceOptions", | ||
| "MemoryResource", | ||
| "ObjectCode", | ||
| "PinnedMemoryResource", | ||
| "PinnedMemoryResourceOptions", | ||
| "Program", | ||
| "ProgramOptions", | ||
| "SMResource", | ||
| "SMResourceOptions", | ||
| "Stream", | ||
| "StreamOptions", | ||
| "TensorMapDescriptor", | ||
| "TensorMapDescriptorOptions", | ||
| "VirtualMemoryResource", | ||
| "VirtualMemoryResourceOptions", | ||
| "WorkqueueResource", | ||
| "WorkqueueResourceOptions", | ||
| "launch", | ||
| ] | ||
|
|
||
| from cuda.core import checkpoint, system, utils | ||
| from cuda.core._context import Context, ContextOptions | ||
| from cuda.core._device import Device | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from ._graph_builder import * | ||
| from ._graph_builder import __all__ as _graph_builder_all | ||
| from ._graph_definition import * | ||
| from ._graph_definition import __all__ as _graph_definition_all | ||
| from ._graph_node import * | ||
| from ._graph_node import __all__ as _graph_node_all | ||
| from ._subclasses import * | ||
| from ._subclasses import __all__ as _subclasses_all | ||
|
|
||
| __all__ = [*_graph_builder_all, *_graph_definition_all, *_graph_node_all, *_subclasses_all] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to wire this job up in the check status job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should eventually. I was thinking we could make it a "soft fail" to start with while we learn about whether it produces any false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure we ever want this to fail. There will be times in our release cycle where we are ok with breaking the API (once something has been deprecated for some time). We could maybe convince griffe to understand that someday, but in the meantime, having human-override judgement in the loop is probably good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we could admin-merge maybe? Or is there a way to skip griffe on a per-line basis (like
# noqa)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could introduce a
griffe-overridelabel or similar to enable merging. Or alternatively[API-BREAK]in the PR title, to make it totally obvious. Or both, so we can easily deal with griffe false positives.