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

Improvements to final logical-physical qubit mapping, documentation #1562

Closed
GuillermoAbadLopez opened this issue Jan 23, 2025 · 16 comments · Fixed by #1563
Closed

Improvements to final logical-physical qubit mapping, documentation #1562

GuillermoAbadLopez opened this issue Jan 23, 2025 · 16 comments · Fixed by #1563
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@GuillermoAbadLopez
Copy link
Contributor

GuillermoAbadLopez commented Jan 23, 2025

Hello, I'm a developer for Qilimanjaro, and we are currently implementing your placing/routing into our stack,
here I leave some small improvements that would have helped me, hopefully, they are useful to you.

  • version: qibo 0.2.15

Small problems:

  1. All the docstrings for the call of the Router/Placer passes, say something similar to: "final physical-logical qubits mapping." for explaining the final mapping/layout, without explicitly stating what the keys and values are. And for an external user, although I can guess the format is: keys-values, I think stating it explicitly would be better.

  2. Assuming the order of: physical-logical in the dosctring corresponds to keys-values, then there might be a contradiction. In the Sabre router call it says logical-physical, but then the function it ends up calling, has the opposite physical-logical:

Screenshot 2025-01-22 at 10 45 50 Screenshot 2025-01-22 at 10 45 50

Possible solutions:

  1. State explicitly what are the keys and values for the final mapping, in the Router/Placer passes.
  2. Ensure consistency in the docstrings of the methods that call each other.
@GuillermoAbadLopez GuillermoAbadLopez added the enhancement New feature or request label Jan 23, 2025
@GuillermoAbadLopez
Copy link
Contributor Author

Tagging possible relevant people following PR #1500:
@stavros11 @alecandido @csookim

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 23, 2025

Opened a PR with very small changes, that make it for me.
If you think it's enough feel free to use it: #1563

@alecandido
Copy link
Member

@GuillermoAbadLopez thanks for your interest and the issue!

In a sense, the current transpiler is more or less in an intermediate stage. It is usable, but not incredibly optimized and deeply reviewed. That's just for you to gauge the depth you want to reach in there.

Moreover, Qibo is suffering from a common Python shortcoming, which is exposing essentially everything from a package, and relying on conventions for internals recognition (which are clear up to a point, and need to be supplemented with project-wide policies - ttbomu).
In particular, Qibo itself is not making (yet) any effort to clarify which is the public interface.

In this context, we (developers) are currently intending the transpiler implementation as mostly internal, while the user-facing portion should be just limited to the transpiler setup and configuration.
Specifically, you should define something like

custom_passes = [Preprocessing(), Random(), ShortestPaths(), Unroller(native_gates=natives)]

# Create a transpiler pipeline with hardware configuration
custom_pipeline = Passes(custom_passes, connectivity=star_connectivity())

https://qibo.science/qibo/stable/code-examples/advancedexamples.html#how-to-transpile-a-circuit
and then use it for transpilation just as custom_pipeline(circuit).
(I do not even guarantee that the docs are fully up-to-date with the latest changes - but they should be, mostly)

Every other function or method (i.e. other than __init__ and __call__) is considered (by us) internal for the time being. Even if it's documented (sorry, I'm just transparently declaring the level of effort).

In any case, now, the preferred way to rearrange the mapping is actually the Circuit.wire_names attribute, such that the operations should affect that. The dictionary mapping is also returned (inconsistently) by some steps. We should standardize the way it's used and produced.
But you can see how the transpilation process affects the circuits' execution

transpiled_circuit, _ = transpiler(self) # pylint: disable=E1102

The current steps should be certainly reviewed, together with the way the information is passed around. The docstrings should be generally improved, and synced with the current flow. The mechanisms for retrieving permutations (only relevant for certain steps) should be standardized (for which I'd recommend just to drop them, and rely on the input_circuit.wire_names vs output_circuit.wire_names sequences, from which is easy to construct a mapping, if needed).

Any help to improve the transpiler is vastly welcome and encouraged :)

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 24, 2025

Hey @alecandido, thank you for such a detailed answer!

transpiler is more or less in an intermediate stage. It is usable, but not incredibly optimized and deeply reviewed.

I figured when I saw that the module is not in the docs API reference, we were aware don't worry 😅. But after checking the PRs and the last version of the code, it seems to me it was usable from our side 👌

In particular, Qibo itself is not making (yet) any effort to clarify which is the public interface.

I wondered if I was being too annoying complaining about something I saw peeking at the code itself, sorry about that :)

and then use it for transpilation just as custom_pipeline(circuit).

That is exactly how we are doing things around here, so everything is perfect until here 👍


Origin of doubts:

Every other function or method (i.e. other than init and call) is considered (by us) internal for the time being.

Actually, my initial doubts (about keys/values in final_layout) came from Passes.__call__() precisely, since it's from where we were retrieving the final_layout to start with, and after doubting when reading its docstring, is when I started peeking at the code itself:

Image

I'll try:

