Propagate Pydantic serialization

[alecandido]

Use more extensively Pydantic in to (de)serialize the runcard content.

The overall goal is to replace most of serialize.py to use just Pydantic, without manual rules for parsing (i.e. delegate as much as possible to the classes' definition).

I split from #818, because introducing Pydantic in that PR was borderline a mistake: the PR got bloated because of many related substitutions (but it had its purpose, since Pulse.eval() had to be replaced, and the current version is definitely more reliable...).

[alecandido]

@stavros11, e38232c should address your comment, https://github.com/qiboteam/qibolab/pull/818#discussion_r1545813668

[codecov[bot]]

Codecov Report

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

Project coverage is 45.18%. Comparing base (f829e8f) to head (1b80394). Report is 424 commits behind head on 0.2.

Files	Patch %	Lines
...rc/qibolab/instruments/emulator/pulse_simulator.py	50.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## 0.2 #870 +/- ## ========================================== - Coverage 48.75% 45.18% -3.57% ========================================== Files 61 70 +9 Lines 5532 6250 +718 ========================================== + Hits 2697 2824 +127 - Misses 2835 3426 +591 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/870/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/870/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `45.18% <93.33%> (-3.57%)` | :arrow_down: | 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]

My current intention was to start from the Pulses, and propagate Pydantic to the other classes involved in serialize.py, to eventually drop the custom functions, manipulating the various objects.

While inspecting serializing.py, I'm finding a few limitations that makes it impractical to just follow this plan, without taking care at the same time of other aspects involved.

• [ ] _load_pulse is almost fully converted, but for couplers https://github.com/qiboteam/qibolab/blob/b6335dcff00b05e631858dd855bac4215f4f9a4f/src/qibolab/serialize.py#L90-L107
• [ ] _load_single_qubit_natives should not present any further complication (beyond full _load_pulse conversion)
• [ ] _load_two_qubit_natives is also dealing with couplers explicitly, and converting one-element's sequences
• [ ] register_gates is also dealing with couplers explicitly, and with qubit names (it requires json.loads to deserialize integers and strings at the same time), and similar shenanigans are involved for the qubit pair names, and extra handling for symmetric gates
• [ ] _dump_pulse is taking the burden of deleting a bunch of things https://github.com/qiboteam/qibolab/blob/b6335dcff00b05e631858dd855bac4215f4f9a4f/src/qibolab/serialize.py#L185-L191 and it is also explicitly converting the enum variants to the values (that I suspect being wrong, since already done by Pydantic, so most likely it's not properly tested)
• [ ] _dump_single_qubit_natives is deleting the .qubit attribute
• [ ] _dump_two_qubit_natives is dealing with couplers explicitly, and it's also filtering 0-length sequences
• [ ] dump_native_gates is dealing with couplers explicitly, filtering 0-length sequences, and constructing the pair name

Thus, to advance this PR, it would be nice to:

• [ ] improve couplers handling
  • they are essentially everywhere, and you always have to check manually if you want to target a qubit or a coupler, and then do the exact same thing
  • we should treat couplers as regular qubits, or inherit both from the same object
• [ ] standardize qubit names
  • I still would like to restrict to integers, and keep a mapping somewhere else (if in Qibolab as well, fully external)
  • Pydantic could handle the Union for us (but I'd suggest avoiding)
• [ ] same for pairs identifiers
• [ ] symmetric gates should not be duplicated on load, but the lookup process should realize it is symmetric, and check for the sorted one (or check for a random one, and fallback on the other)
• [ ] 0-length sequences should be left where they are (if you don't want to serialize, do not add to the platform at all) and 1-length sequences should be treated as sequences, and not converted to elements
• [ ] pulse attributes should be serialized as they are, without manual deletions
  • if there are fields that should not be there when serializing, maybe they shouldn't be there at all

  • 792 should help with this

[stavros11]

• [ ] improve couplers handling

  • they are essentially everywhere, and you always have to check manually if you want to target a qubit or a coupler, and then do the exact same thing
  • we should treat couplers as regular qubits, or inherit both from the same object

Do you have a proposal for this? I think the main issue was that qubits and couplers may have the same names (especially if you use integers) and there was no easy way to tell if it is a qubit or a coupler during deserialization. This was even propagated to the code, because pulse.qubit is a str/int and not a Qubit/Coupler so the PulseType.COUPLERFLUX was needed to know if we are dealing with couplers.

I agree about the inheritance.

• [ ] standardize qubit names

  • I still would like to restrict to integers, and keep a mapping somewhere else (if in Qibolab as well, fully external)

The main objection for this came from the lab, so a big group of users. Personally, I would also prefer to restrict to a single type and even better the simplest type. Of course, internally we can do whatever we want, however in the current approach users are directly playing with the platform runcards, so I am assuming that they would like to be able to use strings there. Most likely this will affect deserialization (if we decide to provide the feature).

I agree with the rest of the points. Let us know if we should contribute in any of this (most likely from next week). The only thing to mention is that some of these issues may be resolved automatically if we delay a bit. For example, I understand that pulse.qubit will not exist in the final 0.2 iteration, so maybe there is no point in trying to deserialize it or deleting it before serializing - these will be solved just by removing pulse.qubit.

[alecandido]

Do you have a proposal for this? I think the main issue was that qubits and couplers may have the same names (especially if you use integers) and there was no easy way to tell if it is a qubit or a coupler during deserialization. This was even propagated to the code, because pulse.qubit is a str/int and not a Qubit/Coupler so the PulseType.COUPLERFLUX was needed to know if we are dealing with couplers.

Actually, I have an abstract idea, but I should try to implement it to check if and which are the shortcomings.

They could literally be the same object, and in most situations you don't really care whether it is a qubit or a coupler. Either the function knows (since it is only performing an action for a qubit or a coupler, but not both), or it is irrelevant (the action is just the same), or you have to process multiple of them. In my mind:

1. multiple means all and
2. all means that you have access to the platform

If you have access to the platform, we just need to record which are couplers (e.g. with a set of integers). If really needed (to avoid some duplication, not possible in other ways - and I believe it will be mostly possible) this information could be passed down by argument.

I assume 1. because I don't see how you might want to operate just a subset of qubits & couplers (unless you're working with something like a sliced view, but it could be a platform-like object, or you are playing 2-qubits gates, that should not need the information - they should just need suitable channels to play the pulses).

As I said, I should try to implement it to check if I'm still missing something.

[alecandido]

The main objection for this came from the lab, so a big group of users. Personally, I would also prefer to restrict to a single type and even better the simplest type. Of course, internally we can do whatever we want, however in the current approach users are directly playing with the platform runcards, so I am assuming that they would like to be able to use strings there. Most likely this will affect deserialization (if we decide to provide the feature).

I would solve this during platform creation: we will process the names during the runcard load, store the map (for Qibocal) and never use it internally.

This will affect debugging (since you should manually look up the mapping, if ever), but it could be automatically translated in plots and everything else for users.

I agree with the rest of the points. Let us know if we should contribute in any of this (most likely from next week). The only thing to mention is that some of these issues may be resolved automatically if we delay a bit. For example, I understand that pulse.qubit will not exist in the final 0.2 iteration, so maybe there is no point in trying to deserialize it or deleting it before serializing - these will be solved just by removing pulse.qubit.

Sure, we could coordinate. However, I was almost writing to myself: as soon as I will have time I will start doing some of these things. If you want to contribute, feel free to pick up your favorite and start tackling it :)

(btw, removing pulse.qubit is related to the channel refactoring)

[alecandido]

Ok, this PR is essentially a fix for @stavros11 comment, and there is no reason to keep it open forever.

I turned the inspection on automated serialization blockers in a separate issue, #882, and it could be addressed incrementally in the 0.2 finalization phase (as soon as @hay-k will refine enough the channel proposal, I could make a Coupler refactor attempt on top, that is the main blocker).

This PR is tiny and should be simple enough to review. I propose to merge it immediately (after the review, of course).

[alecandido]

@stavros11 @hay-k this is pretty small, it should be quick to review :P

(@stavros11, I know you did it, but I'm still waiting for your reply above)

[stavros11]

(@stavros11, I know you did it, but I'm still waiting for your reply above)

My reply: https://github.com/qiboteam/qibolab/pull/892. If you (and @hay-k) agree, feel free to merge this directly here, as I am not doing anything other than removing FluxPulse and propagating it to the dummy runcards so that tests pass.

[alecandido]

Thanks @stavros11 for the approval. I'll still wait for @hay-k's feedback as well :)

[stavros11]

@alecandido now that 0.1.8 is released, shall we merge this and rebase 0.2 to main? Let me know if you are planning to do it, otherwise I can also give a try.

[alecandido]

@alecandido now that 0.1.8 is released, shall we merge this and rebase 0.2 to main? Let me know if you are planning to do it, otherwise I can also give a try.

@stavros11 ok, I did it, and by now 0.2 is slightly broken, as it is this branch.

I'm trying to fix it here (a couple of problems are already solved, namely a missing poetry lock, and a minimal emulator update, to pass the linter), and once the tests will pass again I'll merge in 0.2.

[alecandido]

To update whoever is reading over the state of the 0.2 branch (this is not 0.2, but it will be as soon as I will have fixed the remaining issues).

• [x] in 7b2d302 I finally solved the usual dependency hell: after upgrading the QuTiP version, I also reupgraded the CI for MacOS; by 0.2, we'll only test on MacOS ARM, and (with the specified version of QuTiP) the emulator won't be supported on MacOS x86_64 (however, QuTiP v5.0.1 most likely will work as well, and it has wheels for MacOS)
• [x] I have to fix a few tests, left after rebasing, not related to the emulator
• [ ] I need to upgrade the emulator to use 0.2 components, to pass even its tests

[alecandido]

Ok, locally all tests are passing, but those related to the emulator. It seems not to be the case in the CI, I'm not exactly sure why, but I'm trying to understand it.

If that's confirmed, I will merge this PR (stop abusing it), and open a new one to upgrade the emulator.

[alecandido]

It seems not to be the case in the CI, I'm not exactly sure why, but I'm trying to understand it.

Ok, I might have confused which was the latest run. Yes, they are actually all passing - but the doctests... of course those related to the emulator.

I will ignore it as well for the time being, and append to the dedicated issue #924.

[alecandido]

Ok, everything fixed (but the emulator, of course). And we have our green ticks back.

Let's merge this, and keep going with the next one.