Skip to content
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

✨ update to PyO3 0.23, add 3.13 support #22

Merged
merged 8 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12']
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13']
steps:
- uses: actions/checkout@v4
with:
Expand All @@ -43,7 +43,7 @@ jobs:
runs-on: windows-latest
strategy:
matrix:
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12']
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13']
steps:
- uses: actions/checkout@v4
with:
Expand All @@ -68,7 +68,7 @@ jobs:
runs-on: macos-latest
strategy:
matrix:
python-version: [ '3.8', '3.9', '3.10', '3.11', '3.12' ]
python-version: [ '3.8', '3.9', '3.10', '3.11', '3.12', '3.13']
steps:
- uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -109,7 +109,7 @@ jobs:
rust-toolchain: nightly
target: ${{ matrix.target }}
manylinux: auto
args: --release --out dist --interpreter 3.8 3.9 3.10 3.11 3.12
args: --release --out dist --interpreter 3.8 3.9 3.10 3.11 3.12 3.13
- name: Upload wheels
uses: actions/upload-artifact@v3
with:
Expand Down
7 changes: 5 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,18 @@ name = "_python_genshin_artifact"
crate-type = ["cdylib"]

[dependencies]
pyo3 = { version = "0.20.3", features = ["anyhow"] }
# TODO it would be very nice to remove the "py-clone" feature as it can panic,
# but needs a bit of work to make sure it's not used in the codebase
# see https://pyo3.rs/v0.23.3/migration.html#pyclone-is-now-gated-behind-the-py-clone-feature
pyo3 = { version = "0.23", features = ["anyhow", "py-clone"] }
mona_wasm = { path = "genshin_artifact/mona_wasm" }
mona = { path = "genshin_artifact/mona_core" }
mona_generate = { path = "genshin_artifact/mona_generate" }
num = "0.4"
serde="1.0"
serde_json = "1.0"
anyhow = "1.0"
pythonize = "0.20.0"
pythonize = "0.23"
bincode = "1.3.3"

[features]
Expand Down
69 changes: 69 additions & 0 deletions docs/PyO3_v0.23.0_upgrade.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# PyO3 v0.23.0 Upgrade

## Phase out the usage of GIL-refs

According to the PyO3 0.21.0 relase note,

