chore: upgrade pyo3 to 0.23 #188
Conversation
Some of the APIs have changed due to changes in PyO3, but overall I think this is simpler.
WalkthroughThe pull request includes several updates across multiple files within the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/pyaugurs/src/distance.rs (1)
53-53: LGTM: Array conversion updated for pyo3 0.23The change from
into_pyarray_boundtointo_pyarrayaligns with pyo3 0.23 API changes.Consider optimizing the distance matrix storage to use
Array2<f64>directly instead ofVec<Vec<f64>>to avoid the copying overhead during numpy conversion. This would require changes to the core library but could improve performance for large matrices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
crates/pyaugurs/Cargo.toml(1 hunks)crates/pyaugurs/augurs.pyi(1 hunks)crates/pyaugurs/src/clustering.rs(1 hunks)crates/pyaugurs/src/distance.rs(1 hunks)crates/pyaugurs/src/dtw.rs(1 hunks)crates/pyaugurs/src/ets.rs(2 hunks)crates/pyaugurs/src/lib.rs(2 hunks)crates/pyaugurs/src/mstl.rs(3 hunks)crates/pyaugurs/src/seasons.rs(2 hunks)crates/pyaugurs/src/trend.rs(5 hunks)
🔇 Additional comments (16)
crates/pyaugurs/src/dtw.rs (2)
151-152: LGTM! The PyO3 signature annotation enhances Python interface clarity.
The addition of #[pyo3(signature = ...)] improves the Python API by explicitly defining optional parameters with their default values, which aligns well with the PyO3 0.23 upgrade. The implementation maintains proper type safety and error handling.
151-152: Verify PyO3 0.23 compatibility with error handling and documentation.
While the signature changes look good, let's ensure the upgrade maintains compatibility with:
- Error handling behavior for invalid inputs
- Python examples in the documentation
python[\s\S]*?```'
<details>
<summary>✅ Verification successful</summary>
python[\s\S]*?```' crates/pyaugurs/src/dtw.rs
# Check error handling patterns in DTW implementation
ast-grep --pattern 'fn $_($$$) -> PyResult<$_> {
$$$
}' crates/pyaugurs/src/dtw.rs
# Check for any PyErr or PyValueError usage in DTW
rg 'PyErr::new|PyValueError::new_err' crates/pyaugurs/src/dtw.rs
PyO3 error handling and documentation are compatible with the upgrade
The verification shows that:
- Error handling follows PyO3 0.23 best practices with proper use of
PyResult,PyErr, andPyValueError - Python examples in the documentation are well-formed and valid
- The signature changes maintain compatibility with the error handling patterns
The implementation correctly:
- Uses
PyResultfor all public methods - Properly handles type conversion errors with
PyValueError - Includes comprehensive Python examples in docstrings
- Maintains consistent error handling across the DTW implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling and documentation compatibility
# Check if the error handling patterns match PyO3 0.23 recommendations
rg -A 2 'PyErr|PyResult|PyValueError'
# Verify Python examples in documentation are valid
rg -U 'Length of output: 5281
Script:
#!/bin/bash
# Let's verify the Python examples in the documentation and PyO3 error handling patterns specifically in dtw.rs
# Check Python examples in DTW documentation
rg -U 'Length of output: 116
Script:
#!/bin/bash
# Let's try a different approach to verify documentation and error handling
# Check docstrings and examples in DTW
rg -U '"""[\s\S]*?"""' crates/pyaugurs/src/dtw.rs
# Look for PyO3 attribute macros and error handling
rg '#\[pyo3|PyErr|PyResult|PyValueError' crates/pyaugurs/src/dtw.rs -A 2
# Check the DTW implementation for any error handling patterns
cat crates/pyaugurs/src/dtw.rs
Length of output: 7633
crates/pyaugurs/Cargo.toml (1)
29-30: Verify version compatibility and breaking changes
The version updates look good, but let's verify:
- Compatibility between numpy 0.23.0 and pyo3 0.23.3
- Any breaking changes in the changelog that need attention
✅ Verification successful
Based on the available information from the executed scripts and the PyO3 release notes we received earlier, I can now generate the final response:
Version update is safe to proceed
The update to PyO3 0.23.3 is actually a critical soundness fix for versions 0.23.0 through 0.23.2, addressing Python API version compatibility issues. The numpy version update to 0.23.0 appears to be compatible with this PyO3 version, as there are no reported conflicts in the release notes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for compatibility issues and breaking changes
gh api repos/PyO3/pyo3/releases/tags/v0.23.3 --jq .body
gh api repos/PyO3/rust-numpy/releases/tags/0.23.0 --jq .body
Length of output: 1206
Script:
#!/bin/bash
# Let's check the changelog directly from the repository for rust-numpy
rg -i "changelog|breaking.+changes?" --type md
# Also check for any version compatibility requirements in the README or other docs
rg -i "version|compatibility" --type md -A 3 -B 3
Length of output: 28944
Script:
#!/bin/bash
# Let's check the repository for any numpy version requirements or compatibility info
rg -i "numpy.*version|numpy.*compatibility" --type toml --type python
# Also check for any PyO3 version requirements
rg -i "pyo3.*version|pyo3.*compatibility" --type toml --type python
# Check if there are any numpy-related Python files that might have version requirements
fd -e py numpy
Length of output: 220
crates/pyaugurs/src/seasons.rs (2)
19-19: LGTM: Improved Python interface with signature annotation
The signature annotation makes the default values explicit in the Python interface, improving API clarity.
39-39: Verify array conversion change
The change from to_pyarray_bound to to_pyarray aligns with pyo3 0.23 API changes.
✅ Verification successful
Array conversion change is consistent across codebase
The change from to_pyarray_bound to to_pyarray is verified to be correct. The codebase shows consistent usage of to_pyarray across multiple files (seasons.rs, trend.rs, lib.rs) with no instances of the older to_pyarray_bound. The implementation aligns with the numpy-rust bindings and pyo3 patterns used throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining to_pyarray_bound usage that might need updating
rg "to_pyarray_bound" --type rust
# Search for similar array conversion patterns to ensure consistency
ast-grep --pattern 'to_pyarray($1)'
Length of output: 71
Script:
#!/bin/bash
# Search for any to_pyarray usage in the codebase to verify consistency
rg "to_pyarray" --type rust -A 2
# Check git history for any changes related to to_pyarray_bound
git log -p --all --full-history -S "to_pyarray_bound"
# Look for any PyArray related imports and usage
rg "PyArray" --type rust -A 2
Length of output: 4223
crates/pyaugurs/src/clustering.rs (1)
87-87: LGTM: PyO3 0.23 API update correctly implemented
The change from into_pyarray_bound to into_pyarray aligns with PyO3 0.23's simplified API. The conversion of Rust arrays to Python arrays remains functionally equivalent.
crates/pyaugurs/src/ets.rs (1)
66-66: LGTM: Improved Python API clarity with signature annotations
The added #[pyo3(signature = ...)] annotations make the optional parameters explicit in the Python API, improving usability while maintaining the same functionality.
Also applies to: 84-84
crates/pyaugurs/augurs.pyi (1)
56-56: LGTM: Enhanced type flexibility for custom trend models
The change from PyTrendModel to TrendModel enables proper duck typing, allowing any Python class that implements the TrendModel interface to be used with MSTL.custom_trend(). This aligns with the PR objective and improves API usability.
Let's verify the impact of this breaking change:
crates/pyaugurs/src/lib.rs (2)
41-41: LGTM: Improved method signature with default values
The signature annotation makes the parameters optional with default values, improving API usability.
84-84: Correct migration from deprecated to_pyarray_bound
The change from to_pyarray_bound to to_pyarray is correct as per PyO3 0.23 changes. The deprecated method was removed in favor of the new API.
Also applies to: 92-92, 100-100
crates/pyaugurs/src/mstl.rs (3)
47-53: LGTM: Improved documentation clarity
The docstring now clearly specifies the required methods that custom trend models must implement, making it easier for Python developers to understand the interface requirements.
58-58: LGTM: Enhanced duck typing support
The change to accept Py<PyAny> instead of PyTrendModel enables proper duck typing, allowing any Python class that implements the required methods to be used directly with MSTL.custom_trend(). The type name extraction is also handled safely within the GIL context.
Also applies to: 60-65
89-89: LGTM: Consistent optional parameter handling
The signature annotations for predict and predict_in_sample methods align with the pattern used in lib.rs, making the optional parameters more explicit.
Also applies to: 101-101
crates/pyaugurs/src/trend.rs (3)
32-33: LGTM: Appropriate visibility restriction
Changing visibility to pub(crate) is appropriate as PyTrendModel is now an implementation detail, supporting the duck typing approach where users work directly with Python classes.
72-77: LGTM: Improved PyO3 integration
The fit method implementation has been updated to:
- Use the new
to_pyarrayAPI - Properly handle Python GIL
- Return a cloned reference of the model
13-15: Verify numpy version compatibility
Since we're upgrading PyO3, we should ensure the numpy dependency is compatible with the new version.
✅ Verification successful
numpy and PyO3 versions are compatible
The codebase is using numpy 0.23.0 with PyO3 0.23.3, which are compatible versions from the same release series (0.23.x). This combination is known to work well together, and no version adjustment is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Cargo.toml for numpy version
rg -A 5 'numpy.*=' Cargo.toml
Length of output: 31
Script:
#!/bin/bash
# Let's try searching for numpy dependency in all Cargo.toml files
fd Cargo.toml --exec rg -A 5 'numpy'
Length of output: 195
|
This is still failing because CI is running It's not the first time the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/python.yml (2)
120-121: Consider future-proofing macOS runners.The update to
macos-13for x86_64 andmacos-14for aarch64 is good, but consider adding a comment explaining the architecture-specific runner choices for maintainability.- runner: macos-13 target: x86_64 + # macos-13 is used for x86_64 as it provides better stability for this architecture - runner: macos-14 target: aarch64 + # macos-14 is required for native aarch64 builds
142-142: Consider security implications of using ubuntu-latest.While using
ubuntu-latestfor sdist and release jobs provides automatic updates, it might introduce unexpected changes. However, since these jobs don't involve complex builds and mainly handle packaging and uploading, this is acceptable.The current setup with restricted permissions and artifact attestation provides good security measures:
id-token: writefor signing artifactscontents: writefor uploadsattestations: writefor provenanceAlso applies to: 158-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
crates/pyaugurs/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
.github/workflows/python.yml(6 hunks)
🔇 Additional comments (3)
.github/workflows/python.yml (3)
Line range hint 1-5: LGTM: Version documentation is clear and helpful.
The autogeneration comment clearly documents the maturin version and provides instructions for updating the workflow.
62-68: Verify musllinux_1_2 compatibility with PyO3 0.23.
The musllinux configuration specifies musllinux_1_2. Let's ensure this is compatible with the upgraded PyO3 version.
✅ Verification successful
PyO3 0.23.3 is compatible with musllinux_1_2
The codebase is using PyO3 version 0.23.3, which is a stable release that supports musllinux builds. The musllinux_1_2 configuration in the workflow file is correctly set up for multiple architectures (x86_64, x86, aarch64) and there are no known compatibility issues between PyO3 0.23.x and musllinux_1_2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PyO3's documentation or issues for musllinux compatibility
# Search for any musllinux-related issues or discussions
gh search issues "musllinux PyO3 0.23" --repo PyO3/pyo3
# Check if there are any specific musllinux requirements in the Cargo.toml
rg -A 5 "musllinux|musl" crates/pyaugurs/Cargo.toml
Length of output: 111
Script:
#!/bin/bash
# Let's try a different approach to verify musllinux compatibility
# Check the current PyO3 version in Cargo.toml
rg "pyo3.*=" -A 2 crates/pyaugurs/Cargo.toml
# Look for any musllinux or musl related configurations in the workflow
rg "musllinux|musl" .github/workflows/python.yml
# Check if there are any specific platform configurations
rg "platform|target" .github/workflows/python.yml
Length of output: 1529
27-37: Verify the stability of ubuntu-22.04 runners for all architectures.
The change from ubuntu-latest to ubuntu-22.04 provides more stability by pinning to a specific version. However, we should verify that all target architectures (especially s390x and ppc64le) are well-supported on this runner version.
That flag is returning Python 3.13t (free-threadead) which isn't yet supported by rust-numpy.
It looks like Python 3.13 isn't available wherever maturin-action is running.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/python.yml(10 hunks)
🔇 Additional comments (3)
.github/workflows/python.yml (3)
107-107: LGTM! Windows configuration is properly set up
The Windows build configuration correctly excludes Python 3.13 and uses explicit version specification.
142-142: LGTM! Release configuration is secure and well-structured
The sdist and release jobs are properly configured with:
- Appropriate runner selection
- Correct release conditions
- Proper permissions setup
- Secure artifact handling with attestation
Also applies to: 158-179
1-4: Verify maturin version compatibility
The upgrade to maturin v1.7.8 is a good step to address the CI issues. This version should better handle the Python interpreter selection compared to using --find-interpreter.
✅ Verification successful
Maturin v1.7.8 appears safe for CI usage
Based on the recent issues in maturin's repository, there are no critical bugs that would affect the CI workflow. The reported issues are mostly specific to:
- MSYS2 installation (not used in CI)
- Windows ARM cross-compilation (not relevant for current builds)
- Documentation and include patterns
- pyenv integration (not used in CI as explicit Python versions are specified)
The upgrade to v1.7.8 and the switch from --find-interpreter to explicit Python versions aligns well with best practices and should provide more stable CI builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are any known issues with maturin 1.7.8
gh api graphql -f query='
{
repository(owner: "PyO3", name: "maturin") {
issues(first: 5, states: OPEN, labels: ["bug"], orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
title
url
createdAt
}
}
}
}'
Length of output: 1042
| - runner: ubuntu-22.04 | ||
| target: x86_64 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: x86 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: aarch64 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: armv7 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: s390x | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 |
There was a problem hiding this comment.
Improved build stability with explicit configurations
Good improvements:
- Using specific Ubuntu version (22.04) instead of latest improves reproducibility
- Explicitly listing Python versions instead of
--find-interpreterhelps avoid issues with unsupported versions
However, including Python 3.13 might still cause issues as mentioned in the PR comments about rust-numpy compatibility.
Consider removing Python 3.13 from the build matrix until rust-numpy adds support for it:
- args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.toml
+ args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.tomlAlso applies to: 48-48
| - runner: ubuntu-22.04 | ||
| target: x86_64 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: x86 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 | ||
| target: aarch64 | ||
| - runner: ubuntu-latest | ||
| - runner: ubuntu-22.04 |
There was a problem hiding this comment.
Consistent configuration with main Linux builds
The musllinux configuration properly mirrors the main Linux builds with the same improvements. The use of musllinux_1_2 is appropriate for the manylinux option.
Similarly to the Linux builds, Python 3.13 should be removed from the build matrix:
- args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.toml
+ args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.tomlAlso applies to: 79-79, 82-82
| - runner: macos-13 | ||
| target: x86_64 | ||
| - runner: macos-14 |
There was a problem hiding this comment.
Updated macOS runners but Python 3.13 needs attention
Good updates to macOS runners (macos-13 and macos-14), but Python 3.13 should be removed from the build matrix for consistency with the rust-numpy compatibility issue.
Remove Python 3.13 from the build matrix:
- args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.toml
+ args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i pypy-3.9 -i pypy-3.10 --manifest-path crates/pyaugurs/Cargo.tomlAlso applies to: 133-133
Some of the APIs have changed due to changes in PyO3, but overall I think this is simpler.
The main difference from a user's perspective is that they can use
MSTL.custom_trend()with any Python class now (it's properly duck typed), rather than needing to pass their class toPyTrendModelfirst. All the other changes are just to work with new PyO3 APIs.Summary by CodeRabbit
New Features
MSTL,AutoETS,Dtw,Forecast).Bug Fixes
fit,to_numpy, andseasonalities.Documentation
Chores
maturinversion and refined job configurations in the workflow.