rely on the input_circuit.wire_names vs output_circuit.wire_names sequences, from which is easy to construct a mapping

Nice, thank you for the info!
I'll definitely check it out now, and compare it against using directly the returned final_layout 👍

@alecandido
Copy link
Member

alecandido commented Jan 24, 2025

I wondered if I was being too annoying complaining about something I saw peeking at the code itself, sorry about that :)

Not at all. I'm truly glad that you showed interest, but just sorry about the limitations.

Thanks for your in-depth investigation!


In case there are things about which you're unsure (something that doesn't look clearly as an issue, or a proposal that is vague enough to call for feedback to iterate), consider to post it as a discussion https://github.com/qiboteam/qibo/discussions.
We do not have a clear policy for issues vs discussions (and, indeed, they end up being little used), but it's a more interactive channel.

If instead, at any time, you'll be looking for an actual chat, consider using our Matrix channel
https://github.com/qiboteam/qibo?tab=readme-ov-file#contacts
(even this is little used, mainly because many other people have personal contacts of some developers - but it's not necessarily optimal...)

@GuillermoAbadLopez
Copy link
Contributor Author

Cool, noted it down, thanks for everything 👍

@renatomello
Copy link
Contributor

Closed by #1563

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 25, 2025

Update:

@alecandido after trying both with the returned final_layout and with the circuit.wire_names, for our implementation it seems is better to use the final_layout, since it seems to already contain the changes of states due to the added SWAP gates, its format is not the best as you said, since a simple list containing the permutation would maybe be more clear.. but that is easily fixed with a little function in our side.

(Instead wire_names, from what I tested seems to only contain info on the initial remapping of qubits, not where the final states end after all the swaps. Basically final_layout = wire_names + SWAPs, which is what we need)

Also, I'm a bit worried about what you commented on the return type of Passes changing in each child, since seems you want to change it (which makes sense ofc). But since right now it's the most useful to us, we will use it, and whenever you change it, we will think of a better solution 👍.


Possible solution:

Maybe a possible solution would be, to add an extra wire name attribute to the circuit, that also contains the changes of states due to SWAPs, like a: circuit.swapped_wire_names/final_wire_names, I don't find any name particularly inshigtful, maybe you can come with a better one 🙌 . This way the routing and placer passes, can just change that attribute, and return the single circuit, like the rest of Passes , and we can extract from there that info.


Farewell

I didn't want to open a new discussion/Issue, since from our side it seems to be working just fine now (using the final_layout), at least in the cases I tested. So everything is good for us, and we don't need further changes, thanks! 👍

@alecandido
Copy link
Member

Also, I'm a bit worried about what you commented on the return type of Passes changing in each child, since seems you want to change it (which makes sense ofc). But since right now it's the most useful to us, we will use it, and whenever you change it, we will think of a better solution 👍.

Farewell

I didn't want to open a new discussion/Issue, since from our side it seems to be working just fine now (using the final_layout), at least in the cases I tested. So everything is good for us, and we don't need further changes, thanks! 👍

@GuillermoAbadLopez no worries, it's not going to happen anytime soon, and if you wish you can be part of the process (or even lead it).

For as long as it is not changed, you'll be free to use it at will.

Moreover, while Qibo itself is now pretty lenient on semantic versioning (by contrast, Qibolab is now much stricter - and sooner or later even Qibo will follow suit), we're still versioning.
So, if anything changes, you may limit the version you use/depend on for some time.

(Instead wire_names, from what I tested seems to only contain info on the initial remapping of qubits, not where the final states end after all the swaps. Basically final_layout = wire_names + SWAPs, which is what we need)

Possible solution:

Maybe a possible solution would be, to add an extra wire name attribute to the circuit, that also contains the changes of states due to SWAPs, like a: circuit.swapped_wire_names/final_wire_names, I don't find any name particularly inshigtful, maybe you can come with a better one 🙌 . This way the routing and placer passes, can just change that attribute, and return the single circuit, like the rest of Passes , and we can extract from there that info.

The problem I see is that you are following the path of "qubit tracking" during the circuit operation.
However, while it is meaningful during the transpilation phase (and especially certain steps), swaps are only logical, and they can be used for different purposes than routing qubits, and even composed/decomposed by/to further gates, which makes their role much less clear (you can certainly create SWAPs out of CNOTs - but if you have CNOTs around, do you actually have SWAPs? or part of them?).

So, to me, the .wire_names is just the initial remapping, but it is also the only part consisting in an actual remapping. While everything else are arbitrary parts of a unitary, which I'm not sure whether is worth tracking more than that...

In any case, I never went too deep into transpilation, so I'm certainly up for discussion, and open to insightful inputs!

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 27, 2025

@GuillermoAbadLopez no worries, it's not going to happen anytime soon, and if you wish you can be part of the process (or even lead it).

@alecandido nice to hear, and I will gladly take part in it!
But I don't think I'll be able to lead much, except for my company's priorities 😅

So, if anything changes, you may limit the version you use/depend on for some time.

Right


The problem I see is that you are following the path of "qubit tracking" during the circuit operation.
However, while it is meaningful during the transpilation phase (and especially certain steps), swaps are only logical, and they can be used for different purposes than routing qubits, and even composed/decomposed by/to further gates, which makes their role much less clear (you can certainly create SWAPs out of CNOTs - but if you have CNOTs around, do you actually have SWAPs? or part of them?).

So, to me, the .wire_names is just the initial remapping, but it is also the only part consisting in an actual remapping. While everything else are arbitrary parts of a unitary, which I'm not sure whether is worth tracking more than that...

Right, it is truly not trivial, but if you specifically are adding swaps to the original circuit of the user, you can maybe argue, that those are remapping the circuit while the others don't. Even more if you do it automatically without the user knowing for example.

Which can be our case, since we want to automatically place/route a circuit to our backends, with the user only passing a simple flag route=True. And then, in the end, we want to automatically undo the reordering of the execution results, so we give them back ordered as in the original circuit, so the user, doesn't need to make any extra effort.

(But ofc, we will always inform of the used qubits, right now I'm implementing to raise info's to show both wire_names and final_layout with a short explanation. Or also let them manually choose the placement if they are more advanced users, but we want to remain the default case simple, for those who are not).

@alecandido
Copy link
Member

But I don't think I'll be able to lead much, except for my company's priorities 😅

Not expecting anything beyond that. I don't know to what extent this transpilation support is among your company's priorities, but the message was that in case you'll be free to contribute to Qibo as well for that purpose (and we'll be glad to accept it).