> This release introduces a substantial new direction for PyO3's API. The `Bound<'py, T>` smart pointer type
> has been added that replaces "GIL Refs" such as `&'py PyAny` and `&'py PyList` with smart-pointer forms
> `Bound<'py, PyAny>` and `Bound<'py, PyList>`. This new smart pointer brings ownership out of PyO3's internals
> and into user control. This has been done for sake of both performance and soundness.

Thus, the usage of `.as_ref(py)` needs to be phased out and replaced by `.bind(py)`:

```diff
pub fn __repr__(&self, py: Python) -> PyResult<String> {
- let set_name = self.set_name.as_ref(py).to_str()?;
- let slot = self.slot.as_ref(py).to_str()?;
- let main_stat = self.main_stat.0.as_ref(py).to_str()?;
+ let set_name = self.set_name.bind(py).to_str()?;
+ let slot = self.slot.bind(py).to_str()?;
+ let main_stat = self.main_stat.0.bind(py).to_str()?;
let main_stat_value = self.main_stat.1;
```

## Use `Bound<T>` in method arguments and return type

Explicitly use `Bound<'py, T>` in the return type of `__dict__`:

```diff
#[getter]
- pub fn __dict__(&self, py: Python) -> PyResult<PyObject> {
+ pub fn __dict__<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
let dict = PyDict::new(py);
- let name_str = self.name.as_ref(py).to_str()?;
+ let name_str = self.name.bind(py).to_str()?;
dict.set_item("name", name_str)?;
if let Some(config) = &self.config {
- dict.set_item("config", config.as_ref(py))?;
+ dict.set_item("config", config.bind(py))?;
} else {
dict.set_item("config", py.None())?;
}
- Ok(dict.into())
+ Ok(dict)
}
```

Also apply `Bound<T>` to method argument:

```diff
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -25,7 +25,7 @@ use crate::applications::output::transformative_damage::PyTransformativeDamage;
import_exception!(json, JSONDecodeError);

#[pymodule]
-fn _python_genshin_artifact(py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn _python_genshin_artifact(py: Python<'_>, m: &Bound<PyModule>) -> PyResult<()> {
m.add("JSONDecodeError", py.get_type::<JSONDecodeError>())?;
m.add_function(wrap_pyfunction!(get_damage_analysis, m)?)?;
m.add_function(wrap_pyfunction!(get_transformative_damage, m)?)?;
```

## References

- [PyO3 Migration Guide](https://pyo3.rs/v0.23.0/migration.html#from-020-to-021)
- [pydantic-core#1222 - Upgrade to PyO3 0.21 beta](https://github.com/pydantic/pydantic-core/pull/1222/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542)
- [pydantic-core#1556 - Upgrade to PyO3 0.23 (minimal)](https://github.com/pydantic/pydantic-core/pull/1556/files)
- [pydantic-core#1450 - Upgrade to PyO3 0.23 head (WIP)](https://github.com/pydantic/pydantic-core/pull/1450/files)
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ classifiers = [
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
'Programming Language :: Python :: 3.13',
"Intended Audience :: Developers",
"Intended Audience :: Information Technology",
"Intended Audience :: System Administrators",
Expand Down
4 changes: 2 additions & 2 deletions src/applications/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub fn get_damage_analysis(calculator_config: PyCalculatorConfig) -> PyResult<Py
let artifact_config_interface: Option<ArtifactConfigInterface> =
if let Some(artifact_config) = calculator_config.artifact_config {
Python::with_gil(|py| {
depythonize(artifact_config.as_ref(py))
depythonize(artifact_config.bind(py))
.map_err(|err| anyhow!("Failed to deserialize artifact config: {}", err))
})?
} else {
Expand Down Expand Up @@ -117,7 +117,7 @@ pub fn get_transformative_damage(
let artifact_config_interface: Option<ArtifactConfigInterface> =
if let Some(artifact_config) = calculator_config.artifact_config {
Python::with_gil(|py| {
depythonize(artifact_config.as_ref(py))
depythonize(artifact_config.bind(py))
.map_err(|err| anyhow!("Failed to deserialize artifact config: {}", err))
})?
} else {
Expand Down
35 changes: 18 additions & 17 deletions src/applications/input/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ impl PyArtifact {
}

pub fn __repr__(&self, py: Python) -> PyResult<String> {
let set_name = self.set_name.as_ref(py).to_str()?;
let slot = self.slot.as_ref(py).to_str()?;
let main_stat = self.main_stat.0.as_ref(py).to_str()?;
let set_name = self.set_name.bind(py).to_str()?;
let slot = self.slot.bind(py).to_str()?;
let main_stat = self.main_stat.0.bind(py).to_str()?;
let main_stat_value = self.main_stat.1;
Ok(format!(
"PyArtifact(set_name='{}', slot='{}', level={}, star={}, main_stat=({}, {}), id={})",
Expand All @@ -61,25 +61,25 @@ impl PyArtifact {
}

#[getter]
pub fn __dict__(&self, py: Python) -> PyResult<PyObject> {
pub fn __dict__<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
let dict = PyDict::new(py);
dict.set_item("set_name", self.set_name.as_ref(py))?;
dict.set_item("slot", self.slot.as_ref(py))?;
dict.set_item("set_name", self.set_name.bind(py))?;
dict.set_item("slot", self.slot.bind(py))?;
dict.set_item("level", self.level)?;
dict.set_item("star", self.star)?;
let sub_stats_pylist = PyList::new(
py,
self.sub_stats.iter().map(|(s, v)| {
let stat_str = s.as_ref(py).to_str().unwrap();
let stat_str = s.bind(py).to_str().unwrap();
(stat_str, *v)
}),
);
)?;
dict.set_item("sub_stats", sub_stats_pylist)?;
let main_stat_tuple = (self.main_stat.0.as_ref(py), self.main_stat.1);
let main_stat_tuple = (self.main_stat.0.bind(py), self.main_stat.1);
dict.set_item("main_stat", main_stat_tuple)?;
dict.set_item("id", self.id)?;

Ok(dict.into())
Ok(dict)
}
}

Expand All @@ -88,7 +88,7 @@ impl TryInto<MonaArtifact> for PyArtifact {

fn try_into(self) -> Result<MonaArtifact, Self::Error> {
let name: ArtifactSetName = Python::with_gil(|py| {
let _string: &PyString = self.set_name.as_ref(py);
let _string: &Bound<'_, PyString> = self.set_name.bind(py);
depythonize(_string).map_err(|err| {
let serialized_data = format!("{:?}", _string);
anyhow!(
Expand All @@ -100,7 +100,7 @@ impl TryInto<MonaArtifact> for PyArtifact {
})?;

let slot: ArtifactSlotName = Python::with_gil(|py| {
let _string: &PyString = self.slot.as_ref(py);
let _string: &Bound<'_, PyString> = self.slot.bind(py);
depythonize(_string).map_err(|err| {
let serialized_data = format!("{:?}", _string);
anyhow!(
Expand All @@ -112,8 +112,8 @@ impl TryInto<MonaArtifact> for PyArtifact {
})?;

let main_stat_name: StatName = Python::with_gil(|py| {
let main_stat = self.main_stat.0.as_ref(py);
depythonize(self.main_stat.0.as_ref(py)).map_err(|err| {
let main_stat = self.main_stat.0.bind(py);
depythonize(self.main_stat.0.bind(py)).map_err(|err| {
let serialized_data = format!("{:?}", main_stat);
anyhow!(
"Failed to deserialize main stat into mona::artifacts::StatName: {}. Serialized data: \n{}",
Expand All @@ -127,8 +127,8 @@ impl TryInto<MonaArtifact> for PyArtifact {
self.sub_stats
.iter()
.map(|s| {
let sub_stats = s.0.as_ref(py);
let name: Result<StatName, anyhow::Error> = depythonize(s.0.as_ref(py)).map_err(|err| {
let sub_stats = s.0.bind(py);
let name: Result<StatName, anyhow::Error> = depythonize(s.0.bind(py)).map_err(|err| {
let serialized_data = format!("{:?}", sub_stats);
anyhow!(
"Failed to deserialize sub stats into mona::artifacts::StatName: {}. Serialized data: \n{}",
Expand Down Expand Up @@ -166,7 +166,8 @@ mod tests {
pyo3::prepare_freethreaded_python();
Python::with_gil(|py| {
let module = PyModule::import(py, "python_genshin_artifact.enka.artifacts")?;
let artifacts_name_map = module.getattr("artifacts_name_map")?.downcast::<PyDict>()?;
let artifacts_name_map = module.getattr("artifacts_name_map")?;
let artifacts_name_map = artifacts_name_map.downcast::<PyDict>()?;
for (_, value) in artifacts_name_map.iter() {
let artifacts_name_str = value.extract::<String>()?;
let res: Result<ArtifactSetName, anyhow::Error> = depythonize(&value).context(
Expand Down
17 changes: 9 additions & 8 deletions src/applications/input/buff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ pub struct PyBuffInterface {
#[pymethods]
impl PyBuffInterface {
#[new]
#[pyo3(signature = (name, config=None))]
pub fn py_new(name: Py<PyString>, config: Option<Py<PyDict>>) -> PyResult<Self> {
Ok(Self { name, config })
}

pub fn __repr__(&self, py: Python) -> PyResult<String> {
let name = self.name.as_ref(py).to_str()?;
let name = self.name.bind(py).to_str()?;
let config_repr = match &self.config {
Some(config) => config.as_ref(py).repr()?.to_str()?.to_string(),
Some(config) => config.bind(py).repr()?.to_str()?.to_string(),
None => "None".to_string(),
};
Ok(format!(
Expand All @@ -38,16 +39,16 @@ impl PyBuffInterface {
}

#[getter]
pub fn __dict__(&self, py: Python) -> PyResult<PyObject> {
pub fn __dict__<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
let dict = PyDict::new(py);
let name_str = self.name.as_ref(py).to_str()?;
let name_str = self.name.bind(py).to_str()?;
dict.set_item("name", name_str)?;
if let Some(config) = &self.config {
dict.set_item("config", config.as_ref(py))?;
dict.set_item("config", config.bind(py))?;
} else {
dict.set_item("config", py.None())?;
}
Ok(dict.into())
Ok(dict)
}
}

Expand All @@ -56,7 +57,7 @@ impl TryInto<MonaBuffInterface> for PyBuffInterface {

fn try_into(self) -> Result<MonaBuffInterface, Self::Error> {
let name: BuffName = Python::with_gil(|py| {
let _string: &PyString = self.name.as_ref(py);
let _string: &Bound<'_, PyString> = self.name.bind(py);
depythonize(_string).map_err(|err| {
let serialized_data = format!("{:?}", _string);
anyhow!(
Expand All @@ -69,7 +70,7 @@ impl TryInto<MonaBuffInterface> for PyBuffInterface {

let config: BuffConfig = if let Some(value) = self.config {
Python::with_gil(|py| {
let _dict: &PyDict = value.as_ref(py);
let _dict: &Bound<'_, PyDict> = value.bind(py);
depythonize(_dict).map_err(|err| {
let serialized_data = format!("{:?}", _dict);
anyhow!(
Expand Down
14 changes: 7 additions & 7 deletions src/applications/input/calculator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,23 @@ impl PyCalculatorConfig {
}

#[getter]
pub fn __dict__(&self, py: Python) -> PyResult<PyObject> {
pub fn __dict__<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
let dict = PyDict::new(py);
dict.set_item("character", self.character.__dict__(py)?)?;
dict.set_item("weapon", self.weapon.__dict__(py)?)?;
let buffs = self
.buffs
.iter()
.map(|b| b.__dict__(py))
.collect::<Result<Vec<PyObject>, PyErr>>()?;
dict.set_item("buffs", PyList::new(py, buffs))?;
.collect::<Result<Vec<Bound<PyDict>>, PyErr>>()?;
dict.set_item("buffs", PyList::new(py, buffs)?)?;
let artifacts = self
.artifacts
.iter()
.map(|ar| ar.__dict__(py))
.collect::<Result<Vec<PyObject>, PyErr>>()?;
dict.set_item("artifacts", PyList::new(py, artifacts))?;
if let Some(artifact_config) = self.artifact_config.as_ref().map(|c| c.as_ref(py)) {
.collect::<Result<Vec<Bound<PyDict>>, PyErr>>()?;
dict.set_item("artifacts", PyList::new(py, artifacts)?)?;
if let Some(artifact_config) = self.artifact_config.as_ref().map(|c| c.bind(py)) {
dict.set_item("artifact_config", artifact_config)?;
} else {
dict.set_item("artifact_config", py.None())?;
Expand All @@ -79,6 +79,6 @@ impl PyCalculatorConfig {
} else {
dict.set_item("enemy", py.None())?;
}
Ok(dict.into())
Ok(dict)
}
}
Loading
Loading