HyukjinKwon commented on PR #101: URL: https://github.com/apache/spark-connect-rust/pull/101#issuecomment-5630166723
**Evidence / pointers for the review above.** `freethreaded.yml` import flow is correct — pointers: - `crates/pyspark-rs/Cargo.toml:10-12` → `[lib] name = "_pyspark"`, `crate-type = ["cdylib"]` ⇒ Linux artifact `target/release/lib_pyspark.so`. `freethreaded.yml:52` copies it to `/tmp/_pyspark.so` and `:54` `import _pyspark`; Python resolves `PyInit__pyspark`, which matches `#[pymodule] fn _pyspark` (`crates/pyspark-rs/src/lib.rs:92`). ✓ - Standalone import is safe because module init is pure-Rust: the only Python-side import is the client banner, and it's gated behind session connect via `std::sync::Once` (`crates/pyspark-rs/src/session.rs:212-233`), not run at module init. - The "fills a CI gap" rationale checks out: `pyspark-rs` is excluded from `default-members` (`Cargo.toml:44`, with the note at `Cargo.toml:35`), and pyo3 enforces `#[pyclass]: Sync` only when compiling against `Py_GIL_DISABLED` (migration guide "class/thread-safety"). So this is the only job that proves it. Issue 2 — `release.yml` `--zig` fragility, exact pointers: - `release.yml:289-299` "Build abi3 wheel" runs under Python 3.11 (`setup-python` earlier in the job) and installs both maturin and, for zig targets, `ziglang` into that 3.11 env — note `python -m pip install ziglang` at `release.yml:297`. - `release.yml:301-307` "Set up free-threaded interpreters" runs `setup-python` with `3.13t` + `3.14t`; the **last** entry (`3.14t`) becomes the default `python`/`python3` on PATH. - `release.yml:308-321` "Build free-threaded wheels" then runs `maturin build … --zig` (`:317-319`). maturin resolves zig by probing `python -m ziglang`; if it probes the now-default 3.14t (no `ziglang` there) rather than the 3.11 env, `--zig` fails. Both Linux matrix rows are `zig: true` (`release.yml:255-256`), so this is the common path, and it isn't covered by `freethreaded.yml` — it only runs in the release dry-run. Please paste a green dry-run run link, or move the `ziglang` install after the free-threaded `setup-python`. Issue 3 — outdated GIL-default premise (cross-refs #100): - Description: "the GIL only actually stays off once the module declares `gil_used = false` (companion change); with just this PR … CPython re-enables the GIL on import." - `freethreaded.yml:44-49` comment: "The GIL only actually stays off once the module declares gil_used = false (SPARK-59432); until then CPython re-enables it on import, which is expected". - Both are inaccurate for pyo3 0.28 (opt-out default — see the evidence I posted on #100: `pyo3-macros-backend-0.28.3/src/module.rs:452`, `pyo3-0.28.3/CHANGELOG.md:49`, migration guide https://pyo3.rs/v0.28.3/migration#default-to-supporting-free-threaded-python). With #101 alone, the free-threaded wheel already runs GIL-off, so `freethreaded.yml:54` will print `gil_enabled: False` on master too. The step doesn't assert, so no failure — but the comment/description mislead, and the "#100 first, #101 second" ordering is not actually a dependency under 0.28. Minor — interpreter names: `release.yml:317` `--interpreter python3.13t python3.14t` depends on setup-python@v5 exposing those exact `t`-suffixed executables; correct for free-threaded builds, but worth confirming in the dry-run since a name mismatch silently builds nothing for that interpreter. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
