Closed stavros11 closed 1 month ago
Attention: Patch coverage is 0%
with 160 lines
in your changes missing coverage. Please review.
Project coverage is 50.55%. Comparing base (
6087ba8
) to head (4d00aeb
). Report is 20 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Following https://github.com/qiboteam/qibolab_platforms_qrc/pull/188, this should now be ready. I also confirmed that the qw11q that is controlled with the old OPX+ cluster still works: http://login.qrccluster.com:9000/jnGzYDzWTuyMrezsF-khoA==/
In general, it is a bit larger change than what I expected at first, but still sufficiently localized. We should be able to merge it soon.
No rush in terms of merging. It is also larger and required more effort than what I also expected. But this is mainly because it ended up not only for adding OPX1000 support, but also supporting its new features and doing workarounds around various bugs with (I believe) the qm-qua pre-release (the TODO list in the first comment).
Two other things to note: I have changed a few type annotations from str
to Literal[...]
. Of course is not strictly required and does not affect execution, but I saw you using it in a few other places and I believe it is good to have here as well. It also contributes to the diff.
But a more important point to mention is about how OPX1000 controllers are exposed in terms of interface. This is more clearly visible in the platform, for example: https://github.com/qiboteam/qibolab_platforms_qrc/blob/7e2b2fb62300c9ba6e94f98c342597294cd4edab/qw5q_platinum/platform.py#L54 It was a bit hard for me to decide between:
DcChannel(device="con1/4", path="1")
and
DcChannel(device="con1", path="4/1")
The second is probably a bit better on the user end, since path
is a more accurate description, however I finally opted for the first, because it was easier to handle in the driver. For OPX1000, each controller (here con1
) has some front end modules (FEMs) and each FEM has ports, while for OPX+ we only have controllers and ports. Therefore, in terms of code, the analogue of an OPX+ controller (conX
) is an OPX1000 FEM (conX/Y
), which is why I decided to label it as device
in the channels.
The second is probably a bit better on the user end, since
path
is a more accurate description, however I finally opted for the first, because it was easier to handle in the driver. For OPX1000, each controller (herecon1
) has some front end modules (FEMs) and each FEM has ports, while for OPX+ we only have controllers and ports. Therefore, in terms of code, the analogue of an OPX+ controller (conX
) is an OPX1000 FEM (conX/Y
), which is why I decided to label it asdevice
in the channels.
Yes, I agree with your analysis and decision.
Essentially, it's about deciding whether a FEM is a device or a device's component. And that's ambiguous.
It may be a good time to ask ourselves what is actually defining a device, and why each port is not a device on its own. I guess the main motivation is that we're managing one connection per device, or that devices are somehow parametrized. But I'm not sure myself, so we should try to agree on some definition.
Most likely, there was no problem until now, just because we were dealing with simpler devices (which at most had ports, or not even those).
@alecandido is there something wrong with pre-commit? It didn't complain to me locally. Other than that, if you are okay with the last changes we can probably merge. I did a quick test and it still works on hardware.
@alecandido is there something wrong with pre-commit? It didn't complain to me locally. Other than that, if you are okay with the last changes we can probably merge. I did a quick test and it still works on hardware.
Yes, it's broken because of a recent update (v3 -> v4) that propagated to the CI, but not all our hooks are supporting the update (it makes sense).
However, there is only one hook failing, that is already patched in main
. I'm just waiting for them to release
https://github.com/PyCQA/docformatter/issues/293
such that the bot will trigger to usual upgrade PRs...
For the time being, please ignore it.
Ports #1045 to qibolab 0.2.
TODO:
triggered
andalways_on
)