feat(caching): tie lifetime of cached tables to python refs #9477

coroa · 2024-06-29T19:51:02Z

Description of changes

Makes use of weakref.finalizers to release the cached table automatically when the cached table (or more precisely its .op()) is garbage collected.

In [1]: import pandas as pd

In [2]: import ibis

In [3]: con = ibis.duckdb.connect()

In [4]: df = pd.DataFrame([[0, 1], [2, 3]], columns=["foo", "bar"])

In [5]: noncached = con.create_table("tab", df).mutate(foo=2 * tab.foo)

In [6]: cached = noncached.cache()

In [7]: con.tables
Out[7]: 
Tables
------
- ibis_cache_wvrjoptclzhudglqapz4zusnlu
- tab

In [8]: del cached

In [9]: con.tables
Out[9]: 
Tables
------
- tab

If I understand the nodal expression tree structure correctly, then cached.op() is preserved in all dependent expressions. This PR adds a weakref.finalize(cached.op(), ...) (docs) onto cached.op() which releases the physical cached copy from the databases.

The idempotency of .cache() is also preserved by returning the same table reference.

github-actions · 2024-06-29T19:51:26Z

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

coroa · 2024-06-29T20:13:32Z

~~The bidi dependency (for a bidirectional dictionary) is not necessary anymore with the RefCountedCache implementation in this PR, but quite obviously my attempt at removing it was incomplete.~~

conda/environment.yml

cpcloud

Thanks for digging around in the guts here :)

A couple of questions and requests, but I generally like where this PR is going!

ibis/expr/types/relations.py

ibis/common/caching.py

ibis/expr/types/relations.py

ibis/common/caching.py

ibis/expr/types/relations.py

ibis/common/caching.py

cpcloud · 2024-07-01T12:12:33Z

I will clean up the ruff lints in #9489!

coroa · 2024-07-01T14:02:31Z

I will clean up the ruff lints in #9489!

Once that gets merged, i'll rebase ontop

cpcloud · 2024-07-01T14:23:29Z

@coroa #9489 is merged!

This reverts commit 980f37b.

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>

cpcloud

LGTM!

coroa · 2024-07-01T14:50:12Z

Ok, I rebased ontop of main and removed the DeprecationWarning and updated the .cache() doc-string with another sentence on the explicit and implicit cache eviction.

Only things left on my list of todos are:

Release notes
Check and update user guide

cpcloud · 2024-07-01T14:53:24Z

Looks like there's maybe a bit of DeprecationWarning still hanging around?

cpcloud · 2024-07-01T14:53:57Z

A release note will be generated from the commit message, so no need to do anything there.

coroa · 2024-07-01T14:54:00Z

Looks like there's maybe a bit of DeprecationWarning still hanging around?

Ah yes, i forgot about all the tests i had to change :).

coroa · 2024-07-01T17:10:11Z

Couldn't find any user-guide explanations of the cache'ing mechanism. So i guess the doc-string is the best we have atm. And that is up-to-date.

So, from my side this is good to go.

cpcloud

LGTM!

@jcrist Did you wanna take a look here again?

jcrist

Overall LGTM! This is something we'd talked about doing internally before, nice to see it handled in such a nice way be a new contributor!

jcrist · 2024-07-01T18:37:01Z

ibis/common/caching.py

+
+    def release(self, name: str) -> None:
+        # Could be sped up with an inverse dictionary,
+        # but explicit release is discouraged, anyway


I don't think explicit release is discouraged. I view this as a way to catch users not being explicit, but if users want to be explicit that's also fine. I'd drop this part of the comment.

jcrist · 2024-07-01T18:39:11Z

ibis/common/caching.py

+        raise IbisError(
+            "Key has already been released. Did you call "
+            "`.release()` twice on the same expression?"
+        )


I think this should be a no-op, not an error. Python doesn't error for calling close() explicitly multiple times on a file, we shouldn't either.

Makes sense

ibis/common/caching.py

jcrist · 2024-07-01T18:53:36Z

ibis/backends/duckdb/__init__.py

-    def _clean_up_cached_table(self, op):
-        self.drop_table(op.name)
+    def _clean_up_cached_table(self, name):
+        self.drop_table(name)


I believe this can be deleted to just rely on the base sql backend implementation.

Done, and works

jcrist · 2024-07-01T18:55:42Z

ibis/backends/sql/__init__.py

-    def _clean_up_cached_table(self, op):
-        self.drop_table(op.name)
+    def _clean_up_cached_table(self, name):
+        self.drop_table(name)


We may want to pass force=True to most of these drop_table calls (here and elsewhere) to not error if the table doesn't exist. What we care about is that the table is cleaned up, if some other mechanism already deleted it then I don't think that should be a user-facing error.

jcrist · 2024-07-01T19:05:07Z

ibis/common/caching.py

+
+    def _release(self, key) -> None:
+        entry = self.cache.pop(key)
+        self.finalize(entry.name)


