-
Notifications
You must be signed in to change notification settings - Fork 287
[agents] Update AGENTS.MD with review-derived guidance #2106
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 |
|---|---|---|
|
|
@@ -12,6 +12,83 @@ guide for package-specific conventions and workflows. | |
| - `cuda_core/`: High-level Pythonic CUDA APIs built on top of bindings. | ||
| - `cuda_python/`: Metapackage and docs aggregation. | ||
|
|
||
| # Review-derived repository guidance | ||
|
|
||
| These rules come from recurring cuda-python PR review comments. Apply them | ||
| across the repository, in addition to any package-specific `AGENTS.md`. | ||
|
|
||
| ## Public API and design | ||
|
|
||
| - For new public APIs, major behavior changes, or broad feature work, make sure | ||
| the API surface is sketched in an issue or design discussion before coding. | ||
| Reviewers repeatedly block large feature PRs that arrive without design | ||
| context, especially before 1.0 API stabilization. | ||
| - Keep public APIs minimal and intentional. Avoid exposing private helpers just | ||
| to make tests or examples easier; prefer improving the public path or keeping | ||
| helpers private. | ||
| - When adding public behavior, update docs, examples, release notes, and API | ||
| index pages in the same PR unless the PR explicitly documents why those | ||
| updates are deferred. | ||
| - User-facing errors and warnings should name the user-actionable concept, not | ||
| a private helper. Include diagnostics when something cannot be discovered, | ||
| and avoid silent success-shaped fallbacks. | ||
|
|
||
| ## Tests and CI behavior | ||
|
|
||
| - Add targeted regression tests for behavioral fixes. Do not add elaborate | ||
| tests that mostly prove an implementation detail or require large module | ||
| stubbing unless that is the only practical way to cover the bug. | ||
| - Do not weaken tests just to pass a platform or CI configuration. Avoid broad | ||
| platform skips such as "skip all WSL" or "skip all Windows"; query CUDA | ||
| driver/device capability or the specific missing library/feature instead. | ||
|
Comment on lines
+41
to
+43
Contributor
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. I think the key point is that it's fine to "skip all Windows" if the upstream library documentation says something like "not supported on Windows". Otherwise we should be trying to use the tightest criteria possible. |
||
| - Preserve real user workflows in tests. Do not change global CUDA state, skip | ||
| real loading paths, or disable release-note/doc checks merely to reduce CI | ||
| load unless reviewers have agreed to that behavior change. | ||
| - Before pushing, run the narrowest relevant `pixi run ...`, `pytest`, docs, or | ||
| workflow validation command available for the touched package. If local | ||
| validation is impossible, state the exact reason in the PR. | ||
|
|
||
| ## Generated code and CUDA compatibility | ||
|
|
||
| - Do not hand-edit generated binding artifacts as a shortcut. Fix the generator | ||
| source or templates and regenerate/sync outputs so the next generation does | ||
| not reintroduce the same review issue. | ||
| - Lint or formatting changes that touch generated files should either be made | ||
| in the generator (`cython-gen`, `cybind`, templates, or sync source) or should | ||
| exclude generated outputs from the check. | ||
|
Comment on lines
+56
to
+58
Contributor
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. Ditto to above. We have historically just skipped linting and formatting on these files altogether. Maybe saying something like "use .pre-commit-config.yaml for linting and formatting" would be better so we can just deterministically encode the rules there. |
||
| - Keep builds working across the supported CUDA major versions. Do not cimport | ||
| or call newly generated Cython symbols directly unless the older supported | ||
| CUDA-major build is gated or has a wrapper/fallback path. | ||
| - In Cython/CUDA code, preserve CUDA stream ordering, handle ownership, and | ||
| context-manager semantics. Cleanup paths should not mask a user's original | ||
| exception, and `__enter__` should not expose invalid handles. | ||
|
|
||
| ## Workflows, packaging, and metadata | ||
|
|
||
| - GitHub workflow logic should parse structured data with `jq` or GitHub's | ||
| `--jq` support. Avoid substring `grep` checks for labels, milestones, or | ||
| JSON fields when exact matching is possible. | ||
| - Use the correct GitHub Actions context (`env`, `vars`, `github`, `inputs`) | ||
| deliberately; a wrong context often evaluates to an empty string and silently | ||
| breaks release or validation workflows. | ||
| - Keep workflow permissions minimal and explicit, and include all triggers | ||
| needed for metadata checks to rerun when labels or milestones change. | ||
| - Packaging changes should keep version constraints, wheel/sdist behavior, | ||
| release tags, and metapackage dependencies aligned across all affected | ||
| packages. Use exact version or lower-bound choices intentionally. | ||
|
|
||
| ## Documentation style | ||
|
|
||
| - For Sphinx/Numpy-style docs, document class construction in the class | ||
| docstring and signature rather than separately documenting `__init__` or | ||
| `__new__`, unless the surrounding docs already use that convention. | ||
| - Add docs entries for new public classes/functions in the relevant | ||
| `docs/source/api*.rst` or autosummary index, and build docs when changing | ||
| generated API pages. | ||
| - Prefer concise comments that explain non-obvious compatibility, security, or | ||
| workflow choices. Remove duplicated error text, stale TODOs, and comments that | ||
| merely restate the code. | ||
|
|
||
| # Pull requests | ||
|
|
||
| When creating pull requests with `gh pr create`, always assign at least one | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,12 @@ subpackage in the `cuda-python` monorepo. | |||||||||||
| defined in `build_hooks.py`; update those rules when introducing new symbols. | ||||||||||||
| - **Platform split files**: keep `_linux.pyx` and `_windows.pyx` variants | ||||||||||||
| aligned when behavior should be equivalent. | ||||||||||||
| - **Lint at the source**: if formatting or lint fixes affect generated files, | ||||||||||||
| make the fix in the generation source (`cython-gen`, `cybind`, templates, or | ||||||||||||
|
Collaborator
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. cython-gen and cybind aren't public projects so a non-NVIDIA contributor will not have access to contribute fixes |
||||||||||||
| sync source) or exclude generated outputs from the check. Otherwise the next | ||||||||||||
| regeneration will reintroduce the same issue. | ||||||||||||
|
Comment on lines
+38
to
+41
Contributor
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. I would be stricter about this, since it isn't always easy to generate code that would pass all lints.
Suggested change
|
||||||||||||
| - **Cython copies**: prefer typed assignment for wrapper-owned C struct copies | ||||||||||||
| over raw `memcpy` when the generated Cython/C types can define the copy size. | ||||||||||||
|
|
||||||||||||
| ## Testing expectations | ||||||||||||
|
|
||||||||||||
|
|
@@ -65,3 +71,10 @@ subpackage in the `cuda-python` monorepo. | |||||||||||
| `docs/source/module/` and tests in `tests/`. | ||||||||||||
| - Prefer changes that are easy to regenerate/rebuild rather than patching | ||||||||||||
| generated output directly. | ||||||||||||
| - Preserve compatibility with the supported CUDA major-version matrix. If a new | ||||||||||||
| CUDA header symbol is unavailable in an older supported build, gate it or call | ||||||||||||
| through an existing Python wrapper instead of directly cimporting the new | ||||||||||||
| generated Cython symbol. | ||||||||||||
| - For external contributions touching generated `cuda_bindings` code, ask for a | ||||||||||||
| reproducer and environment details, then route fixes through the generation | ||||||||||||
| source rather than accepting one-off generated edits. | ||||||||||||
|
Collaborator
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. Now that we're v1.0+ it would be nice to have something in here to be mindful of API breakage |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,3 +63,21 @@ This file describes `cuda_core`, the high-level Pythonic CUDA subpackage in the | |
| call-site consistency. | ||
| - Prefer explicit error propagation over silent fallback paths. | ||
| - If you change public behavior, update tests and docs under `docs/source/`. | ||
| - For new public APIs or broad feature work, sketch the API and behavior in an | ||
| issue/design discussion before opening a large implementation PR. Reviewers | ||
| often block major `cuda_core` features until API shape, examples, and | ||
| docs/release-note coverage are clear. | ||
| - Feature availability checks should query CUDA driver/device capabilities | ||
| instead of hard-coding broad platform skips. Prefer properties such as | ||
| capability flags over assumptions like "Windows", "Linux", or "WSL". | ||
| - Keep CUDA 12.x and 13.x build compatibility in mind. Do not directly cimport | ||
| newly generated binding symbols unless older supported CUDA-major builds are | ||
| gated or have a wrapper/fallback path. | ||
|
Comment on lines
+73
to
+75
Contributor
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. Do we want to say something like "2 most recent versions of CUDA" to avoid hard-coding 12 and 13 here?
Collaborator
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. +1. I like the wording in the cuda_bindings file above of "Preserve compatibility with the supported CUDA major-version matrix." |
||
| - Resource and context-manager code must preserve stream ordering, ownership, | ||
| and exception semantics. `close()`/cleanup paths should use the stream that | ||
| established the resource ordering, and `__exit__` should avoid masking a | ||
| user's original exception where practical. | ||
| - Tests should cover the behavior users exercise, not just private helpers. | ||
| Avoid large module-stubbing tests for simple implementation choices; prefer | ||
| focused regressions around the public API or the smallest stable internal | ||
| boundary. | ||
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.
"Avoid" feels too strong, unless we plan to move many more tests to Cython. Exposing private members is sort of the only feasible way to write tests in Python when implementing in Cython.