Document the Python stub regeneration workflow and pin its toolchain - #1017
Conversation
pybind11-stubgen renders cytnx/cytnx/*.pyi from the built extension, so a floating version can silently reformat the committed stubs on an unrelated regen (enum handling, overload ordering, etc. differ across releases). pybind11 itself is already pinned exactly in [build-system].requires for the same reason; pin pybind11-stubgen the same way in the dev extra so both halves of the stub-generation toolchain are fixed. Co-Authored-By: Claude <noreply@anthropic.com>
cytnx/cytnx/*.pyi is generated from the built extension by tools/generate_stubs.py, not written by hand, and nothing enforces that a pybind/*.cpp binding change comes with a matching stub update. Add a CONTRIBUTING.md section spelling out when regeneration is required, the pinned pybind11/pybind11-stubgen versions that make it reproducible, the build/install/regenerate steps, and how to validate the result with mypy.stubtest. Co-Authored-By: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation to CONTRIBUTING.md on how to regenerate Python type stubs and pins pybind11-stubgen to version 2.5.5 in pyproject.toml. The review feedback suggests adding mypy to the dev dependencies in pyproject.toml since it is used in the stub verification instructions, clarifying the Python version requirement for stub generation, and adjusting the mypy.stubtest command to run from outside the repository root to avoid import shadowing issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a81f26e3e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CONTRIBUTING.md's stub regeneration section instructs contributors to run `python -m mypy.stubtest cytnx.cytnx` to validate the committed stubs, but `pip install --editable '.[dev]'` did not install mypy, so following those instructions in a clean environment failed with "No module named mypy". Co-Authored-By: Claude <noreply@anthropic.com>
"the lowest supported interpreter (Python 3.10+)" reads as "any version >= 3.10", contradicting "lowest" in the same sentence. Name the exact version to remove the ambiguity. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf011d8f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CMakeLists.txt's find_package(pybind11 3.0.0 QUIET) accepts any installed pybind11 >= 3.0.0, and falls back to FetchContent-ing v3.0.1 when none is found on the system. A direct `cmake --preset` configure/build reads pybind11 through that path, bypassing [build-system].requires entirely, so it can silently use a pybind11 other than the 3.0.4 this section pins for reproducible stub annotations. Routing the build through `pip install --editable '.[dev]'` instead goes through PEP 517 build isolation, which resolves the pinned pybind11==3.0.4 from [build-system].requires; this also folds in the previously separate pybind11-stubgen/mypy install step. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1017 +/- ##
===========================================
+ Coverage 32.81% 55.68% +22.86%
===========================================
Files 230 230
Lines 33039 32960 -79
Branches 13769 71 -13698
===========================================
+ Hits 10841 18353 +7512
+ Misses 14904 14587 -317
+ Partials 7294 20 -7274
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
… versions The stub-regeneration section named the exact pinned versions (pybind11 ==3.0.4, pybind11-stubgen ==2.5.5) inline, which duplicates the pins in pyproject.toml and goes stale the moment either is bumped. Point at pyproject.toml instead, and move the "regenerate the stubs when you bump this" reminder into a comment beside each pin (pybind11, pybind11-stubgen, and the requires-python floor, since the stubs are generated with the lowest supported interpreter). The doc now describes which knob lives where without restating the values. Co-Authored-By: Claude <noreply@anthropic.com>
BUILD_PYTHON looked up pybind11 with `find_package(pybind11 3.0.0 QUIET)` and, when none was found, fetched `v3.0.1` via FetchContent. Both paths can bind a pybind11 other than the `==3.0.4` pinned in pyproject.toml's [build-system].requires. Since pybind11 controls the type annotations baked into the extension that tools/generate_stubs.py turns into the committed cytnx/cytnx/*.pyi stubs, that silently regenerates the stubs from the wrong version. Replace the lookup with `find_package(pybind11 3.0.4 REQUIRED)` so the build fails loudly unless the pinned pybind11 (provisioned via build isolation on the `pip install` path) is present. Update the CONTRIBUTING regeneration note, which described the removed FetchContent fallback. Co-Authored-By: Claude <noreply@anthropic.com>
…ild-dir The regeneration recipe forced `--config-settings=build-dir=build/python` so the generator's `build/` auto-discovery would find the freshly built extension. That override is unnecessary: scikit-build-core builds the editable install in a temporary tree regardless (it does not honor the CMake preset's binaryDir), so the pinned build-dir only mattered for discovery. Drop it and instead point `tools/generate_stubs.py --extension` at the extension the editable install just placed on the import path, which is unambiguous and avoids the auto-discovery picking up a stale build under `build/`. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd4d6179e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
find_package(pybind11 3.0.4 REQUIRED) broke the conda build, which provisions pybind11 3.0.3 from conda-forge as a host dependency: CMake rejected 3.0.3 as incompatible with the requested 3.0.4. The exact 3.0.4 pin only needs to hold on the `pip` path that regenerates the committed cytnx/cytnx/*.pyi stubs (enforced by [build-system].requires); every other build merely compiles the extension and ships the already-committed static stubs, so its pybind11 version does not affect stub content. Require pybind11 to be present with a compatible floor (3.0.0) instead of a specific version, which keeps the FetchContent fallback removed while accepting the conda-forge pybind11. Co-Authored-By: Claude <noreply@anthropic.com>
74a05a4 to
3cfcc21
Compare
find_extension() only scanned build/, so the documented regeneration recipe had to pass --extension pointing at the editable install's extension (the editable install builds in a temporary tree, not under build/). Look up the installed cytnx.cytnx first via importlib and fall back to the build/ scan, so `python tools/generate_stubs.py` with no arguments uses exactly the extension the editable install just placed on the import path. Simplify the CONTRIBUTING recipe to drop the --extension incantation accordingly. Co-Authored-By: Claude <noreply@anthropic.com>
find_package(pybind11 3.0.0 REQUIRED) is looser than the ==3.0.4 pin in [build-system].requires because conda-forge currently ships pybind11 3.0.3, which a 3.0.4 requirement would reject. Record a TODO to bump the CMake requirement to 3.0.4 once conda-forge catches up, and add a note beside the pyproject pin so a future pybind11 bump also updates the CMake version string. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b2e8b2236
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e0e8ff1 to
7189b52
Compare
The BuildAndTest job's `cmake --build --target test_main` step may trigger a CMake reconfigure, which re-evaluates find_package(pybind11). Declare pybind11 in the `dev` optional-dependencies so `pip install --editable '.[dev]'` installs it into the environment, and point CMake at it with CMAKE_PREFIX_PATH in the gtest-build step so the reconfigure resolves it. This also covers local `cmake --preset` builds, which likewise need pybind11 outside build isolation. Co-Authored-By: Claude <noreply@anthropic.com>
7189b52 to
97b6b2d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97b6b2d337
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
find_extension() preferred any importable installed cytnx.cytnx before falling back to scanning build/. If the environment has an unrelated cytnx installed (e.g. a released wheel) alongside a freshly built local extension under build/, that unrelated install would win, silently regenerating the committed stubs from the wrong API. Only accept the installed extension when the cytnx package's __path__ includes this checkout's cytnx/ source directory, which holds for an editable install of this checkout (`pip install --editable .`) and rejects everything else, falling through to the build/ scan instead. Resolving cytnx.cytnx via importlib.util.find_spec imports the parent cytnx package as a side effect, and cytnx/__init__.py imports cytnx.cytnx before verifying it has real content (e.g. __version__), so a checkout with no built extension can leave an empty cytnx.cytnx namespace module cached in sys.modules even though the overall import fails. Clear both cytnx and cytnx.cytnx from sys.modules after probing so a later import of the staged build/ extension in main() is not shadowed by that state. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b35aa7a82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
find_installed_extension() unconditionally popped cytnx and cytnx.cytnx from sys.modules after probing, but left every cytnx.* continuation module (Storage_conti, Tensor_conti, ...) cached. Resolving cytnx.cytnx imports the parent cytnx package as a side effect, which runs cytnx/__init__.py to completion whenever a real extension is found (even one this function goes on to reject as unrelated to this checkout), including its `from .Storage_conti import *`-style imports. Those submodules' @add_method decorators patch whichever Storage/ Tensor/... classes were live at that moment; if main() later imports a distinct copy of the correct extension, it would reuse the cached submodules without rerunning the decorators, silently dropping the patched methods from the regenerated stubs. Clear every "cytnx"/"cytnx.*" sys.modules entry, but only when the probed extension is rejected (or unresolvable). Accepting a candidate is only reachable through the same mechanism (an editable-install meta-path finder redirecting cytnx.cytnx to a real compiled extension) that also makes any later import resolve to the identical objects, so leaving that accepted state cached is correct and necessary: clearing it would force Storage_conti et al. to rerun against the same already-patched classes, which raises "method already exists" from add_ovld_method's duplicate-registration guard. Co-Authored-By: Claude <noreply@anthropic.com>
e6069e9 to
a911b4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a911b4ff30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Posted by Claude Code on behalf of @pcchen Reviewed the current head (a911b4f). Docs/build-plumbing only — no committed stubs or runtime behavior change, CI fully green across all 19 checks, and the earlier bot findings were all either fixed in named commits or rejected with documented reasoning. The regeneration recipe and the pin-bump checklist comments in Two small things, neither blocking:
|
pre-commit's end-of-file-fixer enforces exactly one trailing newline, so the extra blank line would be rewritten as churn by the next pre-commit run --all-files in an unrelated PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
find_installed_extension()'s cleanup only removes cytnx/cytnx.* entries from sys.modules; it does not touch pybind11's per-process C++ type registry. If the rejected probe imported a foreign cytnx.cytnx that registered pybind11 types under the same qualified names, a later staged import of a different .so can still raise "generic_type: type ... is already registered!" instead of producing wrong stubs, so a future debugger of that crash isn't misled into looking only at sys.modules. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the review. Both nits from cdda7ec's review body:
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2da7346d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3 participants
Summary
Documents how to regenerate the committed PEP 561 stubs (
cytnx/cytnx/*.pyi) and pins/tightens the toolchain so that regeneration is reproducible. No committed stubs are changed here — only docs, packaging, and the build/generator plumbing.Changes
CONTRIBUTING.md— new "Regenerating the Python type stubs" section: when regeneration is required (anypybind/*.cppbinding change), the two pinned tool versions that make it reproducible, the build → regenerate → validate (mypy.stubtest) steps.Pin the stub-generation toolchain (
pyproject.toml) —pybind11-stubgen ==2.5.5in thedevextra (pybind11 was already==3.0.4in[build-system].requires); a floating stubgen silently reformats the committed stubs on an unrelated regen. Addedmypyto thedevextra, since the docs tell contributors to runpython -m mypy.stubtest. Each pin carries a comment reminding you to regenerate the stubs — and update the CMake version string — when it is bumped.CMake (
CMakeLists.txt) — removed the pybind11 FetchContent fallback (which could fetchv3.0.1when no pybind11 was found). It is nowfind_package(pybind11 3.0.0 REQUIRED). Thepippath provisions the pinned3.0.4via PEP 517 build isolation; conda-forge/system builds just compile and ship the already-committed static stubs, so their pybind11 version cannot drift the.pyi. ATODOtracks bumping the requirement to3.0.4to match the pin once conda-forge ships it (it currently provides3.0.3, which a3.0.4requirement would reject — this was caught by CI on the conda build).tools/generate_stubs.py—find_extension()now prefers the installedcytnx.cytnx(exactly what the editable install just built), falling back to scanningbuild/, sopython tools/generate_stubs.pyneeds no--extensionargument.Testing
pip install --editable '.[dev]'builds cleanly against the relaxed CMake — pybind11 is resolved via build isolation with the FetchContent fallback removed.python tools/generate_stubs.py(no arguments) discovers the installed extension and regenerates all 7.pyifiles with sane output (real bindings present).🤖 Generated with Claude Code