This call may error for a few reasons:

Table doesn't exist (I think we should ignore these always, as noted elsewhere).

Spurious error

Connection is in weird state since python is finalizing

Since most backends will cleanup temp tables automatically (and errors on exit can give a bad impression of a tool), I wonder if want to do something like:

try: self.finalize(entry.name) except Exception: # silence all errors if system is shutting down if not sys.is_finalizing(): raise

This way we can still catch bugs in our code since most release calls won't silence everything, but in the case the system is shutting down and e.g. the network is in a weird state we don't see a mountain of failed-to-release-table errors on exit.

Ok, I added that.

coroa

Thanks for the nice words

coroa · 2024-07-02T09:06:09Z

Unsure what tripped the mssql backend. Unfortunately, have not been able to run those tests locally.

I think I'll need help to address the test failure.

cpcloud · 2024-07-02T09:48:41Z

One transient error and one XPASS, which is a win!

cpcloud · 2024-07-02T09:48:48Z

I'll fix this up and merge!

cpcloud · 2024-07-02T10:19:14Z

Clouds are passing:

…/ibis on  self-releasing-cached-tables is 📦 v9.1.0 via 🐍 v3.10.14 via ❄️   impure (ibis-3.10.14-env)
❯ pytest -m 'bigquery or snowflake' -n 8 --dist loadgroup --snapshot-update -q
bringing up nodes...
.x.x...........................x....s................x..........s...........................x.................xx..........x..x............x..........................x...x........................... [  5%]
............x....................x....................s..................s................................................s......................................................s..x.x.......x...... [ 10%]
x.......x.x..x.........x....x..x.xx....x..........x....................x....................................................x...............................................sssssssssssssssssssss.... [ 16%]
xxxxxxxxxx.x.xx.x.x....x.x.....................................x.............x........x......xx.x.........................x.......x....x.x...........x...x.......................x................... [ 21%]
...x....x..........................x.x....................x........x.......x.x.......x..x......................x..........................xx...............x......x.x..x...x...x......x.............. [ 26%]
.....x.............................x....xx.......x......x..xxxx.xx.x.x.xxx..xx.x...xxxx...x..xx.xxx..x.x..xx...xx.x.x...x......xxx.xxxx.xxx.x...xx.xx.xxxxxx.xx...x.xx.......x.........x.....x....... [ 32%]
xx..........x............................x...x..x..................x....x.....x............x.....x................x..x.....xxx..x.x...xxx.xxxx.xx......xx.x...x..x.....x..............x.............. [ 37%]
..x.x.x.x.............x..x.....x..x.x.......x.x............................x....x..x.x..x...x..x.....x...x..............x...xxxxx....x..................x.........x.......xx..x.x.................... [ 42%]
.....s.............x..............s.x...x....x..............sx..xx.s.x...x..x.......s......x.x......x..........x...................x...........x...........................x..................xx..... [ 48%]
.x.........x.....................x...........................s..........................s....................s..........x................................x..............x.x...x..x...........x...x... [ 53%]
..............xxx.....xx......................x.xx..x......x.x..x...x.x......x............x.x..x.......x..................x.......xx..xx..xx...xxxxxxxxxx...xxx...xxxx.xxxx..x...x.x..x..x.xxx.xx..x. [ 58%]
xxxxx..x.x.x...x..................x..x...x...x....x........x........x.....x.x....x.............x.x.x...xx.....x....x...x.......x...xxxxx................x................x..xx....xx................. [ 64%]
....x...x....x................x.............x.x.....x........x.x.x....................x..........x..xx.....x.......x.x...........x..xx.......x..x................x.....x......xxx...x.x.xxx..xxxxxxxx [ 69%]
xxxx.xxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx..x......................x.........................x.....................x...x........ [ 74%]
....................................................................x.xxxxxx.xx.x....xx.xx....x..x.......x....................x..............x........x.x.xx...x.x...x.....x....x...xx..............x [ 80%]
..xx....x...xx..x..x.....x.....................x..x.................x.x.....x.......x...........x........x....x..x.x.......x.......x................................................................. [ 85%]
..........................................................................................................................................................................s.......................... [ 90%]
.........x.............................................................................................................................................................................x.x........... [ 96%]
......................................................s...ss......................................x...........................................                                                        [100%]
3097 passed, 39 skipped, 552 xfailed in 840.67s (0:14:00)

cpcloud · 2024-07-02T10:20:21Z

@coroa Thanks for pushing this through, great work!

coroa · 2024-07-02T11:51:23Z

Thanks for the quick reviews and comments!

coroa changed the title ~~feat: Tie lifetime of cached tables to python refs~~ feat: tie lifetime of cached tables to python refs Jun 29, 2024

coroa commented Jun 29, 2024

View reviewed changes

conda/environment.yml Outdated Show resolved Hide resolved

cpcloud reviewed Jun 30, 2024

View reviewed changes