Right, it is truly not trivial, but if you specifically are adding swaps to the original circuit of the user, you can maybe argue, that those are remapping the circuit while the others don't. Even more if you do it automatically without the user knowing for example.

Which can be our case, since we want to automatically place/route a circuit to our backends, with the user only passing a simple flag route=True. And then, in the end, we want to automatically undo the reordering of the execution results, so we give them back ordered as in the original circuit, so the user, doesn't need to make any extra effort.

(But ofc, we will always inform of the used qubits, right now I'm implementing to raise info's to show both wire_names and final_layout with a short explanation. Or also let them manually choose the placement if they are more advanced users, but we want to remain the default case simple, for those who are not).

True, but that is potentially unstable if you then attempt optimizing your gates, since the swap you introduced may be further decomposed and then partly optimized away.

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 27, 2025

Not expecting anything beyond that. I don't know to what extent this transpilation support is among your company's priorities, but the message was that in case you'll be free to contribute to Qibo as well for that purpose (and we'll be glad to accept it).

🙌 🙌

True, but that is potentially unstable if you then attempt optimizing your gates, since the swap you introduced may be further decomposed and then partly optimized away.

Right, but any further optimization should have the same global unitary, for the same order of qubits concretely, no?
So even if those SWAPs end up disappearing, the action of adding them should be the only important thing for changing the final_layout IMO

So only changing the final_layout from the wire_names at each step that you add a SWAP in the routing, and not due to the original SWAPs present in the circuit, or when all of these might disappear later on the optimization (not re-routing), should be the most intuitive, no? 🤔

@GuillermoAbadLopez
Copy link
Contributor Author

@alecandido if you are curious about how we are going to use it, you can take a look in: qililab.digital.circuit_router(branch: update_qibo_version_transpiler) 👍

(Take into account, that the PR is still not ready for merge though)

@alecandido
Copy link
Member

alecandido commented Jan 28, 2025

Correct. In my former comment, I focused on the unitary input, and forgot that you also need to permute its output (either manually, which is the simplest, or by further SWAPs - which is quite silly, since they would be virtual anyhow).

So, while the input permutation is encoded in the .wire_names, the output one has to be tracked (unless we impose the two to be the same, which seems also pretty silly, since it would increase the number of SWAPs). And we should return an identity in those cases where it is untouched, to make the steps uniform.

@GuillermoAbadLopez
Copy link
Contributor Author

GuillermoAbadLopez commented Jan 31, 2025

@alecandido right,

In the end, it was handy to have both the wire_names and the final_layout, since we ended up using both:

  • wire_names: to map the switched qubits, for running the execution in the physical qubits
  • final_layout: to unmap the results after the executions, where the states moved around with the added SWAPs, so we could return it to the user in his originally sent order

But of course, maybe you could encode the final_layout as an attribute of the circuit itself, resolving the problem of different returns for each pass 🙌

Hopefully, this feedback is useful, thanks for building such a cool library 👍

@alecandido
Copy link
Member

But of course, maybe you could encode the final_layout as an attribute of the circuit itself, resolving the problem of different returns for each pass 🙌

Yeah, indeed. Either that, or we should always return the final_layout.
But, to be fair, even encoding as an attribute would make sense, since it will be relevant during any execution (any time you execute the transpiled circuit, you may want to rearrange the result back, in order to match the original qubits before transpilation).

But here I will ask for @stavros11's opinion as well :)

Hopefully, this feedback is useful, thanks for building such a cool library 👍

You're more than welcome ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants