qiboteam / qibolab

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

Remove transpilation hook #884

Closed alecandido closed 1 month ago

alecandido commented 2 months ago

Closes #883

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.55%. Comparing base (a514441) to head (dca5bd7).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #884 +/- ## ========================================== - Coverage 66.59% 66.55% -0.04% ========================================== Files 55 55 Lines 5942 5932 -10 ========================================== - Hits 3957 3948 -9 + Misses 1985 1984 -1 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/884/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/884/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.55% <100.00%> (-0.04%)` | :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 commented 2 months ago

Technically it doesn't solve the issue since the error is still there if the circuit contains non-native gates. If, as agreed, we need to use only native gates we should document it properly.

It doesn't "solve" the issue, but it should close it (since it is not going to be solved by Qibolab anyhow).

Indeed, it certainly does not solve the issue. Currently, it is documented in the second paragraph of qibo.science/qibolab/stable/tutorials/compiler.html:

I'm sorry, I grepped for it, but for some reason it failed to find the occurrences (and it was strange, I'm still wondering why rg transp failed...). I will certainly amend the docs, I intended to do it.

Aside from this PR, we could also consider making X gate native by adding the corresponding rule in compiler. It is just the pi-pulse anyway, and could be useful for some applications.

Shouldn't we avoid adding too many compilation rules, and rely on a minimal set of natives?

E.g., even the U3 should never arrive to Qibolab, since it should be transpiled to RZ and RX90 before, isn't it? https://github.com/qiboteam/qibolab/blob/384a029032c6bffbe14ab94ed13e6c55bda99d24/src/qibolab/compilers/default.py#L52-L53

alecandido commented 2 months ago

Technically it doesn't solve the issue since the error is still there if the circuit contains non-native gates. If, as agreed, we need to use only native gates we should document it properly.

Could you check that now it is clear for you from the docs that the transpilation has to come from Qibo?

stavros11 commented 2 months ago

Shouldn't we avoid adding too many compilation rules, and rely on a minimal set of natives?

Yes, but the case of X is fairly simple and is really a native gate. In some sense even more native than the RX90, as the pi-pulse (not the pi-half) is what we are really calibrating with our current workflow (as far as I am aware).

E.g., even the U3 should never arrive to Qibolab, since it should be transpiled to RZ and RX90 before, isn't it?

I agree that U3 rule should be removed and the transpiler should take care of transforming it to RZ and GPI2.

andrea-pasquale commented 2 months ago

Technically it doesn't solve the issue since the error is still there if the circuit contains non-native gates. If, as agreed, we need to use only native gates we should document it properly.

Could you check that now it is clear for you from the docs that the transpilation has to come from Qibo?

This part is now clear. However, (I guess) in qibo we are missing a proper example on how to execute on hardware arbitrary circuits using the transpiler. There is only a section on how to modify the transpiler https://qibo.science/qibo/latest/code-examples/advancedexamples.html#how-to-modify-the-transpiler

Moreover, I think that a naive user shouldn't touch the transpiler at all in order to execute on hardware. I feel like all the transpiler business should be performed by an expert user. Considering also the possibility to have cloud access to quantum hardware making the transpiler manipulations mandatory feels like an overkill to me. I think that we need to have a way to have a default transpiler regardless of where this default transpiler is set (qibo, qibolab or qibocal).

alecandido commented 2 months ago

Moreover, I think that a naive user shouldn't touch the transpiler at all in order to execute on hardware. I feel like all the transpiler business should be performed by an expert user. Considering also the possibility to have cloud access to quantum hardware making the transpiler manipulations mandatory feels like an overkill to me.

Perfect example: it doesn't have to be the case, it could be performed by qibo-cloud-backends using Qibo :)

I think that we need to have a way to have a default transpiler regardless of where this default transpiler is set (qibo, qibolab or qibocal).

We can even engineer a backend for that, but it should be decoupled from the qibolab package, to maintain the scope. At the end of the day, it is just UI (that is relevant, but separate from execution). So, if it's a problem for the end user, we should do it in the interface the end user is going to directly touch (which is unlikely to be Qibolab, if he's coming with a non-native circuit).

andrea-pasquale commented 2 months ago

The naive user is not only the cloud user. Also people that have access to qrccluster might want to execute a circuit on hardware, for example just to run some algorithms. Another example could be people that want to write benchmarking protocols in qibocal. I understand that you want to try to have qibolab as decoupled as possible but having a backend that is only able to execute native gates it feels very limiting. In my eyes I see this backend that can only executing native gates as a special case of the backend where a transpiler is already present and you simply disable the transpilation.

alecandido commented 2 months ago

The naive user is not only the cloud user. Also people that have access to qrccluster might want to execute a circuit on hardware, for example just to run some algorithms.

Well, you can have a non-cloud backend similar to the cloud one. And it could be in Qibo itself, no problem about that.

Another example could be people that want to write benchmarking protocols in qibocal.

This is exactly the user I'd like not to account for: that's not an end user, but an internal user (Qibocal), and assumed to be an advanced one. In that case, if the transpilation is present, I believe it should be explicit in the protocol, and not happening implicitly somewhere else.

However, if the Qibocal developers then prefer to make it implicit somewhere else in Qibocal I can not prevent it (though I can cast a vote against even there, but that's a separate story :P).

alecandido commented 2 months ago

I understand that you want to try to have qibolab as decoupled as possible but having a backend that is only able to execute native gates it feels very limiting.

Well, it's not limiting, you can implement whatever you wish on top, it's just a higher level (like you're not implementing HTTP in the data link layer, to better separate concerns and have a solid foundation).

In my eyes I see this backend that can only executing native gates as a special case of the backend where a transpiler is already present and you simply disable the transpilation.

This is the opposite of modularity: if you build something capable of doing everything with the correct toggles, it will just become a giant monolith.