test: Add free-threading test suite and fix thread-safety issues - #3807
test: Add free-threading test suite and fix thread-safety issues#3807FBruzzesi wants to merge 18 commits into
Conversation
|
Apologies in advance for the direct pings @ngoldbaum any chance you can skim through this work to assess the usage of The PR description hopefully has all the details you need to understand the context 🙏🏼 |
Sure, although it may be a week or two.
If you’re using free-threaded 3.14 or newer, warnings are thread-safe by default. If for some reason you want to run pytest-run-parallel on the GIL-enabled build (why?), you can also enable |
|
This PR doesn't touch the docs at all. Should it? The thread-local connection behavior might be surprising. Also I'm not sure if you're planning to do more free-threaded support work, but I also usually suggest adding explicit docs somewhere on thread safety of the library. Focus on mutable global state and mutable objects. Is the mutable global state in the library thread-safe? Mutable global state is things like configuration state, caches initialized at runtime, and global mutable singletons. Are instances of mutable types defined by the library safe to share between threads without external synchronization (e.g. a lock) or coordination (e.g. an algorithm that is safe by construction, like filling disjoint parts of an array with a thread pool)? Note that, for example, mutating NumPy arrays is not thread-safe, so Pandas backed by NumPy is likely susceptible to races for any workflows that tries to write to a data frame. Pandas' arrow backend uses CoW semantics so it doesn't have that problem. My AI model also found a decent number of additional issues and wrote some scripts to trigger them:
from concurrent.futures import ThreadPoolExecutor
import pandas as pd
import narwhals as nw
df = nw.from_native(pd.DataFrame({"g": [1, 1, 2, 2], "i": [2, 1, 2, 1], "v": [10.0, 20.0, 30.0, 40.0]}), eager_only=True)
gb = df.group_by("g")
def worker(i):
for _ in range(300):
expr, expected = (nw.col("v").sum(), [30.0, 70.0]) if i % 2 else (nw.col("v").first(order_by="i"), [20.0, 40.0])
assert (got := gb.agg(expr).sort("g")["v"].to_list()) == expected, got
with ThreadPoolExecutor(8) as tpe:
[f.result() for f in [tpe.submit(worker, i) for i in range(8)]]
from concurrent.futures import ThreadPoolExecutor
import duckdb
import narwhals as nw
con = duckdb.connect()
frames = [nw.from_native(con.sql("select timestamptz '2024-01-01' as t")) for _ in range(8)]
def worker(lf):
for _ in range(50):
lf.collect_schema()
with ThreadPoolExecutor(8) as tpe:
[f.result() for f in [tpe.submit(worker, lf) for lf in frames]] |
| `rel.query(view, sql)` registers `rel` as a view named `view` and runs `sql` on | ||
| `rel`'s own connection (see [relational api]). We prefer it to `duckdb.sql(statement)`, | ||
| which uses the global default connection and cannot see relations from other | ||
| connections (see [using connections in parallel pythonprograms]). |
There was a problem hiding this comment.
Maybe it makes sense to have a helper to go along with this one that accepts two queries and verifies that they're from the same connection. I bet users will start seeing errors coming from queries on two different connections because of this.
Also are there memory consequences for this change? The paragraph below indicates that these temporary views are never dropped. If this cost is per-thread, that might add up a lot.
There was a problem hiding this comment.
You're right that the views are never dropped: I couldn't find a way we can just drop the view once the relation is built.
Regarding the same-connection helper, similarly I couldn't find something exposed by a DuckDBPyRelation that can be used for such task. What we can do is raise a better exception by wrapping the exception into our own catch_duckdb_exception and explain the issue in full detail.
In both cases, I will run the questions by an agent and/or ask in a duckdb forum/github discussion to see if there is something I couldn't find from dir(<various duckdb objects>) and their docs 😅
There was a problem hiding this comment.
should this be using temporary_view_name() instead of "duckdb_settings()"?
There was a problem hiding this comment.
I assume that the fixed name is what we want, a unique name per call would instead add one permanent catalog entry per collect_schema() on a time-zone-aware frame. I added a with _TIME_ZONE_LOCK so that the relation is consumed by .fetchone() inside the lock.
I would not expect that to be the expensive operation in a query.
On a second thought, I think renaming it would at least be less confusing (e.g. __narwhals_time_zone__), since the current name reads like it's referring to the table function.
This one triggers on the GIL-enabled build as well if you add The issue is how this cache gets filled if the I think there can be a race to set that attribute. I'm not totally sure why the attribute is being stashed there instead of being passed along. |
|
Thanks for all your inputs @ngoldbaum - those are really really useful insights 🙏🏼 |
ngoldbaum
left a comment
There was a problem hiding this comment.
I only did a brief scan but the new docs look nice and this look like it's heading in the right direction!
| assert not is_gil_enabled(), ( | ||
| "The GIL was re-enabled, likely by importing an extension module " | ||
| "without free-threading support." | ||
| ) |
There was a problem hiding this comment.
FWIW pytest-run-parallel includes this check out-of-the-box
Description
This PR adds test cases for free-threading support and fixes multiple issues, both in the test suite and in the codebase, surfaced by running the suite concurrently under
pytest-run-parallel.4c4578e sets up the free-threading tests:
tests/free_threading_test.py, a dedicated stress-test module that runs narwhals from many threads (cold-cache dispatch, plugin discovery, shared-Expr.over(...)push-down,Enumdeferred categories, sharedLazyFrameschema, and concurrentnarwhals.sql.table);python-314tCI step that reruns the suite with--parallel-threads=4, a runtime assertion that the GIL is actually off, and a job timeout as a deadlock backstop;thread_unsafe(reason)marker so tests that cannot run concurrently opt out;narwhals.sql: the module-global DuckDB connection and time-zone object are replaced by a per-call cursor (CONN.cursor()), DuckDB's supported per-thread pattern.273bf7d fixes the issues with reused
pytest.raisescontexts: a singlepytest.raises(...)object shared across parametrized cases is mutated concurrently under threads.The remaining two commits target DuckDB issues:
connectionkeyword toscan_csv/scan_parquetso DuckDB reads can go through a caller-providedDuckDBPyConnectioninstead of the process-global default, and uses one connection per thread in the testconftest(relations from different connections cannot be combined, so frames built within one test must share a connection).duckdb.sql(statement), which runs on the process-global default connection, withrel.query(view, sql), which executes on the relation's own connection, for the asof-join, unpivot, and concat operations.top_kis also rewritten from aQUALIFYSQL string to the relational API. Note the trade-off:rel.querycreates a view, so these operations no longer work on read-only DuckDB databases (bug: fetch_rel_time_zone fails with read-only MotherDuck connection #3567), unavoidable until the relational API gains ASOF-join / unpivot support upstream.I left out
warnings.catch_warnings(), which is also not thread safe, with the risk of filtering warnings.What type of PR is this? (check all applicable)