Add griffe API checking to cuda_core#2300
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 |
github-actions
Bot
added
CI/CD
|
/ok to test |
|
|
| # Findings fail this job (shows red in the UI), but it never blocks merging | ||
| # the PR: it's intentionally excluded from `checks:`'s `needs`, so it isn't | ||
| # part of the required aggregator status. | ||
| api-check-core: |
There was a problem hiding this comment.
Need to wire this job up in the check status job.
There was a problem hiding this comment.
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.
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.
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.
We could introduce a griffe-override label 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.
| del _patch_rlcompleter_for_cython_properties | ||
|
|
||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
Q: If this is a must to make griffe happy, should we just unify the style and always use __all__ in every submodule, private or public?
There was a problem hiding this comment.
It's just needed in the public ones. I think that's a reasonable convention.
|
Very nice! It shows warnings right at the offending diff! 
|
Yep. Someday maybe we can get it to put it in the |
|
Note: The changes that this is highlighting against the This PR should work as-is for checking for changes in a specific PR. For changes against the latest release, we will need to wait until the next release of cuda-core (which will have |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
gpt-5.5 (with manual edits/additions)
Findings
-
Low:
cuda_core/cuda/core/__init__.pyaddsDeviceResourcesto top-level__all__, but it is documented incuda_core/docs/source/api_private.rst, notcuda_core/docs/source/api.rst. Sincecuda_core/AGENTS.mddefines public API by__all__, this promotes a returned helper into the explicit public surface. I would either remove it from top-level__all__or move it to the main API page. -
Low:
.github/workflows/ci.ymlrunsapi-check-coreonly whencuda_core/changes. A follow-up that edits only.github/actions/griffe-api-check/action.ymlwill not exercise the action. Consider gating oncore || shared, or a narrower action-specific change flag. -
Low:
.github/actions/griffe-api-check/action.ymlinvokes unpinneduvx griffein both comparisons. That is fine for a prototype, but if this becomes a trusted signal, pin griffe or route it through the repo-managed environment.
Maintenance Risk
A maintenance risk from adding __all__ is source-of-truth drift. New APIs can be imported and documented but omitted from __all__, stale names can remain after removals (although import * would catch those), and returned/private helper types can accidentally become SemVer-public. The DeviceResources mismatch is a concrete example: it is imported at the top level and now listed in __all__, but the docs currently treat it as a private/returned helper.
That said, an explicit __all__ is probably the right convention if griffe is going to define and compare the supported public API from it. The burden is manageable as long as there is a consistency check that catches drift early.
A simple guard against stale names could be:
def test_cuda_core_all_exports_resolve():
import cuda.core
missing = [name for name in cuda.core.__all__ if not hasattr(cuda.core, name)]
assert missing == []Suggested Follow-On Idea
Add a follow-on docs/API consistency test that compares the public symbols in cuda.core.__all__ against the public entries in cuda_core/docs/source/api.rst, with deliberate exceptions for submodule namespaces and returned-helper/private API docs.
It should parse the Sphinx autosummary and data entries, then account for known surfaces such as cuda.core.graph, cuda.core.texture, cuda.core.utils, cuda.core.checkpoint, and entries intentionally documented in api_private.rst. The useful failure mode is: “this symbol is public by __all__ but missing from public docs” or “this symbol is documented as public but not exported by __all__.”
| # Findings fail this job (shows red in the UI), but it never blocks merging | ||
| # the PR: it's intentionally excluded from `checks:`'s `needs`, so it isn't | ||
| # part of the required aggregator status. | ||
| api-check-core: |
There was a problem hiding this comment.
We could introduce a griffe-override label 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.
3 participants
This adds a CI job to run
griffeovercuda_coreto detect API breakage.For this prototype, it checks both against the merge base of the PR and against the latest
cuda-coretag.This PR also deliberately makes an API breakage (the same one found in #2280) so we can confirm it's working.
One side effect of this is that griffe /requires/ the top-level
__init__.pyto have an__all__so it's explicit about what the public API is. This is a minor ongoing maintenance burden, but probably worth the effort.