Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove transpilation hook #884

Merged
merged 3 commits into from
May 10, 2024
Merged

Remove transpilation hook #884

merged 3 commits into from
May 10, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Apr 25, 2024

Closes #883

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 66.55% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alecandido.
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.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alecandido, I agree with the removal.

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.

Indeed, it certainly does not solve the issue. Currently, it is documented in the second paragraph of https://qibo.science/qibolab/stable/tutorials/compiler.html:
"Note that there is no default transpiler, therefore the backend can only execute circuits that contain native gates by default."

However, right after the above we say:
"The user can modify the transpilation and compilation process by changing the transpiler and compiler attributes of the QibolabBackend."
This is no longer true for transpiler, since this PR is removing this attribute, therefore the documentation should be updated before merging this.

We could also add the fact that only native gates are supported in qibolab in the previous example https://qibo.science/qibolab/stable/tutorials/circuits.html. Note that all circuits there are written using GPI2 which is native, but we don't explicitly say why (probably my fault).

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.

@alecandido alecandido marked this pull request as draft April 25, 2024 11:58
@alecandido
Copy link
Member Author

alecandido commented Apr 25, 2024

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?

def u3_rule(gate, platform):
"""U3 applied as RZ-RX90-RZ-RX90-RZ."""

@alecandido alecandido marked this pull request as ready for review April 25, 2024 12:45
@alecandido
Copy link
Member Author

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
Copy link
Member

stavros11 commented Apr 25, 2024

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
Copy link
Contributor

andrea-pasquale commented Apr 25, 2024

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
Copy link
Member Author

alecandido commented Apr 25, 2024

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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.

@hay-k hay-k mentioned this pull request Apr 26, 2024
4 tasks
@alecandido alecandido merged commit 128d882 into main May 10, 2024
24 checks passed
@scarrazza scarrazza deleted the remove-transpiler-hook branch June 26, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler/transpiler issue with dummy
4 participants