Repository navigation
[SPARK-59432][CONNECT][PYTHON] Declare gil_used = false on the _pyspark module - #100
james-willis wants to merge 1 commit into
Conversation
…rk module Opt the pyspark-client-rust extension into free-threaded (no-GIL) CPython. On a free-threaded interpreter (PEP 703) CPython re-enables the GIL process-wide when it imports a PyO3 extension that has not declared gil_used = false. The extension is already thread-safe, so the declaration is truthful; it is a no-op on GIL-enabled builds. Building the free-threaded (cp3xxt) wheels is tracked as a follow-up.
|
@HyukjinKwon This is a follow up from our linked in discussion |
HyukjinKwon
left a comment
There was a problem hiding this comment.
Reviewed by building and inspecting against the pinned toolchain (pyo3 0.28.3). cargo fmt --all --check and cargo check -p pyspark-rs both pass, and the change is safe. One substantive concern about the rationale:
With pyo3 0.28 this is a functional no-op. In pyo3 0.23–0.27 free-threading was opt-in, so gil_used = false was indeed a prerequisite. But this repo pins pyo3 0.28 (Cargo.toml, lockfile 0.28.3), where the default flipped. From the 0.28 migration guide ("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)].
I confirmed this in the macro codegen: pyo3-macros-backend-0.28.3/src/module.rs computes gil_used = options.gil_used.is_some_and(|op| op.value.value), which is false when the attribute is absent, and with_gil_used(false) emits Py_MOD_GIL_NOT_USED. So:
- The plain
#[pymodule]already on master emitsPy_MOD_GIL_NOT_USEDon a free-threaded build — this change produces byte-identical codegen. - On the shipped
abi3-py39wheels,Py_mod_gilis gated behind#[cfg(Py_3_13)](not set under the 3.9 limited API), so neither form emits the slot at all. (The "no-op on GIL-enabled builds" claim is correct.)
Net: across the entire pinned range (>=0.28.0, <0.29), this alters nothing.
Suggestion: the change is harmless and defensible purely as explicit documentation of intent (and a guard against a future pyo3 default change), which is a fine reason to keep a one-liner. But the "Why are the changes needed?" section should be corrected — under pyo3 0.28 this is not a prerequisite for the GIL to stay off; the module already opts in by default. Worth rewording so it doesn't mislead future readers, and so the "step 1 of 2" framing vs. #101 is accurate (the two are actually independent under 0.28).
Good-news corollary: because master already asserts free-threading-safety by default, the substance that matters is whether the extension is genuinely thread-safe. I audited the crate — no static mut, unsendable, RefCell/Cell, Rc, or thread_local; globals use OnceLock<Mutex<…>> / std::sync::Once / AtomicU64; transport uses Arc<Notify> / Arc<AtomicBool> — so it looks structurally sound. The authoritative compile-time enforcement of that (#[pyclass]: Sync) is exactly what #101 adds.
|
Evidence / pointers for the "no-op under pyo3 0.28" finding above. Version pin (this repo):
The change (this PR): pyo3 0.28.3 codegen — the attribute defaults to
Why the default flipped (authoritative):
Why it's fully inert on the shipped wheels: the wheels are pyo3 source links (v0.28.3 tag):
Reproduce locally: Thread-safety audit backing the "structurally sound" note: no matches for |
|
Short note on what to fix in the PR description (the code is fine; the rationale is what's stale):
The "Does this PR introduce any user-facing change? No … no-op on GIL-enabled interpreters" part is accurate and can stay. |
|
Thank you, updated the description. Up to you if we merge or not |
|
Merged to master. |
What changes were proposed in this pull request?
Declare
#[pymodule(gil_used = false)]on the_pysparkmodule incrates/pyspark-rs/src/lib.rs, opting thepyspark-client-rustextension into free-threaded (no-GIL) CPython.This is the first of the two steps identified in 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?
The original justificationfor this PR is struck out below but as Hyukjin points out in his comment, pyo3 flipped the default of this variable to false so this is effectively a no-op! So this PR only serves to make it explicit in the code that no-gil is supported.
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 declaredgil_used = false. A pure-Rust (tonic) Spark Connect client is well positioned to run with the GIL disabled — it pulls in nogrpcio, 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. details in Jira ticket.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 --checkand 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)