plotly / dash-sample-apps

Open-source demos hosted on Dash Gallery
https://dash.gallery
MIT License
3.13k stars 3.03k forks source link

[plotly] [documentation] use connection pool for mapd example #363

Open michaelbabyn opened 4 years ago

michaelbabyn commented 4 years ago

App name

dash-mapd-demo

Description of antipattern

Currently a new db connection is being made in every callback, and sometimes multiple times per callback. We should implement a connection pool (using sqlalchemy is probably easiest) and use that instead to improve performance and to demonstrate best practices.

ycaokris commented 4 years ago

Did an initial try out on whether connection to mapd database could be passed into sqlalchemy create_engine(), in order for construction pool, unfortunately it fails because dialect of mapd isn't compatible with sqlalchemy.

Here's standard connection with pymapd client:

# standard-way of connection with pymapd client
from pymapd import connect

connection_string = "mapd://mapd:HyperInteractive@{}/mapd".format(DB_HOST)
con = pymapd.connect(connection_string)
print(con)
Connection(mapd://mapd:***@***/mapd?protocol=binary)

Below is snippet:

from sqlalchemy import create_engine
# Pass the same connection uri into sqlalchemy engine
# For connection to localhost, change username to admin
connection_string = "mapd://mapd:HyperInteractive@{}/mapd?".format(DB_HOST)

# Create a connection engine
engine = create_engine(connection_string, pool_size=20, max_overflow=0)
print(engine)
c_query = "SELECT * FROM flights_2008_10k LIMIT 1"
res = engine.execute(c_query)

And triggered a direct traceback:

Engine(mapd://mapd:***@****/mapd)

/Users/yicao/PycharmProjects/dash-sample-apps/venv/lib/python3.7/site-packages/sqlalchemy/engine/default.py:385: SAWarning: Exception attempting to detect unicode returns: DBAPIError('(pymapd.exceptions.Error) Exception: Non-empty LogicalValues not supported yet')
  "detect unicode returns: %r" % de

This DBAPI error is raised primarily due to lack of implementation of a sqlalchemy dialect for Mapd driver.(See dialect supported by sqlalchemy)

I've looked through communities who has been working on similar implementation, this thread seems to support a working solution with most-updated mapD driver and Omnisci database, I tested it out with local-installed instance and got no luck.

Connection(omnisci://admin:***@localhost:6274/omnisci?protocol=binary)
Engine(mapd://admin:***@localhost:6274/omnisci?protocol=binary)
/Users/yicao/PycharmProjects/dash-sample-apps/venv/lib/python3.7/site-packages/sqlalchemy/engine/default.py:385: SAWarning: Exception attempting to detect unicode returns: DBAPIError('(pymapd.exceptions.Error) Exception: Non-empty LogicalValues not supported yet')
  "detect unicode returns: %r" % de

Closely following up this thread and look to see whether there's a workaround.

Another limitation is that I couldn't find a complete database connection log inside this docker instance. That means it'll be hard to test out with a self-built connection pool for this database. For a best-practice app, commonly-used db which has connection pool support (e.g. postgres or snowflake ) might be a better option.

lundstrj commented 4 years ago

@AirballClaytonCalvin this one might be a bit more interesting to dig into.

AirballClaytonCalvin commented 4 years ago

Small typo on the section for setting up an Omnisci Test DB https://github.com/plotly/dash-mapD-demo/blob/master/docker/README.md

Current code -> docker build -t omnisc_db . Should be -> docker build -t omnisci_db .

Also, installing Google Cloud SDK is required.

..

I now have this running locally and will do a further code review.