You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "james-willis (via GitHub)" <gi...@apache.org> on 2026/09/11 04:16:21 UTC
[PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
james-willis opened a new pull request, #100:
URL: https://github.com/apache/spark-connect-rust/pull/100
### What changes were proposed in this pull request?
Declare `#[pymodule(gil_used = false)]` on the `_pyspark` module in `crates/pyspark-rs/src/lib.rs`, opting the `pyspark-client-rust` extension into free-threaded (no-GIL) CPython.
This is the first of the two steps identified in [SPARK-59432](https://issues.apache.org/jira/browse/SPARK-59432). The second — adding a free-threaded (`cp313t`/`cp314t`) wheel build leg — is a separate build/packaging change (abi3 and the free-threaded ABI are mutually exclusive) and is left as a follow-up.
### Why are the changes needed?
On a free-threaded interpreter (CPython 3.13t/3.14t, PEP 703), CPython re-enables the GIL process-wide when it imports a PyO3 extension module that has not declared `gil_used = false`. A pure-Rust (tonic) Spark Connect client is well positioned to run with the GIL disabled — it pulls in no `grpcio`, which otherwise re-enables the GIL on import — so this declaration is a prerequisite for the extension to run GIL-off.
The declaration is truthful: the extension is already thread-safe.
- The global tokio runtime is a `OnceLock<Runtime>` (multi-threaded), and every blocking call releases the GIL via `py.detach(|| block_on(...))`.
- Process-global state is behind `OnceLock` / `Mutex` / atomics (e.g. the active-session `OnceLock<Mutex<Option<SparkSession>>>`).
- Every `#[pyclass]` is `Sync` (PyO3's free-threaded requirement, enforced at compile time): the core `SparkSession` is built from `Arc` / `Arc<Mutex<..>>` / atomics, stored `Py<T>` handles are `Send + Sync`, and the response stream's `tonic::Streaming` is `Sync` via `sync_wrapper::SyncWrapper`.
- There are no `Rc` / `RefCell` / `Cell` / `static mut` in the extension.
Behavioral note (not a soundness issue): under free-threading, two Python threads invoking a `&mut self` method on the *same* object instance get a `RuntimeError: Already borrowed` rather than corrupting state, which is the documented PyO3 free-threaded contract.
### Does this PR introduce _any_ user-facing change?
No. The attribute is a no-op on GIL-enabled interpreters. It has no runtime effect until a free-threaded wheel is published — the current wheels are `cp39-abi3`, which are not selectable on a free-threaded interpreter — and that wheel build leg is the tracked follow-up.
### How was this patch tested?
Existing CI (`cargo fmt --all --check` and the workspace build); the attribute is inert on GIL-enabled builds and compiles under the current pyo3 0.28. End-to-end verification that the GIL is actually disabled (`sys._is_gil_enabled() is False`) will be added together with the free-threaded wheel build leg in the follow-up.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #100:
URL: https://github.com/apache/spark-connect-rust/pull/100#issuecomment-5630166503
**Evidence / pointers for the "no-op under pyo3 0.28" finding above.**
Version pin (this repo):
- `Cargo.toml:87` — `pyo3 = { version = "0.28", features = ["extension-module", "abi3-py39"] }`
- `Cargo.lock` → `pyo3 0.28.3`. Semver `"0.28"` = `>=0.28.0, <0.29`, and the opt-out default holds for all of 0.28.x.
The change (this PR): `crates/pyspark-rs/src/lib.rs:91`, `#[pymodule] fn _pyspark(...)` — the **function-style** `#[pymodule]`, so the codegen path is `pymodule_function_impl`, not the `mod`-style one.
pyo3 0.28.3 codegen — the attribute defaults to `false` when omitted:
- `pyo3-macros-backend-0.28.3/src/module.rs:452` (inside `pymodule_function_impl`, defined at `module.rs:437`):
```rust
let gil_used = options.gil_used.is_some_and(|op| op.value.value);
```
`Option::is_some_and` returns `false` when the option is `None` (attribute absent) → `gil_used = false`. (The `mod`-style path at `module.rs:392` is identical.)
- `pyo3-0.28.3/src/impl_/pymodule.rs:209` `with_gil_used`:
```rust
pub const fn with_gil_used(self, gil_used: bool) -> Self {
#[cfg(Py_3_13)] // line 210
{ self.push(ffi::Py_mod_gil,
if gil_used { ffi::Py_MOD_GIL_USED } // 215
else { ffi::Py_MOD_GIL_NOT_USED }) } // 217
#[cfg(not(Py_3_13))] // line 222
{ let _ = gil_used; self } // no slot emitted at all
}
```
⇒ absent attribute ≡ `gil_used = false` ⇒ `Py_MOD_GIL_NOT_USED` on 3.13+ builds — identical to what this PR writes explicitly.
Why the default flipped (authoritative):
- CHANGELOG `pyo3-0.28.3/CHANGELOG.md:49`, under `[0.28.0]`: "Support for free-threaded Python is now opt-out rather than opt-in. [#5564](https://github.com/PyO3/pyo3/pull/5564)"
- Migration guide `pyo3-0.28.3/guide/src/migration.md:8-16` ("Default to supporting free-threaded Python") — rendered: https://pyo3.rs/v0.28.3/migration#default-to-supporting-free-threaded-python :
> "Modules now automatically allow use on free-threaded Python, unless they directly state they require the GIL with `#[pymodule(gil_used = true)]`."
Why it's fully inert on the shipped wheels: the wheels are `cp39-abi3` (`pyproject.toml:73`, `features = ["pyo3/abi3-py39", ...]`). Under the 3.9 limited API the `Py_3_13` cfg is unset, so `with_gil_used` takes the `#[cfg(not(Py_3_13))]` branch at `pymodule.rs:222` and emits **no** `Py_mod_gil` slot regardless of the attribute value.
pyo3 source links (v0.28.3 tag):
- module.rs: https://github.com/PyO3/pyo3/blob/v0.28.3/pyo3-macros-backend/src/module.rs
- pymodule.rs: https://github.com/PyO3/pyo3/blob/v0.28.3/src/impl_/pymodule.rs
- migration.md: https://github.com/PyO3/pyo3/blob/v0.28.3/guide/src/migration.md
Reproduce locally: `cargo check -p pyspark-rs` (passes; the crate builds `cp39-abi3` per `pyproject.toml`). The codegen facts above are read directly from the vendored crate under `~/.cargo/registry/src/*/pyo3-macros-backend-0.28.3/` and `.../pyo3-0.28.3/`.
Thread-safety audit backing the "structurally sound" note: no matches for `static mut`, `unsendable`, `RefCell`/`Cell<`, `Rc<`, or `thread_local` under `crates/pyspark-rs/src/`; process-global state is `session.rs:201-204` (`OnceLock<Mutex<Option<SparkSession>>>`), `session.rs:213` (`std::sync::Once`), `observation.rs:8` (`AtomicU64`); transport uses `Arc<Notify>` / `Arc<AtomicBool>` (`transport.rs:70-71,150-151`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #100:
URL: https://github.com/apache/spark-connect-rust/pull/100#issuecomment-5630192850
Short note on what to fix in the PR description (the code is fine; the rationale is what's stale):
- **"Why are the changes needed?"** — drop the framing that this is a *prerequisite* for GIL-off. Under the pinned pyo3 0.28 (`Cargo.toml:87`), free-threading is opt-out, so the plain `#[pymodule]` already emits `Py_MOD_GIL_NOT_USED`. Reframe as: *making the free-threading declaration explicit rather than relying on pyo3 0.28's opt-out default* (guards against a future default change / downgrade).
- **"the extension to run GIL-off" claim** — the sentence "CPython re-enables the GIL … when it imports a PyO3 extension module that has not declared `gil_used = false`" was true for pyo3 0.23–0.27 (opt-in); it no longer holds on 0.28. Either scope it to those versions or remove it.
- **"first of the two steps … prerequisite" framing vs #101** — under 0.28 the two PRs are independent (the free-threaded wheel in #101 runs GIL-off with or without this change), so "step 1 of 2" overstates the dependency. Soften to "companion, explicitness change."
The "Does this PR introduce any user-facing change? No … no-op on GIL-enabled interpreters" part is accurate and can stay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #100:
URL: https://github.com/apache/spark-connect-rust/pull/100#issuecomment-5630315362
Thank you, updated the description. Up to you if we merge or not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #100:
URL: https://github.com/apache/spark-connect-rust/pull/100#issuecomment-5629406191
@HyukjinKwon This is a follow up from our linked in discussion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #100:
URL: https://github.com/apache/spark-connect-rust/pull/100#issuecomment-5630801191
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #100: [SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module
URL: https://github.com/apache/spark-connect-rust/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org