qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
43 stars 14 forks source link

Remove platform start and stop #739

Closed stavros11 closed 8 months ago

stavros11 commented 8 months ago

Fixes #673. In the end I kept platform.connect() and platform.disconnect() and the same for instruments. I also kept instrument.setup because it is used for loading the settings from the runcard (in serialize.py), however I removed platform.setup() as the loading of settings happens in the platform creation.

Since we are breaking the interface anyway, I also removed some set_*/get_* methods from the platform as they are not used much in qibocal anymore (with a very small exception) and the SPI driver as we have not used it for very long time and the driver may be outdated anyway (if we need it we can go back to the latest tag).

Some small updates in qibocal and qibolab_platforms_qrc are needed after merging this PR!

Checklist:

PiergiorgioButtarini commented 8 months ago

Actually, are you sure do we need to retain setup()? To the best of my knowledge:

  • only the LO are using it
  • most of the others are calling it empty in the docstring (Qblox, Zurich & RFSoC)
  • some are even calling it deprecated (QM & RFSoC)

Actually also the Qblox modules are using the setup() in order to cache the values loaded from the runcard.

stavros11 commented 8 months ago
  • some are even calling it deprecated (QM & RFSoC)

Actually also the Qblox modules are using the setup() in order to cache the values loaded from the runcard.

And also in QM it is used as of #733, not in the Controller but in the individual devices (OPX/Octave), which is essentially similar structure with the Qblox modules.

The thing is that we need a way to deserialize the instruments section of the runcard YAML and because the options there are instrument dependent most likely this needs to be an instrument method. Actually, we will also probably need the inverse of this (instrument.dump()) which will serialize the settings, but I left its implementation for later.

It does not appear in Zurich because it is still following the old approach of hard-coding instrument settings in the create method: https://github.com/qiboteam/qibolab/blob/cc8a1b46754f91d3fc80cef00f941575f1f0f223/tests/dummy_qrc/zurich.py#L131 and maybe RFSoC does not need any instrument settings (that could be calibrated).

codecov[bot] commented 8 months ago

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (9745314) 62.00% compared to head (fc55f79) 62.68%.

Files Patch % Lines
src/qibolab/instruments/qblox/controller.py 12.50% 7 Missing :warning:
src/qibolab/couplers.py 83.33% 2 Missing :warning:
src/qibolab/instruments/oscillator.py 77.77% 2 Missing :warning:
src/qibolab/instruments/abstract.py 83.33% 1 Missing :warning:
src/qibolab/instruments/dummy.py 50.00% 1 Missing :warning:
src/qibolab/instruments/qblox/cluster_qcm_bb.py 0.00% 1 Missing :warning:
src/qibolab/instruments/qm/driver.py 0.00% 1 Missing :warning:
src/qibolab/qubits.py 92.30% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #739 +/- ## ========================================== + Coverage 62.00% 62.68% +0.67% ========================================== Files 48 47 -1 Lines 6095 5930 -165 ========================================== - Hits 3779 3717 -62 + Misses 2316 2213 -103 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/739/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/739/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `62.68% <69.81%> (+0.67%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 8 months ago

Actually, we will also probably need the inverse of this (instrument.dump()) which will serialize the settings, but I left its implementation for later.

These could be .load() and .dump() (like in json stdlib module, and also PyYAML and many others), but I thought that most of it was handled by the ...Settings classes, and thus being done during instantiation of the instruments (i.e. during __init__, and uplaoded during connect, rather than in a separate stage).

stavros11 commented 8 months ago

These could be .load() and .dump() (like in json stdlib module, and also PyYAML and many others), but I thought that most of it was handled by the ...Settings classes, and thus being done during instantiation of the instruments (i.e. during __init__, and uplaoded during connect, rather than in a separate stage).

Then I will rename setup() to load() to avoid confusion with the old setup().

Settings are indeed uploaded during connect but loaded (from the runcard dict to the instrument object) using serialize.load_instrument_settings which uses instrument.setup. Handling in the Settings classes is not possible, because for example for Qblox, settings look like:

module_name:
    port_ name:
        setting_name: setting_value

so Settings are in the inner layer and do not have information about the ports. So it is probably more convenient to load at the module/instrument level (@PiergiorgioButtarini let us know if you have any input).

Loading in __init__ is also possible, but I am afraid that then initialization will become more complex (we are already passing addresses and names) so it may be easier to have a separate load/dump mechanism.

alecandido commented 8 months ago

Settings are indeed uploaded during connect but loaded (from the runcard dict to the instrument object) using serialize.load_instrument_settings which uses instrument.setup. Handling in the Settings classes is not possible, because for example for Qblox, settings look like:

I was imagining (and them proposing) __init__, not anything else (so ...Settings should have been used in there).

Loading in __init__ is also possible, but I am afraid that then initialization will become more complex (we are already passing addresses and names) so it may be easier to have a separate load/dump mechanism.

The idea was just to pass the corresponding runcard section, and deserializing the Settings inside, rather than outside. But I'm open for discussion about this, I'm not so strongly opinionated.

stavros11 commented 8 months ago

Qblox and Zurich qpu tests are now passing on this branch. As soon as the CI passes, I will merge this.