refactor(backends): clean up resources produced by `memtable` #10055

cpcloud · 2024-09-07T14:14:19Z

Description of changes

PR on top of #10053 to clean up temporary tables produced by ibis.memtable.

The approach is a generalization of the one taken in #10042.

The main caveat here, which I don't think is a blocker is that the following
code will start to fail:

table = ibis.memtable({"a": [1, 2, 3]}, name="foo")
register_memtable(table)

table = None  # very likely to cause `table` op to be GC'd, and thus delete the underlying resource
run_query_against("foo")  # fails because `foo` has been dropped

Issues closed

Closes #10044.

cpcloud · 2024-09-07T14:24:38Z

ibis/backends/exasol/__init__.py

@@ -382,31 +384,27 @@ def create_table(
        else:
            temp_name = name

-        table = sg.table(temp_name, catalog=database, quoted=quoted)
-        target = sge.Schema(this=table, expressions=column_defs)
+        table_expr = sg.table(temp_name, catalog=database, quoted=quoted)


I changed these variable names because table = <not the memtable> causes it to get GC'd. I gave another example of this in the PR description.

cpcloud · 2024-09-07T14:37:12Z

Still need to add at least one cross-backend test to verify the behavior.

cpcloud · 2024-09-09T12:35:24Z

I'll split out the faster memtable checks into a separate PR once #10053 is merged.

cpcloud · 2024-09-09T13:45:13Z

Going to split this out into in-memory versus not-in-memory backends since the former are more important to clean up more aggressively.

…sages

cpcloud · 2024-09-10T12:04:02Z

After hours of trying to understand what is happening with MySQL I was able to get a reproducer:

I can only reproduce this with a Python 3.12 environment.

pytest --randomly-seed=159558151 -m mysql -x --pdb ibis/backends/tests/test_generic.py -vv

Still not sure why executing SQL in a finalizer causes the socket on the connection to get set to None, so I'm going to disable memtable cleanup for MySQL for now. It's fairly low risk in that memtables are created with TEMPORARY, so they'll get clean up when the session ends.

We won't be able to prevent build up of disk space when constructing large memtables in a loop, but we won't keep all the storage around when the session ends.

cpcloud · 2024-09-10T12:09:21Z

I know I said I would break out the in-memory backends into their own PR, but that would require no-op-ing the non-in-memory backends and then changing that code again. That doesn't seem worth the trouble, given the hopefully manageable small number of changes here.

jcrist

A could questions/suggestions around error handling, but otherwise LGTM.

ibis/backends/exasol/__init__.py

ibis/backends/__init__.py

ibis/backends/duckdb/__init__.py

ibis/backends/tests/test_client.py

jcrist

Presuming tests pass, LGTM.

cpcloud · 2024-09-10T15:17:40Z

Running the clouds locally now.

…session dataset usage

cpcloud · 2024-09-10T16:00:48Z

BigQuery is good:

❯ pytest -m bigquery -n auto --dist loadgroup --snapshot-update -q
bringing up nodes...
...........sssssssssssssssssssss.......x.....x.........x............x.............x....x........x.x..xx.....s..x..x..x...x......x...x.s...ssssssssssssssssssssssssssssssssssssssssssssssssssss [  8%]
sssssssssssssssssssssssssssssssssssssssssssssss.........s......x...xx.x.xx........x.....s........................x........x......x...........s................................................ [ 17%]
.x......x.....................x......x.........................x........................................................xx.....x....x.X............x.............xx..x.......x.....xx....x...x [ 26%]
.....xx.......x.x..x.....X.x...x.x....x.....x....x.x...........x......x.xx.x.xXxx...x.x.xxXxx....Xx....x...x........x.........x...x......x..................................x......x.........x [ 35%]
..........x.x...x..xxxxx.xx.x..xx.x.x.........x...x..x.........x...x..........x....x.......x..............x......x....x.............x....x.x............x.............x............x.......... [ 44%]
....xx.........x....x...x.x..x.............xx...x.xx.x.x..xx...xxxx..xxxx...x.....xxxx.xx.....x......xxx.....x.x.........x....x..xx..x....x........x.x...x..............x.......x.x....xx.x... [ 53%]
........x.x..x............x........xx..xx...x..x.....x......x.x.x...x....x......xx...x.......x...x....x.x...x..x..............x..x.......................x........x...x...x.....x.........x... [ 62%]
...x..x......x.......x........x..........x..x..............x.x...x..........x.....x....x..x................xx....xx.....x.....................x.................s.........s................... [ 71%]
.....................................................x.s.xxxx.xxx.x.xx.xxx.x..xx...xxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxx.xxxxxxxxxxxxxxxxxx.xxxxxx................... [ 79%]
.............................x.............................x.....................x.......x........x...............x.x...x.....................x............................................... [ 88%]
..............x..x...................................................xxxxxxxx.xx...........................x...x.......................x..............................................x....... [ 97%]
.................................................                                                                                                                                              [100%]
1663 passed, 128 skipped, 343 xfailed, 5 xpassed in 344.90s (0:05:44)

cpcloud · 2024-09-10T16:01:38Z

Snowflake is good:

❯ pytest -m snowflake -n 8 --dist loadgroup --snapshot-update -q && pytest -m bigquery -n auto --dist loadgroup --snapshot-update -q
bringing up nodes...
.......x.....................s..................x..............x....x.............................x..................x...................x...x........x...x..................x................ [  9%]
............x.....x.......x........x..x.x...............x....................x....................................xx....................x...x..................x...x.........................x [ 19%]
.x.........x.....x.......x..x.........x..................x............................x..........x........x....x.....x......................x..........x.................x............x....... [ 29%]
..x.........x.....x..xx....x...x.......x..............xx..s.................x...............xxx.xx...............x......x..x.......x.x.x.......x......................s........x.............. [ 39%]
.......................s.......s........x.....xx.........xx...x..............x............xx.x......x..................x....xx......x............x.................x....x............x..x..... [ 49%]
x.x.....xx.x......x...xx..xx...x..x....................x..................x...............................................................x..............x.................................xx. [ 59%]
.........................x..xx..xxxx...x.x...x.xx.x.........xx..xx.....x.xxxx.x.xxxxxxxxx.x..xxxxx.x....xxxxx...x...xx.xx..xxx....xxxx..........xxx...x.....x.xx..x......x.................... [ 69%]
..........x..........x.s...x...x.........x..x..xxxxxxxxxxxx.x....................................................x........................x.......x........................................... [ 79%]
.....................x..x...........x........x.....x..x......................x...x........x.............xx...x...x.........x...............x.x.........x..........x....x...xxxxxxxx.x...x....x [ 89%]
.x.xx..........s...s...s.s.................................................................................................................................................................... [ 99%]
..........

jcrist · 2024-09-10T18:43:07Z

Looks like the new test might be a bit flaky on bigquery: https://github.com/ibis-project/ibis/actions/runs/10798011642/job/29950561913

cpcloud · 2024-09-10T18:44:27Z

Blah, okay, looking into it.

Replaces pymysql with mysqlclient, mostly out of frustration with bizarre GC behavior discovered during #10055. I think this is probably a breaking change due to some changes in how types are inferred for JSON, INET and UUID types. BREAKING CHANGE: Ibis now uses the `MySQLdb` driver. You may need to install MySQL client libraries to **build** the extension.

Replaces pymysql with mysqlclient, mostly out of frustration with bizarre GC behavior discovered during ibis-project#10055. I think this is probably a breaking change due to some changes in how types are inferred for JSON, INET and UUID types. BREAKING CHANGE: Ibis now uses the `MySQLdb` driver. You may need to install MySQL client libraries to **build** the extension.

cpcloud commented Sep 7, 2024

View reviewed changes

cpcloud marked this pull request as draft September 7, 2024 14:26

cpcloud force-pushed the cleanup-temp-resources branch 9 times, most recently from fb642a4 to c6c0677 Compare September 8, 2024 13:25

cpcloud added refactor Issues or PRs related to refactoring the codebase performance Issues related to ibis's performance labels Sep 8, 2024

cpcloud force-pushed the cleanup-temp-resources branch 5 times, most recently from 009261a to 837e3d2 Compare September 9, 2024 19:58

cpcloud added 3 commits September 10, 2024 05:39

refactor(backends): clean up memtable resources

4bec625

test: use special mocker assertion methods that give better error mes…

f119e20

…sages

test(flink): memtables are constructed every time

85ac3ad

cpcloud force-pushed the cleanup-temp-resources branch from a4530d1 to 2c5f4dc Compare September 10, 2024 09:47

cpcloud added 2 commits September 10, 2024 08:05

test(memtable): generate a table name instead of a fixed name

ff12db6

test(mysql): acquiesce to the sql gods; live to fight another day

22cec69

cpcloud force-pushed the cleanup-temp-resources branch from 8bc8b6a to 22cec69 Compare September 10, 2024 12:07

cpcloud marked this pull request as ready for review September 10, 2024 12:08

chore(mysql): bring back annoying memtable cleanup

a0d43b5

cpcloud requested review from jcrist and gforsyth September 10, 2024 14:03

jcrist reviewed Sep 10, 2024

View reviewed changes

cpcloud added 4 commits September 10, 2024 10:49

chore: remove redundant test

a287865

chore: make test more robust to interface changes

39dbe02

chore(exasol): remove incorrect flag setting

7bcd327

chore: factor out finalizer registration

191a107

cpcloud requested a review from jcrist September 10, 2024 15:09

cpcloud added 2 commits September 10, 2024 11:11

chore: fix polars

b19cc51

chore: fix pandas signature

06ae2e4

jcrist approved these changes Sep 10, 2024

View reviewed changes

chore: revert test assertion changes

dd2ee70

chore(bigquery): implement _finalize_memtable so we can handle the …

b1aab14

…session dataset usage

cpcloud force-pushed the cleanup-temp-resources branch from a98a0f5 to b1aab14 Compare September 10, 2024 15:32

cpcloud added this to the 9.5 milestone Sep 10, 2024

chore: apparently quoting project id is not allowed

26acf61

cpcloud enabled auto-merge (squash) September 10, 2024 16:03

cpcloud mentioned this pull request Sep 10, 2024

chore(mysql): port to MySQLdb instead of pymysql #10077

Merged

cpcloud merged commit 019cae5 into ibis-project:main Sep 10, 2024
81 checks passed

cpcloud deleted the cleanup-temp-resources branch September 10, 2024 16:29

acuitymd-filip mentioned this pull request Sep 12, 2024

feat(duckdb): restore or add new method for consuming pyarrow.dataset #10082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(backends): clean up resources produced by `memtable` #10055

refactor(backends): clean up resources produced by `memtable` #10055

cpcloud commented Sep 7, 2024

cpcloud Sep 7, 2024

cpcloud commented Sep 7, 2024

cpcloud commented Sep 9, 2024

cpcloud commented Sep 9, 2024

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

jcrist left a comment

jcrist left a comment

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

jcrist commented Sep 10, 2024

cpcloud commented Sep 10, 2024 •

edited

Loading

refactor(backends): clean up resources produced by memtable #10055

refactor(backends): clean up resources produced by memtable #10055

Conversation

cpcloud commented Sep 7, 2024

Description of changes

Issues closed

cpcloud Sep 7, 2024

Choose a reason for hiding this comment

cpcloud commented Sep 7, 2024

cpcloud commented Sep 9, 2024

cpcloud commented Sep 9, 2024

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

jcrist left a comment

Choose a reason for hiding this comment

jcrist left a comment

Choose a reason for hiding this comment

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

cpcloud commented Sep 10, 2024

jcrist commented Sep 10, 2024

cpcloud commented Sep 10, 2024 • edited Loading

refactor(backends): clean up resources produced by `memtable` #10055

refactor(backends): clean up resources produced by `memtable` #10055

cpcloud commented Sep 10, 2024 •

edited

Loading