qiboteam / qibolab

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

Handle integer pulse duration in QM #1077

Closed stavros11 closed 4 weeks ago

stavros11 commented 1 month ago

@alecandido I believe we had this discussion before releasing 0.2 and we were expecting that int (not float) durations will be captured by pydantic, however this does not seem to be the case. This issue appears in some qibocal routines that allow to change pulse durations (eg. qubit spectroscopy), so I am using

pulse = pulse.model_copy(update={"duration": 1000})

in the routine and there it seems that I am able to pass ints, which then cause the driver to fail. In these cases, the duration is read from the qibocal action runcard, so another fix is to just use floats there (1000.0 instead of 1000). However that's a bit annoying and I wouldn't expect every user to do it, that's why the proposed fix here.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 50.55%. Comparing base (d16f03f) to head (77ede74). Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/qm/controller.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1077 +/- ## ======================================= Coverage 50.55% 50.55% ======================================= Files 63 63 Lines 2886 2886 ======================================= Hits 1459 1459 Misses 1427 1427 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/1077/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/1077/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `50.55% <0.00%> (ø)` | | 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 4 weeks ago

Sorry if it took me some time, I wanted to check what's happening with Pydantic.

Apparently, .model_copy(update=...) is not validating the update, https://github.com/pydantic/pydantic/issues/418, despite Pydantic being also capable of validating the assignments.

That was a bit unexpected...

Of course, we could force a revalidation by doing something like:

pulse = pulse.model_validate(pulse.model_dump())

It seems a huge waste, but it would solve our problem.

Otherwise, we should look for another workaround, to manually validate updates with the generated schema (i.e. accept missing pieces, but validating the present ones), before applying them with .model_copy(update=...). Which is exceedingly annoying... (since I should go back to fiddling with the schema, or some Pydantic's potentially-obscure-feature...).

For the time being, your specific solution is good enough to solve the current issue.

stavros11 commented 4 weeks ago

Indeed, the patch here is only to make things a bit less annoying for QM and it certainly doesn’t solve the problem. I will merge it now, however you may want to open an issue for the rest.

For this particular case, it would even be sufficient to cast the int to float, instead of validating and raising error. It’s just that now none of this happens and we end up with a weird error from the QM driver which is fixed here.