Db connection issue #100

tweddielin · 2017-12-13T17:00:56Z

Solve previous DB connection switch issue by using app.config['SQLALCHEMY_DATABASE_URI'] and app.config['SQLALCHEMY_BINDS']
Refactor query.py based on the change

houndci-bot · 2017-12-13T17:01:05Z

webapp/query.py


+def get_model_parameters(model_id):


expected 2 blank lines, found 1

houndci-bot · 2017-12-13T17:01:05Z

webapp/controller.py

+    methods=['GET', 'POST'])
+def get_model_detail(model_id):
+    df = query.get_model_parameters(model_id)
+    output=df


missing whitespace around operator

houndci-bot · 2017-12-13T17:01:05Z

webapp/controller.py

@@ -395,7 +392,16 @@ def get_metric_over_time(model_id):
    except Exception as e:
        return jsonify({"sorry": "Sorry, no results! Please try again."}), 500

-
+@app.route(


expected 2 blank lines, found 1

houndci-bot · 2017-12-13T17:01:05Z

webapp/controller.py

-
-# Default DB
-app.config['DB_NAME'] = "cmpd"
+from webapp import db


'webapp.db' imported but unused

jesteria

I know you're not really done with this. I was just curious about what was going on in Tyra-land 😁

jesteria · 2017-12-13T17:36:36Z

webapp/__init__.py


 app = Flask(__name__,instance_relative_config=True)
 app.secret_key = "tyra american top models"
+app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['sfpd']['url']


I'm not really sure, but is it possible to omit this configuration setting? It seems like the project doesn't really have a "default" or "main" database connection, (unless we are in fact using SFPD that way?).

jesteria · 2017-12-13T17:43:22Z

webapp/__init__.py

@@ -3,9 +3,15 @@
 from flask_sqlalchemy import SQLAlchemy
 import flask_login
 import json
+from config import db_dict


This is me getting out of scope, but I'm seeing a lot of code in our projects to map environment and/or file configuration variables to database URIs. The standard I've used in the past is simply to pass database URIs around, in a single setting variable. For frameworks which don't have native support for this URI, there are often simple libraries to add support; and, it's cool that Flask-SQLAlchemy supports these natively.

DATABASE_URI=postgresql://USER@HOST:PORT/DATABASE?PARAMS

Yeah I absolutely agree with you that the config thing is kinda messy or redundant. I don't like that personally too. Maybe we should change it in other PR.

jesteria · 2017-12-13T17:50:08Z

webapp/controller.py

@@ -43,7 +41,6 @@ def testing():
 @app.route('/db_choose/<string:project>', methods=['GET', 'POST'])
 def db_choose(project):
    try:
-        app.config['SQLALCHEMY_DATABASE_URI'] = db_dict[project]['url']
        app.config['DB_NAME'] = project


I don't have a ton of experience with Flask, and so I'm curious – is this app.config dictionary not process-global? Is db_choose meant for something other than for the user to select the database for their session?

I end up using flask-session now.

session['engine'] = project

jesteria · 2017-12-13T17:53:52Z

webapp/query.py

    return model_comments


 def get_model_groups(query_arg):
+    engine = db.get_engine(app, app.config['DB_NAME'])


I wonder if it might not make more sense for the caller (the controller) to pass the database engine to the query method, rather than ask the query method to look up the appropriate database in this shared configuration? At least, that would make these query methods more easily testable. And it makes sense for the controller to be tying the parts together.

yes, I change to pass the database engine to the query method.
In controller.py:

engine = db.get_engine(app, session['engine']) query_arg = {'model_id': model_id, 'evaluation_start_time': evaluation_start_time} prediction_df = query.get_model_prediction(query_arg, engine)

jesteria · 2017-12-13T17:54:33Z

webapp/query.py

+    df = pd.read_sql(
+        query,
+        con=engine)
+    print(df)


Stray print?

terminated!

jesteria · 2017-12-13T18:00:32Z

webapp/query.py

+    FROM results.models
+    WHERE model_id={model_id};
+    """.format(
+        model_id=model_id)


Perhaps we're not too worried about SQL injection attacks here, but I'd suggest passing expression parameters to the database driver, nonetheless. At the least it might make this a little cleaner.

return pd.read_sql("""\ SELECT model_parameters FROM results.models WHERE model_id=%s """, engine, params=(model_id,), )

[docs]

Yeah for model_id and as_of_date we should pass it to database driver ( I don't know why I miss this one and I'll fix it), but for column_name or schema_name it won't work and have to just interpolate.

…tabase engine

houndci-bot · 2017-12-14T01:03:01Z

webapp/query.py

    query = """
    SELECT feature_list FROM results.model_groups WHERE model_group_id={}
    """.format(model_group_id)
    df = pd.read_sql(
        query,
-        con=db.engine)
+        con=engine)


undefined name 'engine'

houndci-bot · 2017-12-14T01:03:01Z

webapp/query.py

    )
    output = df_importance
    return output

-def get_feature_importance(query_arg):
+def get_feature_importance(query_arg, engine):


expected 2 blank lines, found 1

houndci-bot · 2017-12-14T01:03:01Z

webapp/query.py

@@ -1,12 +1,11 @@
 import pandas as pd
-from webapp import db
+from webapp import app


'webapp.app' imported but unused

houndci-bot · 2017-12-14T01:03:01Z

webapp/__init__.py

@@ -1,12 +1,26 @@
-from flask import Flask
+from flask import Flask, session
+from flask.ext.session import Session, SqlAlchemySessionInterface


'flask.ext.session.SqlAlchemySessionInterface' imported but unused

houndci-bot · 2017-12-14T01:03:01Z

webapp/__init__.py

@@ -1,12 +1,26 @@
-from flask import Flask
+from flask import Flask, session


'flask.session' imported but unused

codecov-io · 2017-12-14T14:51:38Z

Codecov Report

Merging #100 into master will decrease coverage by 0.59%.
The diff coverage is 85.24%.

@@            Coverage Diff            @@
##           master     #100     +/-   ##
=========================================
- Coverage   71.77%   71.17%   -0.6%     
=========================================
  Files           4        4             
  Lines         418      444     +26     
=========================================
+ Hits          300      316     +16     
- Misses        118      128     +10

Impacted Files	Coverage Δ
webapp/__init__.py	`100% <100%> (ø)`	⬆️
webapp/controller.py	`64.38% <81.08%> (-0.22%)`	⬇️
webapp/query.py	`84.37% <87.5%> (-3%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1921de...a83226c. Read the comment docs.

jesteria

🆒 – just a couple comments

jesteria · 2017-12-19T20:57:37Z

webapp/__init__.py

 app.secret_key = "tyra american top models"
+app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['cmpd']['url']


Did you intend to keep this setting? It looks like you've just switched defaulting to SFPD with CMPD.

Oh and in case this setting is required, that is that Flask requires a default database connection, that's OK, of course; though, I might suggest we make it something that doesn't actually resolve, to avoid unintentionally defaulting.

Moreover, we might want a database user/schema for Tyra, e.g. for session data, which this could (at some point) point to.

jesteria · 2017-12-19T20:58:38Z

webapp/__init__.py

 app.secret_key = "tyra american top models"
+app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['cmpd']['url']
+
+app.config['SQLALCHEMY_BINDS'] = {key: db_dict[key]['url'] for key in db_dict.keys()}


Same thing, but if you like 😸

{key: value['url'] for (key, value) in db_dict.items()}

good point.

jesteria · 2017-12-19T21:06:17Z

webapp/__init__.py

 db = SQLAlchemy(app)
+
+app.config['SESSION_TYPE'] = 'filesystem'


Where do we currently deploy Tyra? Just want to check that it has a filesystem appropriate for writing session data.

it's on an ec2 machine.

jesteria · 2017-12-19T21:08:03Z

webapp/controller.py

@@ -11,8 +11,6 @@
 import os
 from config import db_dict

-# Default DB
-app.config['DB_NAME'] = "cmpd"


Do we need to set/get a default database in the session?

nope. removed!

jesteria · 2017-12-19T21:09:51Z

webapp/query.py

+    # try:
+    #     engine = db.get_engine(app, app.config['DB_NAME'])
+    # except:
+    #     engine = db.engine


stale code?

thcrock · 2018-01-03T15:33:58Z

tests/test_evaluations_individual_importance.py

@@ -21,6 +21,8 @@

 def test_individual_importances():
    with rig_test_client(data) as test_app:
+        with test_app.session_transaction() as session:


do we just want to add an optional engine to rig_test_client and have it handle this?

jesteria · 2018-01-03T16:33:15Z

tests/test_evaluations_feature_distribution.py

        assert expected == response_data

 def test_get_feature_dist_train():
    with rig_test_client(data) as test_app:
+        with test_app.session_transaction() as session:
+            session['engine'] = 'cmpd'


what exactly is happening in these cases where you only set the session engine in the context-managed block? usually a context manager is doing something in __enter__ and something else in __exit__. does this affect the remainder of the test? is it different from:

session = test_app.session_transaction() session['engine'] = 'cmpd'

It will give me this error if I don't use the context-managed block with session.
TypeError: '_GeneratorContextManager' object does not support item assignment

That makes sense 😄 I would've guessed that it had an interface that wasn't based around the context manager, but I'm not sure it matters, anyway.

Ah, in fact, I would guess that it's writing the session to the backend upon __exit__ 😉

jesteria

Looks good to me. Just a couple comments, hopefully helpful.

jesteria · 2018-01-29T00:28:42Z

tests/test_evaluations_feature_distribution.py

        assert expected == response_data

 def test_get_feature_dist_train():
    with rig_test_client(data) as test_app:
+        with test_app.session_transaction() as session:
+            session['engine'] = 'cmpd'


Ah, in fact, I would guess that it's writing the session to the backend upon __exit__ 😉

jesteria · 2018-01-29T00:30:38Z

webapp/__init__.py

 app.secret_key = "tyra american top models"
+app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['cmpd']['url']


jesteria · 2018-01-29T00:32:53Z

webapp/query.py

+    # try:
+    #     engine = db.get_engine(app, app.config['DB_NAME'])
+    # except:
+    #     engine = db.engine


jesteria · 2018-01-29T00:36:30Z

webapp/controller.py

+    try:
+        return jsonify(results=output)
+    except Exception as e:
+        return jsonify({"sorry": "Sorry, no results! Please try again."}), 500


Is it possible for jsonify to fail here? For that matter, is there no middleware or other layer which catches Python exceptions and returns response 500, (such as in flask or gunicorn)? It seems as though there's always a chance of an unanticipated, and therefore uncaught, exception being raised; but, here, we're not catching a particular case and returning a useful message, so much as (re?)-implementing a default exception handler.

tweddielin added 3 commits November 9, 2017 18:16

db connection issue

0f7abad

fix db connection issue

f79615c

remove comments

f359785

houndci-bot reviewed Dec 13, 2017

View reviewed changes

jesteria reviewed Dec 13, 2017

View reviewed changes

tweddielin added 4 commits December 13, 2017 18:59

pass the database engine to the query method instead of looking up da…

d86b319

…tabase engine

using simple session

7a5b5de

add flask-session to requirement

e9ad115

make tests compatible with session

a792aa2

houndci-bot reviewed Dec 14, 2017

View reviewed changes

tweddielin added 7 commits December 13, 2017 19:04

fix violations hound ci found

5aa3632

fix test_db_select

6e4acb0

handle testing

bbd3261

db test

659be1e

handle test

570e06c

change global variable on travis

217d355

replace test with cmpd

2e5e382

pass parameter directly to database driver as much as possible

a83226c

tweddielin assigned thcrock Dec 14, 2017

jesteria reviewed Dec 19, 2017

View reviewed changes

thcrock reviewed Jan 3, 2018

View reviewed changes

jesteria reviewed Jan 3, 2018

View reviewed changes

jesteria reviewed Jan 29, 2018

View reviewed changes

		@@ -1,12 +1,26 @@
		from flask import Flask
		from flask import Flask, session

		app.secret_key = "tyra american top models"
		app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['cmpd']['url']

		db = SQLAlchemy(app)

		app.config['SESSION_TYPE'] = 'filesystem'

Db connection issue #100

Are you sure you want to change the base?

Db connection issue #100

Conversation

tweddielin commented Dec 13, 2017

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

jesteria 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

tweddielin Dec 14, 2017 • edited Loading

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

tweddielin Dec 14, 2017 • edited Loading

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

codecov-io commented Dec 14, 2017 • edited Loading

Codecov Report

jesteria 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

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

jesteria 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

tweddielin Dec 14, 2017 •

edited

Loading

tweddielin Dec 14, 2017 •

edited

Loading

codecov-io commented Dec 14, 2017 •

edited

Loading