-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Db connection issue #100
base: master
Are you sure you want to change the base?
Db connection issue #100
Changes from all commits
0f7abad
f79615c
f359785
d86b319
7a5b5de
e9ad115
a792aa2
5aa3632
6e4acb0
bbd3261
659be1e
570e06c
217d355
2e5e382
a83226c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,3 +100,6 @@ webapp/static/output.js | |
|
||
# Users info | ||
users.json | ||
|
||
# Flask Session | ||
flask_session/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,5 +8,6 @@ numpy | |
scipy | ||
scikit-learn | ||
flask_sqlalchemy | ||
flask-session | ||
flask-login | ||
gunicorn |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,18 @@ | ||
from tests.utils import rig_test_client | ||
from webapp import app | ||
import json | ||
import flask | ||
|
||
def test_db_list(): | ||
response = app.test_client().get("/db_list") | ||
assert response.status_code == 200 | ||
|
||
|
||
def test_db_choose(): | ||
response = app.test_client().get("/db_choose/cmpd") | ||
assert response.status_code == 200 | ||
response_data = json.loads(response.get_data().decode('utf-8')) | ||
assert response_data['result'] == app.config['DB_NAME'] | ||
|
||
with rig_test_client({}) as test_app: | ||
with test_app.session_transaction() as session: | ||
session['engine'] = 'cmpd' | ||
response = test_app.get("/db_choose/cmpd") | ||
assert response.status_code == 200 | ||
response_data = json.loads(response.get_data().decode('utf-8')) | ||
assert response_data['result'] == session['engine'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ | |
|
||
def test_individual_importances(): | ||
with rig_test_client(data) as test_app: | ||
with test_app.session_transaction() as session: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we just want to add an optional engine to |
||
session['engine'] = 'cmpd' | ||
url = '/evaluations/1/individual_feature_importance/1234/2014-12-01' | ||
response = test_app.get(url) | ||
assert response.status_code == 200 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,24 @@ | ||
from flask import Flask | ||
from flask_session import Session | ||
from webapp.flask_util_js import FlaskUtilJs | ||
from flask_sqlalchemy import SQLAlchemy | ||
import flask_login | ||
import json | ||
from config import db_dict | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
app = Flask(__name__,instance_relative_config=True) | ||
app = Flask(__name__, instance_relative_config=True) | ||
app.secret_key = "tyra american top models" | ||
app.config['SQLALCHEMY_DATABASE_URI'] = db_dict['cmpd']['url'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you intend to keep this setting? It looks like you've just switched defaulting to SFPD with CMPD. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (bump) |
||
|
||
app.config['SQLALCHEMY_BINDS'] = {key: db_dict[key]['url'] for key in db_dict.keys()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing, but if you like 😸
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point. |
||
|
||
|
||
db = SQLAlchemy(app) | ||
|
||
app.config['SESSION_TYPE'] = 'filesystem' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do we currently deploy Tyra? Just want to check that it has a filesystem appropriate for writing session data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's on an ec2 machine. |
||
sess = Session(app) | ||
sess.init_app(app) | ||
|
||
fujs = FlaskUtilJs(app) | ||
login_manager = flask_login.LoginManager() | ||
login_manager.init_app(app) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, in fact, I would guess that it's writing the session to the backend upon
__exit__
😉