Change return type of root_instrument to Instrument by jenshnielsen · Pull Request #7924 · microsoft/Qcodes

jenshnielsen · 2026-03-12T15:31:08Z

The point of root_instrument is to get to the instrument.

Breaking change notice

src/qcodes/instrument/delegate/delegate_instrument.py

codecov · 2026-03-13T09:27:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.26%. Comparing base (7428872) to head (9fa7995).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7924   +/-   ##
=======================================
  Coverage   70.26%   70.26%           
=======================================
  Files         333      333           
  Lines       32169    32169           
=======================================
  Hits        22604    22604           
  Misses       9565     9565

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates root_instrument typing/usage to better represent the “top-level instrument” concept and adjusts tests accordingly.

Changes:

Changes ParameterBase.root_instrument return type to Instrument | None (with casting) and adjusts monitor typing usage.
Updates InstrumentBase.root_instrument typing and adds/updates overrides in several drivers/channels.
Strengthens driver tests by asserting mocked root_instrument behavior/types.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/drivers/keysight_b1500/b1500_driver_tests/test_sampling_measurement.py	Adds assertion that `root_instrument.ask` is mocked in tests.
tests/drivers/keysight_b1500/b1500_driver_tests/test_b1520a_cmu.py	Adds assertions that `root_instrument` is a `MagicMock` before using `.ask`.
tests/drivers/keysight_b1500/b1500_driver_tests/test_b1517a_smu.py	Adds assertion that `root_instrument` is a `MagicMock` before verifying `.write` calls.
src/qcodes/parameters/parameter_base.py	Changes `root_instrument` type to `Instrument
src/qcodes/monitor/monitor.py	Updates local typing for `baseinst` when using `parameter.root_instrument`.
src/qcodes/instrument_drivers/tektronix/DPO7200xx.py	Overrides `root_instrument` return type with `type: ignore[override]` and runtime assert.
src/qcodes/instrument_drivers/Keysight/N52xx.py	Same as above for Keysight PNA driver.
src/qcodes/instrument_drivers/Keysight/KtMAwg.py	Same as above for Keysight AWG driver.
src/qcodes/instrument_drivers/Keysight/Infiniium.py	Same as above for Keysight Infiniium driver.
src/qcodes/instrument/instrument_base.py	Changes `root_instrument` return type annotation to `Self`.
src/qcodes/instrument/channel.py	Overrides `root_instrument` return type to `Instrument` (via `cast`) with expanded comment.

Copilot · 2026-04-14T09:00:33Z

src/qcodes/instrument/instrument_base.py


    @property
-    def root_instrument(self) -> InstrumentBase:
+    def root_instrument(self) -> Self:


The PR title/description says the return type of root_instrument is being changed to Instrument, but InstrumentBase.root_instrument is now annotated as returning Self. Self implies the return value is the same concrete type as self, which is not generally true for modules/channels where root_instrument typically returns the top-level Instrument (a different class than the module/channel). Consider adjusting the base annotation to a type that matches the actual contract (e.g., InstrumentBase or InstrumentBase + a narrower override on Instrument/channels), or update the PR description/title to reflect that this is a Self-typing change rather than an Instrument return-type change.

Copilot · 2026-04-14T09:00:34Z

src/qcodes/instrument/channel.py

+        # the root instrument is the top level parent of this module, we need to
+        # go up the parent hierarchy until we find an object that returns itself as the parent, this should be the root instrument. We also
+        # this is required to be an Instrument.
+        # Once 3.13 is the minimum supported version
+        # consider replacing with a generic parameter with a default
+        # value of Instrument.
+        return cast("Instrument", self._parent.root_instrument)



The comment describes walking the parent hierarchy to locate the root, but the implementation simply returns self._parent.root_instrument with a cast. This makes the comment misleading and also relies on cast (type-only) rather than a runtime check. Consider either (a) simplifying/updating the comment to match the implementation and invariants, and/or (b) using a runtime isinstance(..., Instrument) assertion (similar to the driver overrides in this PR) to enforce the contract that the channel’s root is an Instrument.

Suggested change

# the root instrument is the top level parent of this module, we need to

# go up the parent hierarchy until we find an object that returns itself as the parent, this should be the root instrument. We also

# this is required to be an Instrument.

# Once 3.13 is the minimum supported version

# consider replacing with a generic parameter with a default

# value of Instrument.

return cast("Instrument", self._parent.root_instrument)

# Delegate root resolution to the parent and enforce that the

# resolved root instrument is an Instrument at runtime.

# Once 3.13 is the minimum supported version

# consider replacing with a generic parameter with a default

# value of Instrument.

from .instrument import Instrument

root_instrument = self._parent.root_instrument

assert isinstance(root_instrument, Instrument)

return root_instrument

Copilot · 2026-04-14T09:00:34Z

src/qcodes/instrument_drivers/tektronix/DPO7200xx.py

-    def root_instrument(self) -> "TektronixDPO7000xx":
+    def root_instrument(self) -> "TektronixDPO7000xx":  # type: ignore[override]
+        # ideally this should be a generic type parameter but
+        # for now we override it here. This requies a mypy ignore


Corrected spelling of 'requies' to 'requires'.

Suggested change

# for now we override it here. This requies a mypy ignore

# for now we override it here. This requires a mypy ignore

Copilot · 2026-04-14T09:00:34Z

src/qcodes/instrument_drivers/Keysight/N52xx.py

-    def root_instrument(self) -> "KeysightPNABase":
+    def root_instrument(self) -> "KeysightPNABase":  # type: ignore[override]
+        # ideally this should be a generic type parameter but
+        # for now we override it here. This requies a mypy ignore


Corrected spelling of 'requies' to 'requires'.

Suggested change

# for now we override it here. This requies a mypy ignore

# for now we override it here. This requires a mypy ignore

Copilot · 2026-04-14T09:00:35Z

src/qcodes/instrument_drivers/Keysight/KtMAwg.py

-    def root_instrument(self) -> "KeysightM9336A":
+    def root_instrument(self) -> "KeysightM9336A":  # type: ignore[override]
+        # ideally this should be a generic type parameter but
+        # for now we override it here. This requies a mypy ignore


Corrected spelling of 'requies' to 'requires'.

Suggested change

# for now we override it here. This requies a mypy ignore

# for now we override it here. This requires a mypy ignore

Copilot · 2026-04-14T09:00:35Z

src/qcodes/instrument_drivers/Keysight/Infiniium.py

-    def root_instrument(self) -> "KeysightInfiniium":
+    def root_instrument(self) -> "KeysightInfiniium":  # type: ignore[override]
+        # ideally this should be a generic type parameter but
+        # for now we override it here. This requies a mypy ignore


Corrected spelling of 'requies' to 'requires'.

Suggested change

# for now we override it here. This requies a mypy ignore

# for now we override it here. This requires a mypy ignore

jenshnielsen commented Mar 13, 2026

View reviewed changes

src/qcodes/instrument/delegate/delegate_instrument.py Show resolved Hide resolved

jenshnielsen force-pushed the jenshnielsen/root_instrument_type branch 2 times, most recently from 269ad6e to 9fa2192 Compare April 7, 2026 19:41

jenshnielsen added 3 commits April 10, 2026 07:26

More specific return type for root_instrument

1e09c49

Parameter.root_instrument is Instrument

7fdecd0

Fix mypy type checking

9fa7995

jenshnielsen force-pushed the jenshnielsen/root_instrument_type branch from 9fa2192 to 9fa7995 Compare April 10, 2026 05:34

jenshnielsen requested a review from Copilot April 14, 2026 08:52

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Copilot started reviewing on behalf of jenshnielsen April 14, 2026 09:12 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change return type of root_instrument to Instrument#7924

Change return type of root_instrument to Instrument#7924
jenshnielsen wants to merge 3 commits intomicrosoft:mainfrom
jenshnielsen:jenshnielsen/root_instrument_type

jenshnielsen commented Mar 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# for now we override it here. This requies a mypy ignore
	# for now we override it here. This requires a mypy ignore

Conversation

jenshnielsen commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jenshnielsen commented Mar 12, 2026 •

edited

Loading

codecov bot commented Mar 13, 2026 •

edited

Loading