cpcloud changed the title ~~feat: tie lifetime of cached tables to python refs~~ feat(caching): tie lifetime of cached tables to python refs Jun 30, 2024

cpcloud added feature Features or general enhancements ux User experience related issues labels Jun 30, 2024

cpcloud reviewed Jun 30, 2024

View reviewed changes

ibis/common/caching.py Show resolved Hide resolved

cpcloud reviewed Jun 30, 2024

View reviewed changes

ibis/expr/types/relations.py Outdated Show resolved Hide resolved

cpcloud reviewed Jun 30, 2024

View reviewed changes

ibis/common/caching.py Show resolved Hide resolved

cpcloud added this to the 9.2 milestone Jul 1, 2024

coroa force-pushed the self-releasing-cached-tables branch from 6d923c9 to 72c91ee Compare July 1, 2024 14:30

coroa and others added 9 commits July 1, 2024 16:43

feat: Tie lifetime of cached tables to python refs

807c814

Update tests

337b8d1

Remove bidi dependency

d5b69eb

Revert "Remove bidi dependency"

2ec9c37

This reverts commit 980f37b.

Remove bidi dependency (take 2)

ac546e0

Update poetry lockfile

50267dd

Apply suggestion

2ee5a37

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>

Protect against races

e10f469

Try to remove tables on atexit

1f79e36

coroa force-pushed the self-releasing-cached-tables branch from 72c91ee to 1f79e36 Compare July 1, 2024 14:43

cpcloud approved these changes Jul 1, 2024

View reviewed changes

Remove deprecation warning from tests

7578128

cpcloud approved these changes Jul 1, 2024

View reviewed changes

jcrist reviewed Jul 1, 2024

View reviewed changes

coroa added 2 commits July 2, 2024 07:49

Make double-release graceful

8c5f43a

Suppress exceptions during interpreter shutdown

860d47b

This comment was marked as duplicate.

Sign in to view

coroa commented Jul 2, 2024

View reviewed changes

coroa added 2 commits July 2, 2024 09:18

refactor(duckdb): Rely on generic sql cache functions

81163e9

ignore exceptions if caching tables do not exist

9b3511d

test(mssql): enable xpassing test

002b330

cpcloud enabled auto-merge (squash) July 2, 2024 10:19

cpcloud merged commit f51546e into ibis-project:main Jul 2, 2024
89 checks passed

coroa deleted the self-releasing-cached-tables branch August 8, 2024 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(caching): tie lifetime of cached tables to python refs #9477

feat(caching): tie lifetime of cached tables to python refs #9477

coroa commented Jun 29, 2024 •

edited

Loading

github-actions bot commented Jun 29, 2024

coroa commented Jun 29, 2024 •

edited

Loading

cpcloud left a comment

cpcloud commented Jul 1, 2024

coroa commented Jul 1, 2024

cpcloud commented Jul 1, 2024

cpcloud left a comment

coroa commented Jul 1, 2024 •

edited

Loading

cpcloud commented Jul 1, 2024

cpcloud commented Jul 1, 2024

coroa commented Jul 1, 2024

coroa commented Jul 1, 2024

cpcloud left a comment

jcrist left a comment

jcrist Jul 1, 2024

jcrist Jul 1, 2024

coroa Jul 2, 2024

jcrist Jul 1, 2024

coroa Jul 2, 2024

jcrist Jul 1, 2024

coroa Jul 2, 2024

jcrist Jul 1, 2024

coroa Jul 2, 2024

This comment was marked as duplicate.

coroa left a comment

coroa commented Jul 2, 2024 •

edited

Loading

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

coroa commented Jul 2, 2024

feat(caching): tie lifetime of cached tables to python refs #9477

feat(caching): tie lifetime of cached tables to python refs #9477

Conversation

coroa commented Jun 29, 2024 • edited Loading

Description of changes

github-actions bot commented Jun 29, 2024

coroa commented Jun 29, 2024 • edited Loading

cpcloud left a comment

Choose a reason for hiding this comment

cpcloud commented Jul 1, 2024

coroa commented Jul 1, 2024

cpcloud commented Jul 1, 2024

cpcloud left a comment

Choose a reason for hiding this comment

coroa commented Jul 1, 2024 • edited Loading

cpcloud commented Jul 1, 2024

cpcloud commented Jul 1, 2024

coroa commented Jul 1, 2024

coroa commented Jul 1, 2024

cpcloud left a comment

Choose a reason for hiding this comment

jcrist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

coroa left a comment

Choose a reason for hiding this comment

coroa commented Jul 2, 2024 • edited Loading

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

cpcloud commented Jul 2, 2024

coroa commented Jul 2, 2024

coroa commented Jun 29, 2024 •

edited

Loading

coroa commented Jun 29, 2024 •

edited

Loading

coroa commented Jul 1, 2024 •

edited

Loading

coroa commented Jul 2, 2024 •

edited

Loading