-
Notifications
You must be signed in to change notification settings - Fork 309
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
Call start_all_logging
on qcodes import
#1850
Call start_all_logging
on qcodes import
#1850
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1850 +/- ##
==========================================
- Coverage 70.38% 70.37% -0.01%
==========================================
Files 152 152
Lines 18859 18875 +16
==========================================
+ Hits 13273 13283 +10
- Misses 5586 5592 +6 |
This PR is ready for review. I have, however, not figured out how the cyclic import works: |
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.
@Dominik-Vogel I approve the code, but - could you please add relevant notes to the 15minutesOfQcodes notebook? and, perhaps, to the logging notebook(s)?
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 update to docs, thanks!
@Dominik-Vogel There are some (unintentional i guess) changes to to the docs templates in this pr |
Thank you for spotting this. I'll fix it. |
41fa04a
to
9668acc
Compare
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 log the outcome of conditionally_start_all_logging and why that decision was made?
Adding the call to
start_all_logging
to theqcodes.__init__
file.This PR provide a new option in
qcodesrc.json
to enable this feature (defaults to false). It will also automatically enabled if a workstation id and location id has been defined.Furthermore, it is tested if we are in a pytest session. It is bad practice to change anything dependent on whether it is a test situation (this lesson we learned from an anonymous automobile company). Here one could argue that we have not been testing with logging enabled in any case, so that the effective behavior is not altered.
I checked if the logging was triggered by pylint and mypy. Neither do trigger logging.
I tried to incorporate Jens suggestion about marking the test environment in conftest.py, but this failed because qcodes gets imported before entering conftest.
I added a condition for the instrumentation key and refactored the pr.