Repository navigation
Conversation
Signed-off-by: Shrish Kulkarni <[email protected]>
Signed-off-by: Shrish Kulkarni <[email protected]>
zhindes
left a comment
zhindes
left a comment
There was a problem hiding this comment.
Concern about the maintainability of the PR yml and how to break compat if we need to.
| # API Compatibility | ||
|
|
||
| Pull requests run Griffe to check the generated public API against the pull request's base commit. | ||
| The check covers the package and the modules in the API reference separately because package |
There was a problem hiding this comment.
Is this something we should fix instead of working around?
| nidaqmx.system.storage.persisted_task nidaqmx.system.system nidaqmx.system.watchdog \ | ||
| nidaqmx.task nidaqmx.task.channels nidaqmx.task.collections nidaqmx.task.triggering \ | ||
| nidaqmx.types nidaqmx.utils; do | ||
| if ! griffe check --search generated --against "$BASE_SHA" --format github "$module"; then |
There was a problem hiding this comment.
Good, --format github creates GitHub annotations. Can you please link to a workflow log that shows what these look like?
There was a problem hiding this comment.
Also please post a screenshot, since the workflow log will expire and no longer be accessible.
Signed-off-by: Shrish Kulkarni <[email protected]>
…pi-compatibility-checks Signed-off-by: Shrish Kulkarni <[email protected]>
|
Updated the PR with Poetry-managed tools, the shared Python default, and a reusable workflow called from CI. It defaults to the latest tag and supports explicit bases and manual previous-release comparisons. The manual package list is replaced by documentation-derived visibility, with regression tests. Approved breaking changes are documented; operational errors remain fatal. @bkeryan, regarding fixing the underlying exports: would you prefer that cleanup in this PR or a follow-up? Updating current exports would improve consistency, but historical release comparisons would still need additional visibility handling. Also, could you please approve the workflow run? Once an approved run demonstrates compatibility findings, I’ll post the log link and annotation screenshot. |
follow-up PR |
@bkeryan @zhindes I feel the rest of the changes are in. Once the workflow is approved, I’ll post the log link and annotation screenshot. |
You can test out workflow changes in your personal fork of this repo. |
There was a problem hiding this comment.
Why are you doing this? It seems like you ran into some sort of problem, but you haven't clearly communicated what that problem is. Instead, you went off and wrote an elaborate workaround.
You wrote this in the PR description:
The documentation-derived policy covers documented APIs in private implementation modules and public aliases. Each snapshot uses its own documentation, preventing removal of documentation from hiding a previously documented API break.
I have no idea what policy you are talking about. Is this something you made up? Is this something that griffe implements? Is this something that was output from an LLM?
griffe is a tool that I have used before. Please use it in a normal way. If there are problems with doing that, please communicate what those problems are so that we can discuss how to solve them.
The GitHub issue includes an example of how to use griffe. I tried it again, and it still works:
PS C:\dev\nidaqmx-python> griffe check --search generated nidaqmx -a 1.3.0 --verbose
generated\nidaqmx\stream_readers\_analog_multi_channel_reader.py:140: AnalogMultiChannelReader.read_waveforms(timeout):
Parameter default was changed:
Old: 10
New: 10.0
generated\nidaqmx\stream_readers\_analog_single_channel_reader.py:112: AnalogSingleChannelReader.read_waveform(timeout):
Parameter default was changed:
Old: 10
New: 10.0
generated\nidaqmx\stream_readers\_digital_multi_channel_reader.py:489: DigitalMultiChannelReader.read_waveforms(timeout):
Parameter default was changed:
Old: 10
New: 10.0
generated\nidaqmx\system\system.py:80: System.DriverVersion:
Attribute value was changed:
Old: collections.namedtuple('DriverVersion', ['major_version', 'minor_version', 'update_version'])
New: nidaqmx.types.DriverVersion
generated\nidaqmx\task\_task.py:364: Task.close:
Public object points to a different kind of object:
Old: function
New: attribute
These are all intentional changes to the API signature that are not expected to break compatibility for normal use cases.
| CI runs [Griffe](https://mkdocstrings.github.io/griffe/guide/users/checking/) for pull requests and | ||
| pushes to `master` and release branches. The reusable `.github/workflows/check_api_compatibility.yml` | ||
| workflow accepts an optional `base` Git reference. By default it uses the most recently created Git | ||
| tag, matching Griffe's default. For PRs targeting `releases/**`, CI passes the PR's base commit SHA | ||
| instead. | ||
| Label changes and PR edits rerun CI through the same caller; they do not bypass existing build or | ||
| test jobs. | ||
|
|
||
| The checker loads each package snapshot once and compares the `nidaqmx` root once. Docutils parses | ||
| the hand-written RST API declarations, including canonical object targets, rather than searching | ||
| generated documentation with regular expressions. Each snapshot's own documentation defines extra | ||
| visibility in Griffe's model, including documented APIs in private implementation modules and public | ||
| aliases of documented objects. Removing documentation in a PR therefore cannot hide a previously | ||
| documented API removal. There is no package list to maintain in the workflow. | ||
|
|
||
| This policy changes only the analysis models, not Python exports. Cleaning up source-level exports | ||
| is a separate public API decision requiring maintainer agreement, including effects on wildcard | ||
| imports and supported import paths. Such a cleanup would not retroactively fix the exports in | ||
| previous release snapshots, so it cannot replace the historical documentation policy. | ||
|
|
||
| To reproduce the package checks locally, use the locked Poetry tooling group with Python 3.11 or | ||
| later. For the default release comparison: | ||
|
|
||
| ```sh | ||
| poetry install --only compatibility --no-root | ||
| git fetch origin --tags | ||
| poetry run python .github/scripts/check_api_compatibility.py | ||
| poetry run python -m unittest discover -s .github/scripts -v | ||
| ``` | ||
|
|
||
| Run these commands from the repository root. To compare against a particular release, pass | ||
| `--base 1.3.0`. For a release-target PR, fetch its target branch and pass `--base` with the exact PR | ||
| base SHA. The workflow prints the selected | ||
| reference and resolved SHA so a CI run can be reproduced without relying on a moving branch or tag. | ||
| If no release tag exists, supply an explicit base reference instead. The checks compare your | ||
| current generated source against that baseline. | ||
|
|
||
| Once the workflow is available on the default branch, maintainers can also select **Actions > Check | ||
| API compatibility > Run workflow**, choose `master` as the branch, and set `base` to a previous release | ||
| tag. The equivalent GitHub CLI command is: | ||
|
|
||
| ```sh | ||
| gh workflow run check_api_compatibility.yml --repo ni/nidaqmx-python --ref master -f base=1.3.0 | ||
| ``` | ||
|
|
||
| Griffe reports potential breaks, including removed objects, changed parameters, and changed |
There was a problem hiding this comment.
Contributors should not have to read all this. CONTRIBUTING.md should focus on the contributor workflow and expectations for the contributor.
Did you write this or is it the output of an LLM?
| permissions: | ||
| contents: read | ||
| with: | ||
| base: ${{ github.event_name == 'pull_request' && startsWith(github.base_ref, 'releases/') && github.event.pull_request.base.sha || '' }} |
There was a problem hiding this comment.
| permissions: | ||
| contents: read | ||
| with: | ||
| base: ${{ github.event_name == 'pull_request' && startsWith(github.base_ref, 'releases/') && github.event.pull_request.base.sha || '' }} |
There was a problem hiding this comment.
For CI runs triggered on push, I think the base branch is probably specified by github.ref_name. Please test this out in your personal fork of the repo and link to the resulting workflow log.
https://docs.github.com/en/actions/reference/workflows-and-actions/contexts#github-context
| permissions: {} | ||
|
|
||
| jobs: | ||
| check_api_compatibility: |
There was a problem hiding this comment.
Add this after "build", not before. Reason: if a PR has syntax errors or something, the tools used by "build" will probably return more helpful errors.
| branches: | ||
| - master | ||
| - 'releases/**' | ||
| types: [opened, synchronize, reopened, edited, labeled, unlabeled] |
| docutils = { version = ">=0.21,<0.23", python = ">=3.11" } | ||
| griffe = { version = "^2.0", python = ">=3.11" } |
There was a problem hiding this comment.
Do not specify an upper bound for the package version. If griffe breaks compatibility in a meaningful way, then we will notice when the Renovate PR fails.
| docutils = { version = ">=0.21,<0.23", python = ">=3.11" } | ||
| griffe = { version = "^2.0", python = ">=3.11" } |
There was a problem hiding this comment.
Do you really need to specify the Python version? If so, please specify an upper bound like we do elsewhere in this file. I have seen this cause problems because many packages with Python version markers do not claim support for a future Python 4.0.
| - name: Test compatibility checker | ||
| run: poetry run python -m unittest discover -s .github/scripts -v |
There was a problem hiding this comment.
This project does not use unittest. However this should go away when you delete your workaround script.
This branch has not been deployed
3 participants
I've updated CHANGELOG.md if applicable.No client-facing library behavior changes.What does this Pull Request accomplish?
Adds automated API compatibility checks using Griffe.
masterand release branches.compatibilitydependency group.Fixes #914.
Why should this Pull Request be merged?
Detects potential API-breaking changes before merging, rather than relying solely on internal tests after merge.
The documentation-derived policy covers documented APIs in private implementation modules and public aliases. Each snapshot uses its own documentation, preventing removal of documentation from hiding a previously documented API break.
Source exports and generated API files are unchanged. Export cleanup is a separate public API decision requiring maintainer agreement and would not retroactively repair historical release snapshots.
The
breaking-changelabel allows maintainer-approved compatibility findings without hiding annotations. Operational failures remain fatal.What testing has been done?
DeviceCollection.device_namesremoval.nidaqmx.Taskand gRPC session-option aliases while implementation classes remain.:members:selections and:undoc-members:behavior.1.6.0and the current upstream base passed.1.3.0completed with five expected compatibility findings.origin/masterwas merged without conflicts; all 35 tests and lockfile validation passed afterward.Hardware-dependent regression tests were not run because this change affects CI tooling, not library runtime behavior.