feat: Add support for 3.13 with no GIL #505
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
==========================================
- Coverage 92.89% 92.43% -0.46%
==========================================
Files 227 203 -24
Lines 12203 12334 +131
==========================================
+ Hits 11336 11401 +65
- Misses 867 933 +66 ☔ View full report in Codecov by Sentry. |
blopker
changed the title
| - name: init python venv | ||
| run: | | ||
| python3 -m pip install --upgrade pip | ||
| python3 -m venv env | ||
| source env/bin/activate | ||
| python3 -m pip install pytest | ||
| python3 -m pip install pytest maturin |
There was a problem hiding this comment.
Installing Maturin via pip seems much faster since it's already compiled.
| @@ -86,7 +86,8 @@ class Output: | |||
| @property | |||
| def content(self) -> str: ... | |||
| @property | |||
| def warnings(self) -> List[Warning] | |||
| def warnings(self) -> List[Warning]: ... | |||
| def __init__(self) -> None: ... | |||
There was a problem hiding this comment.
I was getting a linting error here.
| // #[pyclass] | ||
| #[derive(FromPyObject, Clone, Debug)] | ||
| #[pyclass] | ||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
Is there a reason why #[pyclass] was not used? It seems to work well in the new pyo3 version.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new version of pyo3 seems to take care of this now. Curious if this should still be included.
| @@ -275,5 +264,6 @@ fn register(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { | |||
| m.add_function(wrap_pyfunction!(local_loader, m)?)?; | |||
| m.add_function(wrap_pyfunction!(http_loader, m)?)?; | |||
| m.add_function(wrap_pyfunction!(memory_loader, m)?)?; | |||
| m.gil_used(false)?; | |||
There was a problem hiding this comment.
I'm not totally sure where your Python background is, so I'll start high-level. Skip whatever you already know though. :)
The GIL is being removed (currently behind a compile flag for now) from Python. This means Python is going to have real parallelism via threads (Yay!), in contrast to before where only one Python thread could be running at a time (loosely speaking).
Old Python code in general will continue to run without any changes needed since all Python data structures are getting thread-safe locks put on them. The main issue comes from native Python extensions (like this one), which didn't need to be thread safe before. Python itself is unable to verify if a native extension is ready for this new GIL-less world, so native extensions must declare they are not relying on the GIL for correctness. If not all native extensions declare they are not using the GIL in a Python program, the interpreter will fall back to using the GIL.
That is what gil_used(false) is doing here. It's signaling to Python that this extension has been verified to work without the GIL. This flag does not affect behavior in older Python versions though.
However, I did look at the pyo3 docs again, and it looks like the preferred way to declare this when using the pymodule macro is to put #[pymodule(gil_used = false)] in the macro and not imperatively like I did here.
| @@ -0,0 +1,163 @@ | |||
| import concurrent.futures | |||
There was a problem hiding this comment.
Maybe this isn't totally needed since Rust itself is generally thread safe, but it runs fast.
| #[derive(Clone, Debug, Default)] | ||
| pub struct MemoryIncludeLoaderOptions(HashMap<String, String>); | ||
|
|
||
| #[pyclass] | ||
| #[pyclass(frozen)] |
There was a problem hiding this comment.
I've added (frozen) to all the classes to make them immutable. This is the recommended way to keep code thread safe, and may become the default in the future: https://pyo3.rs/main/class.html?highlight=frozen#frozen-classes-opting-out-of-interior-mutability
There was a problem hiding this comment.
I know there are quite a few changes in this PR. Let me know if any of this is interesting or if you'd like me to break it up a bit.
I think there are technically some API breaks in here, but since the type definition on RenderOptions wasn't working, I suspect most people are just using the main API.
Signed-off-by: Bo Lopker <[email protected]>
Signed-off-by: Bo Lopker <[email protected]>
Signed-off-by: Bo Lopker <[email protected]>
Signed-off-by: Bo Lopker <[email protected]>
Signed-off-by: Bo Lopker <[email protected]>
blopker
force-pushed
the
feature/3.13
branch
from
eb689c6 to
aac4f79
Compare
Signed-off-by: Jérémie Drouet <[email protected]>
Signed-off-by: Jérémie Drouet <[email protected]>
jdrouet
force-pushed
the
feature/3.13
branch
from
c282fcc to
fcada0a
Compare
Signed-off-by: Jérémie Drouet <[email protected]>
jdrouet
force-pushed
the
feature/3.13
branch
2 times, most recently
from
da4ebbc to
14f4c91
Compare
Signed-off-by: Jérémie Drouet <[email protected]>
jdrouet
force-pushed
the
feature/3.13
branch
from
14f4c91 to
bc9a6e7
Compare
Signed-off-by: Jérémie Drouet <[email protected]>
Signed-off-by: Jérémie Drouet <[email protected]>
This PR is an attempt to fix #504, by building the Python package with Python 3.13. I've also added support for Python 3.13t (the no GIL version), to make sure this package is ready. The concurrent tests I added are basically just the normal test, but run a bunch in different threads.