-
Notifications
You must be signed in to change notification settings - Fork 380
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
Added the test for the import error. #547
Added the test for the import error. #547
Conversation
sacred/observers/sql.py
Outdated
@@ -20,6 +16,8 @@ | |||
class SqlObserver(RunObserver): | |||
@classmethod | |||
def create(cls, url, echo=False, priority=DEFAULT_SQL_PRIORITY): | |||
from sqlalchemy.orm import sessionmaker, scoped_session | |||
from .sql_bases import sa |
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.
Is there any reason you are import sqlalchemy
here from sql_bases
instead of directly?
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.
Yes, there is a very good reason. It's because I wasn't attentive, haha.
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.
Haha ok. PR looks good, I will merge once the tests pass after the change.
We have an issue with error messages for optional dependencies. For exemple, trying to use the sql observer without sql installed throws the following error:
Not very helpful. See here the travis log for the test I added.
This PR makes sure that we have a proper error message.
I had to enable the test for the observers in the "setup" build. Otherwise the added test will never run.
With the new setup, the error message is:
The majority of the change is just a copy past between sql.py and sql_bases.py.