-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fix db engine check in Experiment [Resolves #538] #539
Conversation
The experiment checks whether or not the passed-in engine is a base sqlalchemy engine, and converts it into the serializable triage engine if so. However, this check was broken. This PR fixes it to actually duck-type, as well as moves the check to the MultiCore so we can actually error out if the wrong engine is passed.
8244f94
to
1445686
Compare
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 82.57% 82.59% +0.02%
==========================================
Files 81 81
Lines 4625 4625
==========================================
+ Hits 3819 3820 +1
+ Misses 806 805 -1
Continue to review full report at Codecov.
|
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.
nice
Made the changes from jesse's review. Since the assignee isn't here until the middle of next week, and I think I've addressed all the concerns that they brought up, and the bug this fixes is a rather onerous one for current users of Triage, I'm opting to just merge this. |
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.
great!
The experiment checks whether or not the passed-in engine is a base sqlalchemy engine, and converts it into the serializable triage engine if so.
However, this check was broken. This PR fixes it to actually duck-type,
to see if the engine can be pickled to a memory file