Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
ymampaey please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Logger identity collisions and mutable scope resolution can break the promised isolation and hierarchy.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (5)
Use module-qualified identities for per-driver logger isolation · New Store scope at root initialization for all descendants · New Avoid ambient logging levels in class logger test · New Clarify propagate=False and separate VISA logger behavior · New Document permanent logging-registry growth caveat · New
What changed in this PR
Adds opt-in per-instrument logger hierarchies while preserving shared logging by default.
Changes:
- Adds configurable logger scope and hierarchical names.
- Applies scoped naming to VISA loggers.
- Adds tests, documentation, and a newsfragment.
| File | Description |
|---|---|
instrument_base.py |
Defines logger scopes and scoped-name generation. |
visa.py |
Applies scoped VISA logging. |
ip_to_visa.py |
Applies scoped logging to simulated VISA instruments. |
test_logger.py |
Tests scope, inheritance, filtering, and VISA behavior. |
logging_example.ipynb |
Documents scoped logger usage. |
8523.new |
Announces the feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """ | ||
| root = self.root_instrument | ||
| if root.default_logger_scope == "instrument": | ||
| return ".".join((base, type(root).__name__, *self.name_parts)) |
There was a problem hiding this comment.
This would basically be equivalent to replacing base which is currently the name of this module qcodes.instrument.instrument_base with the name of the module that the instrument is defined in. qcodes.insrument.instrument_vendor.instrument_filename. I think I agreee that I would prefer this
| root = self.root_instrument | ||
| if root.default_logger_scope == "instrument": | ||
| return ".".join((base, type(root).__name__, *self.name_parts)) |
There was a problem hiding this comment.
I don't think this is a big concern but if we change the type to default_logger_scope: ClassVar[LoggerScope] we make it clear that this should not be modified on a per instance level
| class_logger = logging.getLogger( | ||
| f"{SHARED_INSTRUMENT_LOGGER_NAME}.ScopedDummyInstrument" | ||
| ) | ||
| class_logger.setLevel(logging.DEBUG) | ||
|
|
||
| # created only after the level was configured | ||
| inst_a = ScopedDummyInstrument("class_level_a") | ||
| inst_b = ScopedDummyInstrument("class_level_b") | ||
| other = ScopedDummyChannelInstrument("class_level_other") | ||
|
|
||
| assert inst_a.log.logger.getEffectiveLevel() == logging.DEBUG | ||
| assert inst_b.log.logger.getEffectiveLevel() == logging.DEBUG | ||
| assert other.log.logger.getEffectiveLevel() != logging.DEBUG |
| "Two things are worth keeping in mind:\n", | ||
| "\n", | ||
| "* QCoDeS attaches its console and file handlers to the **root** logger and those handlers have their own levels. Lowering the level of an instrument logger to `DEBUG` only becomes visible if the handler level lets the record through, so combine it with `console_level` or `handler_level` as shown above.\n", | ||
| "* Setting `instrument.log.logger.propagate = False` silences that instrument everywhere, including in the QCoDeS log file.\n", |
There was a problem hiding this comment.
I think this is probably worth doing
| "* QCoDeS attaches its console and file handlers to the **root** logger and those handlers have their own levels. Lowering the level of an instrument logger to `DEBUG` only becomes visible if the handler level lets the record through, so combine it with `console_level` or `handler_level` as shown above.\n", | ||
| "* Setting `instrument.log.logger.propagate = False` silences that instrument everywhere, including in the QCoDeS log file.\n", | ||
| "\n", | ||
| "Note also that parameters log through the logger of their *root* instrument, so messages from a channel parameter appear on the root instrument's logger rather than on the channel's own logger." |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8523 +/- ##
==========================================
+ Coverage 72.00% 72.02% +0.01%
==========================================
Files 305 305
Lines 32019 32028 +9
==========================================
+ Hits 23055 23067 +12
+ Misses 8964 8961 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Closes #8522
Summary
Every instrument currently shares one
logging.Loggerobject, becauseInstrumentBase.__init__names it with a module constant:Since
logging.getLogger()caches by name,setLevel,addHandlerandpropagateset on one instrument apply to all of them.VisaInstrument.visa_loghas the same problem via the constantVISA_LOGGER.This PR lets a driver opt in to its own logger by setting one class attribute. QCoDeS only chooses the logger name — it still never sets levels, adds handlers or touches
propagate; that stays driver/application policy.The default is unchanged. Without opting in, logger names, records, formatting and filtering are byte-for-byte identical to today.
What the scoped names look like
The name is built from the driver class of the
root_instrumentfollowed by the instrument'sname_parts, and stays a descendant of the existing shared logger:Each node is a real ancestor of the next, so a level set at any level is inherited by everything below it, and a level configured on
qcodes.instrument.instrument_base(e.g. vialogger_levelsinqcodesrc.json) is still inherited by everything.Examples
Per instrument
Per driver class
This is the common driver-development case: a station often holds several instruments of the same driver (QCoDeS' own
AMI430_3Dtest fixture buildsmag_x,mag_yandmag_z, allAMIModel430) and you want DEBUG for all of them without listing their names.Because a level can be set on a logger before it exists, this also covers instruments created later and can be written declaratively in
qcodesrc.json:Submodules follow their instrument
Design notes
Why
name_partsrather thanfull_name.name_partsis QCoDeS' canonical identity for an instrument. Joining the parts with.rather than using the_-joinedfull_nameis what makes a channel logger a genuine child of its instrument's logger.Why the class comes from
root_instrument, nottype(self). Usingtype(self)would put an instrument and its own channel in different subtrees (....MyDriver.myinstnext to....DummyChannel.myinst_ChanA), soinst.log.logger.setLevel(DEBUG)would silently not reach the instrument's channels. Taking the class from the root instrument keeps the whole tree under one class node.Why the name extends the existing base instead of replacing it. A name derived from
type(self).__module__would put third-party drivers — and drivers defined in a notebook (__main__) — outside theqcodes.*tree, wherelogger_levelsanddictConfigrules scoped toqcodesno longer apply. Keepingbasealso preserves the separation betweenlogandvisa_log, which are created from different base names.default_logger_scopeis a plain class attribute, not aClassVar, matching the existingVisaInstrument.default_terminator/default_timeoutprecedent.Known caveat (documented)
Python's
loggingregistry keeps a strong reference to every logger and has no removal API, so under"instrument"scope repeatedly creating instruments with distinct names leaks one registry entry per name. Negligible for normal sessions; noted in the docs.Changes
src/qcodes/instrument/instrument_base.pyLoggerScope,default_logger_scope,_logger_name(), scopedself.logsrc/qcodes/instrument/visa.py,ip_to_visa.pyself.visa_logtests/test_logger.pydocs/examples/logging/logging_example.ipynbdocs/changes/newsfragments/Tests
New tests cover: default scope unchanged (regression guard); distinct loggers per instance;
setLevelisolation; inheritance from the shared logger; class-level DEBUG reaching instruments created afterwards and their submodules but not other drivers; per-instrument level overriding the class level; submodule logger being a child of its instrument;filter_instrumentstill working under scoping; and thevisa_logequivalents.Full suite passes (3249 passed, 270 skipped; the one failure in
test_installation_infois a pre-existing local environment issue unrelated to this change and reproduces on unmodifiedmain).pyrightreports 0 errors andpre-commit run --allpasses. The updated notebook was executed end to end.