-
Notifications
You must be signed in to change notification settings - Fork 6
New Session fixing attempt #85
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| from sqlalchemy import event | ||
|
|
||
| from battlebots.database import session | ||
| from battlebots.database import Session | ||
| from battlebots.database.models import MatchParticipation | ||
| from battlebots.ranker.elo import elo_diff | ||
|
|
||
|
|
||
| @event.listens_for(session, 'before_commit') | ||
| @event.listens_for(Session, 'before_commit') | ||
|
Member
Author
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. Untested, no idea how this works.
Member
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. Will error iirc, I have tried before.
Member
Author
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. I'll try to test this locally this evening |
||
| def update_bot_ratings(session): | ||
| # If anyone finds a better way to update the ratings, please do | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| from sqlalchemy.ext.associationproxy import association_proxy | ||
|
|
||
| from battlebots import backports, config, sandbox | ||
| from battlebots.database import session | ||
| from battlebots.database import Session | ||
| from battlebots.ranker.elo import DEFAULT_SCORE | ||
|
|
||
| Base = declarative_base() | ||
|
|
@@ -152,7 +152,7 @@ def loss_percentage(self): | |
|
|
||
| @property | ||
| def rank(self): | ||
| bots = enumerate(session.query(Bot).order_by(desc(Bot.score)).all()) | ||
| bots = enumerate(Session.query(Bot).order_by(desc(Bot.score)).all()) | ||
|
Member
Author
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 here, scoped_session? If not, why?
Member
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 will work I think, as this example is somewhere to be found in the docs.
Member
Author
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 as #85 (comment), but this is a hard one as this is called from the ranker as well as from the webapp. |
||
| return next(dropwhile(lambda bot: bot[1] != self, bots))[0] | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import datetime as d | ||
|
|
||
| from battlebots.database import session | ||
| from battlebots.database import scoped_session | ||
| from battlebots.database.models import User, Bot, Match, MatchParticipation | ||
|
|
||
| user1 = User('tester1', 'test1@mail.com', 'test_pw_1') | ||
|
|
@@ -17,7 +17,6 @@ | |
|
|
||
| l = [user1, user2, bot1, bot1, match1] | ||
|
|
||
| for x in l: | ||
| session.add(x) | ||
|
|
||
| session.commit() | ||
| with scoped_session() as db: | ||
|
Member
Author
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. Commit doesn't seem necesarry here as the scoped_session already commits
Member
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. correct
Member
Author
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. :pompom: |
||
| for x in l: | ||
| db.add(x) | ||
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.
Should we use a
with scoped_session() as db:here? If not, why?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.
No, we shouldn't need them on queries since those should not cause errors that require a session-rollback.
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's correct, but at that point, there isn't an open session as Session() is only called in 2 places: before every Flask request handling and at the start of a scoped transaction. So wrapping that bit in a scoped_session block would open a session, and rollback if the query errors (which the ORM should ignore as there are no open transacation).
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.
The commit/rollback part would indeed be unnecessary, so maybe we can make two kinds of context managers? Or maybe change
Session.query(...)toSession().query(...)?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 last wouldnt close that session connection then?
On 8 Apr 2016 11:00, "Stijn Seghers" notifications@github.com wrote:
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.
Opening a session for each request certainly seems the way to go, as people keep recommending it. It's definitely the most scalable and least bugprone way to do it. We should attempt this and not call it quits before we manage to do it.
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.
@silox: I just reread your first comments. Why wouldn't there be an open session? If we chose (2), than either the ranker or the web server should have opened one. Otherwise this code shouldn't be called in the first place. Right?
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.
@Procrat The ranker and webserver don't share their sessions, as they both use different enginges. But you are right that there should a session should be available because the Session object is connected to a session-factory.
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.
Exactly.
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.
@Procrat Yes! Good thinking, that should indeed be the case and fix this
problem
On 8 Apr 2016 11:26, "Stijn Seghers" notifications@github.com wrote: