Add maturin and PyO3 compatibility tests - #22
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (18)
WalkthroughPin maturin to 1.13.3, add trybuild as a workspace/crate dev dependency, add Python wheel build and snapshot tests, add Rust trybuild UI compile-pass/fail fixtures for PyO3, and update developer docs and an ExecPlan. ChangesMaturin pinning and PyO3 compatibility testing
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuidePins maturin to 1.13.3 across tooling and build-system, adds Python helpers and tests to snapshot native wheel metadata, introduces Rust trybuild-based PyO3 compile-time UI tests, and documents the compatibility-testing and upgrade workflow for future maturin/PyO3 changes. Flow diagram for Python maturin pin and wheel snapshot testsflowchart TD
Developer --> pytest_maturin_build[pytest tests/test_maturin_build.py]
pytest_maturin_build --> pin_sync_checks[test_maturin_pins_in_sync and related pin checks]
pin_sync_checks --> pyproject_pins[pyproject.toml maturin==1.13.3 pins]
pytest_maturin_build --> wheel_snapshot_test[test_maturin_wheel_build_snapshot]
wheel_snapshot_test --> maturin_cli[maturin build via tests/support/maturin.py]
maturin_cli --> native_wheel[normalized native wheel]
native_wheel --> syrupy_snapshot[snapshot comparison in test_maturin_wheel_build_snapshot]
Flow diagram for Rust trybuild-based PyO3 compile-time UI testsflowchart TD
Developer --> cargo_test_compile_time_ui[cargo test -p stilyagi-pyext compile_time_ui]
cargo_test_compile_time_ui --> trybuild_harness[compile_time_ui in crates/stilyagi-pyext/tests/compile_tests.rs]
trybuild_harness --> pass_fixtures[tests/ui/pass/*.rs]
trybuild_harness --> fail_fixtures[tests/ui/fail/*.rs]
pass_fixtures --> rustc_pass[trybuild compile pass]
fail_fixtures --> rustc_fail[trybuild compile fail with diagnostics]
rustc_fail --> stderr_expectations[*.stderr expectation files]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/execplans/maturin-pyo3-test-upgrade.md`:
- Line 8: Update the document's status line from the non-terminal value to a
terminal one: replace "Status: IN PROGRESS" with "Status: COMPLETED" (or another
terminal value) so the Status header matches the later documented outcomes and
finality in the ExecPlan.
In `@tests/support/maturin.py`:
- Around line 82-87: The installed_maturin_version helper can raise
importlib.metadata.PackageNotFoundError when the maturin module is importable
but distribution metadata is missing; update the installed_maturin_version
function to call im.version("maturin") inside a try/except that catches
im.PackageNotFoundError and returns None in that case so the helper remains
stable (refer to function installed_maturin_version and the im.version call).
- Around line 112-135: In build_native_wheel_artifact, stale wheel files in
out_dir can cause the wheels list check to fail; before running maturin (i.e.,
before the command = [...] and subprocess.run call), remove any existing "*.whl"
files from out_dir by iterating out_dir.glob("*.whl") and calling unlink() on
each so the post-build wheels = sorted(out_dir.glob("*.whl")) reflects only the
newly produced wheel; keep out_dir.mkdir(...) and the rest of the function
unchanged.
- Line 3: Remove the top-level "from __future__ import annotations" and import
pathlib unconditionally (move the pathlib import out of the TYPE_CHECKING block)
so runtime annotations are safe; update installed_maturin_version() to catch
importlib.metadata.PackageNotFoundError around im.version("maturin") and return
None or a sensible fallback when metadata is missing (this hardens it relative
to _maturin_module_available() which uses find_spec); and make
build_native_wheel_artifact() robust by cleaning out_dir before running maturin
(remove or move existing *.whl files) and stop asserting exactly one
wheel—either select the newest/most-recent *.whl or explicitly control the
output filename so stale wheels cannot make the test flaky.
In `@tests/test_maturin_build.py`:
- Line 3: Remove the forbidden future import added in
tests/test_maturin_build.py: delete the line "from __future__ import
annotations" so the file no longer contains that import; ensure no other code
depends on postponed evaluation of annotations (adjust any type hints if
necessary) and run tests to confirm nothing breaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1951aac-f5dc-4fc5-8918-1467a582c459
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlcrates/stilyagi-pyext/Cargo.tomlcrates/stilyagi-pyext/tests/compile_tests.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderrcrates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rscrates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rsdocs/contents.mddocs/developers-guide.mddocs/execplans/maturin-pyo3-test-upgrade.mdpyproject.tomltests/__snapshots__/test_maturin_build.ambrtests/support/maturin.pytests/test_maturin_build.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_maturin_build.py`:
- Around line 29-33: The uv_candidates list currently builds two paths via
f-strings; replace those with pathlib construction using REPOSITORY_ROOT /
".uv-tools" / "uv" and REPOSITORY_ROOT / ".venv" / "bin" / "uv" and pass their
str() to any API that requires a string (e.g., shutil.which). Update the
uv_candidates assignment in tests/test_maturin_build.py to use
str(REPOSITORY_ROOT / ".uv-tools" / "uv") and str(REPOSITORY_ROOT / ".venv" /
"bin" / "uv") instead of the f-strings so all path manipulation uses pathlib.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5b9fc53-411d-454c-aeaf-30f411ab292d
📒 Files selected for processing (5)
crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.stderrcrates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rscrates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.stderrtests/test_maturin_build.py
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +163 to +173 def test_read_maturin_pins_raises_when_no_maturin_pin(tmp_path: pathlib.Path) -> None:
"""read_maturin_pins raises AssertionError when no maturin pin is found."""
pyproject = tmp_path / "pyproject.toml"
pyproject.write_text(
"[dependency-groups]\n"
'dev = ["pytest==8.4.2"]\n'
"[build-system]\n"
'requires = ["setuptools"]\n'
)
with pytest.raises(AssertionError, match="Could not locate"):
read_maturin_pins(tmp_path)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_maturin_build.py (2)
107-124:⚠️ Potential issue | 🟠 MajorAssert the wheel
Tag:from.dist-info/WHEELinstead of hard-coding.
Stop hard-coding"tag": "<platform-tag>";_parse_wheel_header()only parsesGenerator:andRoot-Is-Purelib:, so the tests never validate the actualTag:line in the wheel metadata. ParseTag:from.dist-info/WHEEL, fail if missing, and normalise only the machine-specific suffix before snapshotting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_maturin_build.py` around lines 107 - 124, The test currently hard-codes "tag": "<platform-tag>" in _wheel_build_snapshot which prevents validating the actual Tag: field from the .dist-info/WHEEL; update _wheel_build_snapshot to extract the Tag(s) from the wheel_payload (the same string you pass into _parse_wheel_header), raise an AssertionError if no Tag line is present, normalize any machine-specific suffixes (e.g., platform triples or build IDs) from the parsed tag value(s) before snapshotting, and return that normalized tag under "wheel": {"root_is_purelib": root_is_purelib, "tag": parsed_and_normalized_tag} instead of the hard-coded value. Ensure you reference the existing helpers (_parse_wheel_header and any normalization helper or add a small normalizer) and keep the failure behavior consistent with the existing metadata error handling.
44-53:⚠️ Potential issue | 🟠 MajorPrefer checkout-local
uvbefore ambient PATH in_resolve_uv().In
tests/test_maturin_build.py,_resolve_uv()checks"uv"(PATH) before.uv-tools/...and.venv/..., sotest_maturin_wheel_executes_correctlycan silently use a differentuvthan the repo-provided one.♻️ Prefer the checkout-local executable first
- uv_candidates = ["uv", *(str(path) for path in local_uv_paths)] + uv_candidates = [*(str(path) for path in local_uv_paths), "uv"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_maturin_build.py` around lines 44 - 53, The function _resolve_uv currently lists the ambient "uv" first causing PATH to be preferred over repo-local executables; change the uv_candidates construction in _resolve_uv so local_uv_paths (from local_uv_paths / ".uv-tools" and ".venv") appear before the generic "uv" entry (i.e., build uv_candidates as the local paths first, then "uv") so shutil.which checks checkout-local executables before falling back to PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_maturin_build.py`:
- Around line 1-10: Update the module docstring in tests/test_maturin_build.py
to reference the actual pytest entry points used in this file instead of
"runtime_smoke_test" and "refresh_snapshot" (mention the test name
test_maturin_wheel_build_snapshot and the file's snapshot-update helper), and
replace the POSIX-only ".venv/bin/python" invocation with a portable "python -m
pytest ..." example; also add single-line NumPy-style summaries to the private
helper functions (e.g., _build_native_wheel_artifact, _wheel_build_snapshot and
the local refresh_snapshot helper) and ensure all docstrings follow the numpy
style guide so the wheel-build contract and snapshot workflow are clearly
documented.
---
Outside diff comments:
In `@tests/test_maturin_build.py`:
- Around line 107-124: The test currently hard-codes "tag": "<platform-tag>" in
_wheel_build_snapshot which prevents validating the actual Tag: field from the
.dist-info/WHEEL; update _wheel_build_snapshot to extract the Tag(s) from the
wheel_payload (the same string you pass into _parse_wheel_header), raise an
AssertionError if no Tag line is present, normalize any machine-specific
suffixes (e.g., platform triples or build IDs) from the parsed tag value(s)
before snapshotting, and return that normalized tag under "wheel":
{"root_is_purelib": root_is_purelib, "tag": parsed_and_normalized_tag} instead
of the hard-coded value. Ensure you reference the existing helpers
(_parse_wheel_header and any normalization helper or add a small normalizer) and
keep the failure behavior consistent with the existing metadata error handling.
- Around line 44-53: The function _resolve_uv currently lists the ambient "uv"
first causing PATH to be preferred over repo-local executables; change the
uv_candidates construction in _resolve_uv so local_uv_paths (from local_uv_paths
/ ".uv-tools" and ".venv") appear before the generic "uv" entry (i.e., build
uv_candidates as the local paths first, then "uv") so shutil.which checks
checkout-local executables before falling back to PATH.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f337531-1131-4fb1-84a3-1aa54904efff
📒 Files selected for processing (1)
tests/test_maturin_build.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
38-44:⚠️ Potential issue | 🟡 MinorClarify that
maturinis a hard pin, not a “minimum”.
docs/developers-guide.mdlabelsmaturinas part of “The minimum local setup”, butpyproject.tomlpins it viabuild-system.requires = ['maturin==1.13.3']. Update the wording so it is explicitly a hard pin to1.13.3.Triage:
type:docstyle✏️ Suggested wording
- `maturin` 1.13.3 + `maturin` 1.13.3 (pinned; keep this aligned with `[build-system].requires`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developers-guide.md` around lines 38 - 44, Update the “The minimum local setup” section to clarify that maturin is a hard pinned dependency: change the line listing `maturin 1.13.3` to explicitly state that pyproject.toml hard-pins maturin to version 1.13.3 (e.g., mention `build-system.requires = ['maturin==1.13.3']`) so readers understand this is a strict requirement rather than a minimum version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 498-503: Update the two bullet entries that currently read
`tests/ui/pass/` and `tests/ui/fail/` so they don't duplicate the fixture root
`crates/stilyagi-pyext/tests/ui/`; change them to simply `pass/` and `fail/` (or
alternatively replace the bullets with the full path
`crates/stilyagi-pyext/tests/ui/pass/` and
`crates/stilyagi-pyext/tests/ui/fail/`) so the paths correctly reference the
fixture root and no longer point one directory too deep.
In `@docs/execplans/maturin-pyo3-test-upgrade.md`:
- Around line 116-118: Change the sentence that reads "PyPI lists maturin
`1.13.3`, uploaded on 2026-05-11, as the current release. Stilyagi's `uv.lock`
currently resolves maturin `1.13.1` ..." into a retrospective note: state that
PyPI had `1.13.3` on 2026-05-11 and that, prior to pinning, `uv.lock` resolved
`maturin 1.13.1` (or explicitly prefix with "pre-pin baseline:"), and mention
that the repository now pins maturin to `1.13.3` so the live-state claim is no
longer accurate.
In `@tests/test_maturin_build.py`:
- Around line 223-229: The module-scoped fixture built_wheel currently only
calls _toolchain_available() and may build with a wrong maturin; before calling
_build_native_wheel_artifact, compare _installed_maturin_version() to
_read_expected_maturin_version(REPOSITORY_ROOT) and if they differ call
pytest.skip with a clear message about the pinned maturin mismatch so the
expensive wheel build is avoided when the active maturin version is not the
expected one.
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 38-44: Update the “The minimum local setup” section to clarify
that maturin is a hard pinned dependency: change the line listing `maturin
1.13.3` to explicitly state that pyproject.toml hard-pins maturin to version
1.13.3 (e.g., mention `build-system.requires = ['maturin==1.13.3']`) so readers
understand this is a strict requirement rather than a minimum version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38cd711b-4a78-4cdd-903e-32fa56900ddd
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlcrates/stilyagi-pyext/Cargo.tomlcrates/stilyagi-pyext/tests/compile_tests.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.stderrcrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rscrates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderrcrates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rscrates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.stderrcrates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rscrates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rsdocs/contents.mddocs/developers-guide.mddocs/execplans/maturin-pyo3-test-upgrade.mdpyproject.tomltests/__snapshots__/test_maturin_build.ambrtests/test_maturin_build.py
|
@coderabbitai resume |
Clarify that the local maturin dependency is hard-pinned through `pyproject.toml`, and fix the documented trybuild fixture paths so they are relative to the stated fixture root. Skip the shared native wheel build when the active maturin version does not match the pinned build backend, avoiding an expensive compatibility build under the wrong toolchain.
Parse the actual `Tag` metadata from built wheels and snapshot the normalized value instead of a fixed placeholder. Prefer checkout-local `uv` executables before falling back to the ambient PATH. Refresh the maturin test module guidance and move malformed-input cases into a focused companion test file so each test module stays within the project line limit.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +210 to +230 def _parse_wheel_header(
wheel_payload: str, whl_path: pathlib.Path
) -> tuple[str, str, list[str]]:
generator_match = _GENERATOR_RE.search(wheel_payload)
if generator_match is None:
msg = f"Could not parse maturin generator from WHEEL metadata: {whl_path}"
raise AssertionError(msg)
root_is_purelib: str | None = None
tags: list[str] = []
for line in wheel_payload.splitlines():
if line.startswith("Root-Is-Purelib:"):
root_is_purelib = line.removeprefix("Root-Is-Purelib: ")
elif line.startswith("Tag:"):
tags.append(_normalize_wheel_tag(line.removeprefix("Tag: ")))
if root_is_purelib is None:
msg = "wheel is missing Root-Is-Purelib metadata"
raise AssertionError(msg)
if not tags:
msg = "wheel is missing Tag metadata"
raise AssertionError(msg)
return generator_match.group(1), root_is_purelib, sorted(tags)❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Move Root-Is-Purelib and Tag scanning out of `_parse_wheel_header` so the parser only validates the generator header and delegates line-field extraction. This keeps the maturin wheel helper below the complexity threshold while preserving the existing error paths.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. Developer Documentation |
This comment was marked as resolved.
This comment was marked as resolved.
Update the maturin upgrade execplan so the discovery records the earlier `1.13.1` lock resolution as historical context and points to the final exact `1.13.3` pin introduced by this PR.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== tests/test_maturin_build.py lines 25-45 ==="
sed -n '25,45p' tests/test_maturin_build.py
echo ""
echo "=== invalid_pymodule_arg.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rs
echo ""
echo "=== invalid_pymodule_return.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs
echo ""
echo "=== pymodule_three_args.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rsLength of output: 2891 🏁 Script executed: #!/bin/bash
echo "=== tests/test_maturin_build.py lines 29-60 ==="
sed -n '29,60p' tests/test_maturin_build.pyLength of output: 1740
No further work is required on any of these items. |
Summary
This branch pins maturin to
1.13.3and adds compatibility tests so future maturin and PyO3 upgrades can be validated through focused wheel-build and compile-time checks before the full release path fails.Execplan: docs/execplans/maturin-pyo3-test-upgrade.md
Review walkthrough
1.13.3pin.Validation
make markdownlint: passedmake nixie: passedmake check-fmt: passedmake lint: passedmake typecheck: passedmake test: passed; nextest reported 86 Rust tests passing, and pytest reported 85 Python tests passing with 4 snapshots passingNotes
No issue or roadmap task was identified for this branch. The pull request implements the linked ExecPlan rather than carrying a pre-implementation plan only.
Summary by Sourcery
Pin maturin to a specific version and add compatibility tests and documentation to validate future maturin and PyO3 upgrades.
New Features:
Enhancements:
Documentation:
Tests: