feat: Add support for 3.13 with no GIL #505

blopker · 2024-12-23T00:06:33Z

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.

Update pyo3
Declare gil_used(false)
Add concurrent tests
Fix type file issue

codecov · 2024-12-23T00:09:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (5311e58) to head (4abfb63).
Report is 110 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

blopker · 2024-12-23T01:04:08Z

.github/workflows/mrml-python-main.yml

      - 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


Installing Maturin via pip seems much faster since it's already compiled.

packages/mrml-python/mrml.pyi

blopker · 2024-12-23T01:05:22Z

packages/mrml-python/mrml.pyi

@@ -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: ...


I was getting a linting error here.

blopker · 2024-12-23T01:06:03Z

packages/mrml-python/src/lib.rs

-// #[pyclass]
-#[derive(FromPyObject, Clone, Debug)]
+#[pyclass]
+#[derive(Clone, Debug)]


Is there a reason why #[pyclass] was not used? It seems to work well in the new pyo3 version.

blopker · 2024-12-23T01:06:42Z

packages/mrml-python/src/lib.rs

-        }
-    }
-}
-


The new version of pyo3 seems to take care of this now. Curious if this should still be included.

blopker · 2024-12-23T01:06:54Z

packages/mrml-python/src/lib.rs

@@ -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)?;


Could you explain me why?

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.

blopker · 2024-12-23T01:08:06Z

packages/mrml-python/tests/test_thread_safety.py

@@ -0,0 +1,163 @@
+import concurrent.futures


Maybe this isn't totally needed since Rust itself is generally thread safe, but it runs fast.

blopker · 2024-12-23T03:06:29Z

packages/mrml-python/src/lib.rs

 #[derive(Clone, Debug, Default)]
 pub struct MemoryIncludeLoaderOptions(HashMap<String, String>);

-#[pyclass]
+#[pyclass(frozen)]


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

Nice, thanks

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: Jérémie Drouet <[email protected]>

blopker changed the title ~~Add support for 3.13 with no GIL~~ feat: Add support for 3.13 with no GIL Dec 23, 2024

blopker marked this pull request as ready for review December 23, 2024 01:03

blopker commented Dec 23, 2024

View reviewed changes

blopker added 5 commits December 23, 2024 07:42

Add support for 3.13 with no GIL

be5a10f

Signed-off-by: Bo Lopker <[email protected]>

Install maturin via pip for speed, use no gil python for testing

5cdd17d

Signed-off-by: Bo Lopker <[email protected]>

Fix deadsnakes action version

2b89060

Signed-off-by: Bo Lopker <[email protected]>

Add 3.13 to release workflow

e364681

Signed-off-by: Bo Lopker <[email protected]>

Better tests, freeze all classes for better thread safety

aac4f79

Signed-off-by: Bo Lopker <[email protected]>

blopker force-pushed the feature/3.13 branch from eb689c6 to aac4f79 Compare December 23, 2024 15:42

jdrouet added 2 commits December 26, 2024 17:41

ci(python): make sure it builds for python 3.10 and 3.13

9885878

Signed-off-by: Jérémie Drouet <[email protected]>

ci(python): use dadsnakes action and python 3.13 for release action

d10d5ba

Signed-off-by: Jérémie Drouet <[email protected]>

jdrouet force-pushed the feature/3.13 branch from c282fcc to fcada0a Compare December 26, 2024 16:50

ci(python): only use deadsnakes action for 3.13

f4be79d

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 December 26, 2024 19:38

ci(python): try to fix python 3.10 install

bc9a6e7

Signed-off-by: Jérémie Drouet <[email protected]>

jdrouet force-pushed the feature/3.13 branch from 14f4c91 to bc9a6e7 Compare December 26, 2024 19:40

jdrouet added 2 commits December 26, 2024 20:49

build(python): set version in pyproject

c0dd110

Signed-off-by: Jérémie Drouet <[email protected]>

ci(python): move back maturin install

4abfb63

Signed-off-by: Jérémie Drouet <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for 3.13 with no GIL #505

feat: Add support for 3.13 with no GIL #505

blopker commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

blopker Dec 23, 2024

blopker Dec 23, 2024

blopker Dec 23, 2024

blopker Dec 23, 2024

blopker Dec 23, 2024

jdrouet Dec 27, 2024

blopker Dec 27, 2024

blopker Dec 23, 2024

blopker Dec 23, 2024

jdrouet Dec 23, 2024

blopker Dec 23, 2024

feat: Add support for 3.13 with no GIL #505

Are you sure you want to change the base?

feat: Add support for 3.13 with no GIL #505

Conversation

blopker commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blopker commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading