fix(sqlite): defer db connection until needed #3127

saulpw · 2021-11-20T00:23:04Z

to .execute() the same table (or a copy) from multiple threads

Closes #64
Closes #1768

- to .execute() the same table (or a copy) from multiple threads

pep8speaks · 2021-11-20T00:23:07Z

Hello @saulpw! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-17 17:20:30 UTC

for more information, see https://pre-commit.ci

jreback

umm at the very minimum this needs tests

can u show an example why this is needed?

github-actions · 2021-11-20T00:44:22Z

Unit Test Results

      38 files       38 suites 1h 15m 38s ⏱️
  7 241 tests   5 453 ✔️ 1 788 💤 0 ❌
29 251 runs 22 173 ✔️ 7 078 💤 0 ❌

Results for commit 5467afa.

♻️ This comment has been updated with latest results.

saulpw · 2021-11-20T00:45:29Z

umm at the very minimum this needs tests

can u show an example why this is needed?

Yes, sorry, this isn't a complete PR, I wanted to submit it early so we could discuss how to clean it up and how to test it.

This is needed because the sqlite connection is created in the thread that gets a reference to the table, and if another thread tries to use that table, sqlite/sqlalchemy gets confused during execute and raises an Exception with a message like no such table: base.dept. ("base" is the default database_name). Doing both actions in the same thread works fine, however.

So this defers the connection creation until it's needed, and also allows the objects to be copied so they can be used in other threads later (new connections will be created for them). This works adequately for my use case.

I'll talk with @cpcloud on Monday about what a proper test for this looks like.

for more information, see https://pre-commit.ci

ibis/backends/base/__init__.py

jreback · 2021-11-24T01:50:15Z

ibis/backends/base/file/__init__.py

@@ -56,7 +56,7 @@ class BaseFileBackend(BaseBackend):

    database_class = FileDatabase

-    def connect(self, path):


you cannot rename any of these methods as they r public

You're right, I'm proposing an API change. (See the toplevel comment I just posted.)

@jreback The top level API here isn't changing. Every backend still has the same connect API by way of threading args and options through do_connect.

sure it is. this is leaking the db_connect into the base class where its not needed. you can move the base changes to the sql layer i think and achieve what you want.

Right, do_connect would be required for new backends or those that want to upgrade. I don't think I see the problem, we'd release a new major version.

@jreback Unfortunately the thing I need is to detach the object construction from the db connection, so I can copy Expr from any arbitrary db table and have them 'just work'. I think it's generally good practice to separate these two anyway--in my experience the constructor/factory should set initial state and not "do" anything, so it can't fail/raise which makes things more complicated for everyone.

saulpw · 2021-11-24T03:25:34Z

Ibis needs a connection to the data source in order to create expressions which contain reference the connection to their data source. I need to be able to copy (or better, pickle) these expressions and call .execute() on those copies from different threads.

So this PR proposes a small API change. Currently, connect() does two things: it creates an object, and it connects the object to the source. With this PR, connect() still creates the object, and the actual source-connecting is moved to do_connect(). This way, Expr derived from these Backends can be copy()ed and each copy can make its own connection on demand.

The connect() constructor turns out to be boilerplate and can be given in BaseBackend so backends don't need to override it anymore. But every backend will need to override do_connect(), which just needs to do the connecting (not the actual object creation).

Here's an example of a connect() as it would be currently:

    def connect(self, ...):
        new_backend = self.__class__()
        new_backend._context = foodb.connect(...)
        return new_backend

and here's the replacement do_connect():

    def do_connect(self, ...):
        self._context = foodb.connect(...)

This is a mechanical transformation and even results in a bit less code. The signature for do_connect should be identical; the args and kwargs are saved and passed through verbatim.

All backends will eventually need a __getstate__ that does not include unreusable connection context. Most Backends can rely on the parent BaseBackend.__getstate__ and possibly include some conveniences that connections can share, like:

    def __getstate__(self):
        r = super().__getstate__()
        r.update(dict(_session_token=self._session_token, _context=None))
        return r

I think maybe there should be an API method for invalidating the connection. Or we could make do_connect a forced invalidation; but maybe it should be allowed to be a no-op in case it wants to always reuse the existing connection?

If we can come up with better names than connect and do_connect, we can use those instead.

- but expressions from different connections to the same DB should be compatible

- add Backend.datasource to identify database apart from connection object

for more information, see https://pre-commit.ci

ibis/tests/expr/test_table.py

for more information, see https://pre-commit.ci

cpcloud

Almost there, small comment about constructor arguments.

cpcloud · 2021-12-09T19:33:03Z

ibis/backends/base/__init__.py

+        Return new client object with saved args/kwargs, having called
+        .reconnect() on it.
+        """
+        new_backend = self.__class__()


Instead of mutating private attributes outside the class, how about taking con_args and con_kwargs as constructor arguments?

So do you mean, having a default BaseBackend() constructor that takes the same *args, **kwargs parameters as its connect()/do_connect() methods would take?

I pushed a change, let me know if this is what you meant.

ibis/tests/expr/test_table.py

cpcloud

LGTM. Over to you @jreback

jreback · 2021-12-11T00:07:08Z

will look soon

jreback

i don't understand why we have to break the world by renaming connect -> do_connect?

ibis/backends/base/__init__.py

jreback · 2021-12-16T23:30:07Z

ibis/backends/base/__init__.py

    @abc.abstractmethod
-    def connect(connection_string, **options):


can we preserve this for backwards compatibilty

If you look a few lines above this, it's still there, so you can continue to use connect() like you always have.

ahh fi that's the case, then just document connect & db_connect a bit

Okay, I'm not clear on what's missing, or where this documentation would go. Backends need to override the abstractmethod do_connect, but that's virtually identical to what they had to do previously with connect (as shown in this comment in the PR). What else do you need?

ibis/backends/base/sql/alchemy/__init__.py

jreback

also can you add a whatsnew note and describe the change

cpcloud · 2021-12-17T14:29:11Z

also can you add a whatsnew note and describe the change

@jreback On a separate note, can you take a look at #2924? This moves us much closer to automate release notes based on commit messages so we don't have to keep asking people to write release notes.

for more information, see https://pre-commit.ci

cpcloud · 2021-12-17T19:22:04Z

This looks good to me. @jreback ?

jreback · 2022-01-04T17:13:45Z

@cpcloud this appears to have broken ibis-bigquery, but they don't test against master. They are a pretty good proxy for other backends.

I am not sure what to do here as there are a lot of breaking changes in the pipelines.

cpcloud · 2022-01-04T17:18:28Z

ibis-bigquery has an unrealistic requirements.txt with unbounded versions for every dependency. This is bound to break at some point and certainly there's no guarantee that master is compatible with dependents.

cpcloud · 2022-01-04T17:21:05Z

Ah looks like setup.py has constraints.

cpcloud · 2022-01-04T17:35:48Z

I believe that if we remove the abstractmethod decorator we can avoid the breakage, and reintroduce it in 3.0.

jreback · 2022-01-04T17:35:51Z

we shouldnt be constantly breaking downstream like this.

cpcloud · 2022-01-04T17:44:36Z

Yep. I don't think we are at "constantly" yet. The goal is to avoid breakage between major releases. A major will always include at least one breaking change.

ibis-project-bot · 2022-01-12T20:34:09Z

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

2.1.0
GitHub release

Your semantic-release bot 📦🚀

fix(sqlite): defer db connection until needed

43fe224

- to .execute() the same table (or a copy) from multiple threads

[pre-commit.ci] auto fixes from pre-commit.com hooks

157fd86

for more information, see https://pre-commit.ci

jreback suggested changes Nov 20, 2021

View reviewed changes

saulpw marked this pull request as draft November 20, 2021 03:14

fix(api): separate backend creation from configuration

b36d41b

saulpw force-pushed the pw21_lazyconnect branch from ae490eb to b36d41b Compare November 23, 2021 01:01

[pre-commit.ci] auto fixes from pre-commit.com hooks

e94f511

for more information, see https://pre-commit.ci

saulpw force-pushed the pw21_lazyconnect branch 2 times, most recently from 3a614a3 to 3193412 Compare November 24, 2021 01:12

test(lazyconnect): explicit None to connect

7d68a5d

saulpw force-pushed the pw21_lazyconnect branch from 3193412 to 7d68a5d Compare November 24, 2021 01:20

jreback suggested changes Nov 24, 2021

View reviewed changes

Saul Pwanson and others added 7 commits November 30, 2021 18:19

chore(lazyconnect): add type information; reconnect()

75406cc

test(expr): expr from multiple DBs should fail ibis-project#64

7d32cf5

- but expressions from different connections to the same DB should be compatible

fix(expr): multiple conns to same db should be compatible

db05cc5

- add Backend.datasource to identify database apart from connection object

[pre-commit.ci] auto fixes from pre-commit.com hooks

405dc85

for more information, see https://pre-commit.ci

fix(sqlite): move instance vars into __init__

d463c0d

[pre-commit.ci] auto fixes from pre-commit.com hooks

3107c6a

for more information, see https://pre-commit.ci

chore(lazyconnect): fix type imports

c474003

saulpw marked this pull request as ready for review December 6, 2021 22:06

saulpw commented Dec 8, 2021

View reviewed changes

ibis/tests/expr/test_table.py Show resolved Hide resolved

Saul Pwanson and others added 3 commits December 8, 2021 18:04

fix(lazyconnect): set internal state to None when copy/reconstituting

4de2ded

fix(lazyconnect): cleaner default datasource string

3603e10

[pre-commit.ci] auto fixes from pre-commit.com hooks

404b034

for more information, see https://pre-commit.ci

cpcloud reviewed Dec 9, 2021

View reviewed changes

refactor(lazyconnect): set instance vars in constructor

ab70fcf

cpcloud mentioned this pull request Dec 10, 2021

Client metadata caching and staleness #716

Closed

cpcloud approved these changes Dec 10, 2021

View reviewed changes

cpcloud added breaking change Changes that introduce an API break at any level feature Features or general enhancements labels Dec 15, 2021

cpcloud requested a review from jreback December 16, 2021 17:34

jreback suggested changes Dec 16, 2021

View reviewed changes

refactor(lazyconnect): update per PR review comments

02891dc

jreback suggested changes Dec 17, 2021

View reviewed changes

Saul Pwanson and others added 2 commits December 17, 2021 09:19

refactor(lazyconnect): rename backend.datasource to .db_identity

eca2830

[pre-commit.ci] auto fixes from pre-commit.com hooks

5ff220e

for more information, see https://pre-commit.ci

cpcloud approved these changes Dec 17, 2021

View reviewed changes

cpcloud mentioned this pull request Dec 17, 2021

ADD REFRESH capability to Postgres Client #1768

Closed

cpcloud requested a review from jreback December 17, 2021 22:33

jreback approved these changes Dec 18, 2021

View reviewed changes

cpcloud merged commit 5467afa into ibis-project:master Dec 18, 2021

ibis-project-bot bot added the released label Jan 12, 2022

cpcloud mentioned this pull request Jan 18, 2022

refactor: move methods implemented in Client to corresponding Backend methods #3017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqlite): defer db connection until needed #3127

fix(sqlite): defer db connection until needed #3127

saulpw commented Nov 20, 2021 •

edited by cpcloud

Loading

pep8speaks commented Nov 20, 2021 •

edited

Loading

jreback left a comment

github-actions bot commented Nov 20, 2021 •

edited

Loading

saulpw commented Nov 20, 2021

jreback Nov 24, 2021

saulpw Nov 24, 2021

cpcloud Nov 24, 2021

jreback Nov 25, 2021

cpcloud Nov 25, 2021

saulpw Nov 29, 2021

saulpw commented Nov 24, 2021 •

edited

Loading

cpcloud left a comment

cpcloud Dec 9, 2021

saulpw Dec 9, 2021

saulpw Dec 9, 2021

cpcloud left a comment

jreback commented Dec 11, 2021

jreback left a comment

jreback Dec 16, 2021

saulpw Dec 16, 2021

jreback Dec 16, 2021

saulpw Dec 17, 2021

jreback left a comment

cpcloud commented Dec 17, 2021 •

edited

Loading

cpcloud commented Dec 17, 2021

jreback commented Jan 4, 2022

cpcloud commented Jan 4, 2022

cpcloud commented Jan 4, 2022

cpcloud commented Jan 4, 2022

jreback commented Jan 4, 2022

cpcloud commented Jan 4, 2022 •

edited

Loading

ibis-project-bot bot commented Jan 12, 2022

		@@ -56,7 +56,7 @@ class BaseFileBackend(BaseBackend):

		database_class = FileDatabase

		def connect(self, path):

		@abc.abstractmethod
		def connect(connection_string, **options):

fix(sqlite): defer db connection until needed #3127

fix(sqlite): defer db connection until needed #3127

Conversation

saulpw commented Nov 20, 2021 • edited by cpcloud Loading

pep8speaks commented Nov 20, 2021 • edited Loading

Comment last updated at 2021-12-17 17:20:30 UTC

jreback left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2021 • edited Loading

Unit Test Results

saulpw commented Nov 20, 2021

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

saulpw commented Nov 24, 2021 • edited Loading

cpcloud 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

cpcloud left a comment

Choose a reason for hiding this comment

jreback commented Dec 11, 2021

jreback 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

jreback left a comment

Choose a reason for hiding this comment

cpcloud commented Dec 17, 2021 • edited Loading

cpcloud commented Dec 17, 2021

jreback commented Jan 4, 2022

cpcloud commented Jan 4, 2022

cpcloud commented Jan 4, 2022

cpcloud commented Jan 4, 2022

jreback commented Jan 4, 2022

cpcloud commented Jan 4, 2022 • edited Loading

ibis-project-bot bot commented Jan 12, 2022

saulpw commented Nov 20, 2021 •

edited by cpcloud

Loading

pep8speaks commented Nov 20, 2021 •

edited

Loading

github-actions bot commented Nov 20, 2021 •

edited

Loading

saulpw commented Nov 24, 2021 •

edited

Loading

cpcloud commented Dec 17, 2021 •

edited

Loading

cpcloud commented Jan 4, 2022 •

edited

Loading