search

qiboteam / qibo

A full-stack framework for quantum computing.
https://qibo.science
Apache License 2.0
294 stars 60 forks source link

Internal global backend `_Global` #1440

Closed csookim closed 3 weeks ago

csookim commented 2 months ago

This covers issue #1309.

Checklist:

Todo:

csookim commented 2 months ago

@alecandido I have implemented the items we discussed. There are three additional things to fix.

Todo:

  • [ ] Add default transpiler.
  • [ ] Update all documentation containing qibo.backends.GlobalBackend.
  • [ ] Use _Global.set_transpiler in error_mitigation.py.

1

Is it ok to set the default transpiler for hardware backends in resolve_global()? The logic would first check whether the backend is for simulation/hw, and then set the trivial Passes. To perform this check, an intermediate class like SimulationBackend might be needed.

https://github.com/qiboteam/qibo/blob/2419d7d22dd516d931ca99c4015d1e40435dd29a/src/qibo/backends/__init__.py#L142-L147

2

Since GlobalBackend is deprecated, the docstrings referencing GlobalBackend need to be updated.

All the docstrings follow a similar format, so is it okay to update them to:

3

In error_mitigation.py, the backend currently has transpiler: Passes. I believe this is incorrect since the QibolabBackend does not have a transpiler. Should we fix this by using _transpiler from _Global?

https://github.com/qiboteam/qibo/blob/2419d7d22dd516d931ca99c4015d1e40435dd29a/src/qibo/models/error_mitigation.py#L1099-L1103

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 99.93%. Comparing base (fbc49be) to head (65bdfee). Report is 120 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1440 +/- ## ======================================= Coverage 99.93% 99.93% ======================================= Files 81 81 Lines 11795 11859 +64 ======================================= + Hits 11787 11851 +64 Misses 8 8 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibo/pull/1440/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/qibo/pull/1440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `99.93% <100.00%> (+<0.01%)` | :arrow_up: | 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 1 month ago

1

Is it ok to set the default transpiler for hardware backends in resolve_global()? The logic would first check whether the backend is for simulation/hw, and then set the trivial Passes. To perform this check, an intermediate class like SimulationBackend might be needed.

It is definitely acceptable, and the class is needed in any case.

But you could do even in .transpiler() itself, I'm not sure you really need .resolve_global() at all

alecandido commented 1 month ago

3

In error_mitigation.py, the backend currently has transpiler: Passes. I believe this is incorrect since the QibolabBackend does not have a transpiler. Should we fix this by using _transpiler from _Global?

Yes, definitely. I believe this is just a leftover from when the transpiler was inside Qibolab.

alecandido commented 1 month ago

2

Since GlobalBackend is deprecated, the docstrings referencing GlobalBackend need to be updated.

  • by specifying the engine used for calculation, if not provided the current :class:qibo.backends.GlobalBackend is used
  • backend (:class:qibo.backends.abstract.Backend, optional): backend to be used in the execution. if None, it uses :class:qibo.backends.GlobalBackend. Defaults to None.

All the docstrings follow a similar format, so is it okay to update them to:

  • by specifying the engine used for calculation, if not provided the current :class:qibo.backends.abstract.Backend is used
  • backend (:class:qibo.backends.abstract.Backend, optional): backend to be used in the execution. if None, the current backend is used. Defaults to None.

I believe you should use your solution for the second point even in the first one.

I.e. qibo.backends.abstract.Backend is the base class, not a replacement for qibo.backends.GlobalBackend. Just write in words the current backend - at most, it could be cross-linked to another docs page explaining the automated backend selection.

alecandido commented 1 month ago

Btw, as I stressed in the review, docs is poor (it was even before the PR, but it's worth some effort), and the tests are not yet passing.

csookim commented 1 month ago

1

Is it ok to set the default transpiler for hardware backends in resolve_global()? The logic would first check whether the backend is for simulation/hw, and then set the trivial Passes. To perform this check, an intermediate class like SimulationBackend might be needed.

It is definitely acceptable, and the class is needed in any case.

But you could do even in .transpiler() itself, I'm not sure you really need .resolve_global() at all

I removed resolve_global because it's just a combination of _Global.backend() and _Global.transpiler(). Also, _Global.transpiler() automatically adds the default transpiler.

3

In error_mitigation.py, the backend currently has transpiler: Passes. I believe this is incorrect since the QibolabBackend does not have a transpiler. Should we fix this by using _transpiler from _Global?

Yes, definitely. I believe this is just a leftover from when the transpiler was inside Qibolab.

I updated error_mitigation.py, but the part of the code that accesses the hardware topology still needs to be updated.

2

Since GlobalBackend is deprecated, the docstrings referencing GlobalBackend need to be updated.

  • by specifying the engine used for calculation, if not provided the current :class:qibo.backends.GlobalBackend is used
  • backend (:class:qibo.backends.abstract.Backend, optional): backend to be used in the execution. if None, it uses :class:qibo.backends.GlobalBackend. Defaults to None.

All the docstrings follow a similar format, so is it okay to update them to:

  • by specifying the engine used for calculation, if not provided the current :class:qibo.backends.abstract.Backend is used
  • backend (:class:qibo.backends.abstract.Backend, optional): backend to be used in the execution. if None, the current backend is used. Defaults to None.

I believe you should use your solution for the second point even in the first one.

I.e. qibo.backends.abstract.Backend is the base class, not a replacement for qibo.backends.GlobalBackend. Just write in words the current backend - at most, it could be cross-linked to another docs page explaining the automated backend selection.

I updated the docstrings.

csookim commented 1 month ago

@alecandido Thanks for your review! Most changes are minor, but there are a few points we need to discuss:

  1. When initializing and retrieving the backend, which should we use: qibo.get_backend() or _Global.backend()? They both do the same thing. This is related to #U1.

  2. Some test files modify the backend or transpiler. When running pytest, those changes can affect other tests. Is there a good way to handle this? I implemented _clear_global to resolve this issue.

https://github.com/qiboteam/qibo/blob/5c32260d4338e8f00a65e168ae49244e693df6cb/src/qibo/backends/__init__.py#L136-L143

https://github.com/qiboteam/qibo/blob/5c32260d4338e8f00a65e168ae49244e693df6cb/tests/test_backends_qibotn.py#L13-L17

  1. The packages need to be reorganized in another PR.
alecandido commented 1 month ago 
# qibo package

class Backend:
    @property
    @abstractmethod
    def qubits(self) -> Optional[list[QubitIdentifier]]:
         pass

    ...

# ---------------------
# whatever simulation backend, provided internally or externally

class SimulationBackend:
    @property
    def qubits(self) -> Optional[list[QubitIdentifier]]:
         return None

# ---------------------
# qibolab package

class QibolabBackend:
    @property
    def qubits(self) -> Optional[list[QubitIdentifier]]:
         return self.platform.qubits

where QubitIdentifier is a placeholder, and it could well be int, or the name used by the wires (if you'll take care of the conversion).

This is more or less the proposal for the qubits, and you can generalize for the connectivity and native gates.

The idea is that None is telling that there are no restrictions for that item. Now, we could provide a more complex layout for a negative restriction (like "all qubits but these"), and we could work out how to impose some restrictions' absences, but not all. For the time being, a simpler prototype can assume that you'll receive all None or not None values from the properties you need (qubits, connectivity, native gates), and otherwise fail. If you receive all non-None, then you create the transpiler based on the received properties. If they are all None, then you just skip transpilation (so use a trivial transpiler, compatible with the transpiler interface, but returning circuits completely untouched - i.e. the identity function).

Further refinements could be based on storing the None values in a transpiler wrapper, and replacing them when the transpilation is requested, by instantiating an actual transpiler within the wrapper based on the circuit information (like: "if qubits is None, replace it with Circuit.qubits when you're passed a circuit", or something like that). But it could be treated as an enhancement, and postponed to a later PR, just opening a dedicated issue.

csookim commented 1 month ago

1

Is it ok to implement SimulationBackend and QibolabBackend in abstract.py?

2

where QubitIdentifier is a placeholder, and it could well be int, or the name used by the wires (if you'll take care of the conversion).

Since the return value of platform.qubits is QubitMap = dict[QubitId, Qubit] in qibolab, and other hardware backends may have different types of qubit classes, should we create a new class Qubit in Qibo?

In the future, we may need additional qubit information for an advanced transpiler, but currently, using just str seems enough. Is this appropriate?

Qubit = str
...
class Backend(abc.ABC):
...
    @property
    @abc.abstractmethod
    def qubits(self) -> Optional[list[Qubit]]: # pragma: no cover
        """Return the qubits of the backend."""
        raise_error(NotImplementedError)

Or we can simply remove the return type hint.

alecandido commented 1 month ago

Is it ok to implement SimulationBackend and QibolabBackend in abstract.py?

They should not be implemented there, as they are already implemented somewhere else.

The QibolabBackend is part of qibolab (i.e. qibolab.QibolabBackend), and SimulationBackend is a placeholder for NumpyBackend/TensorflowBackend/CupyBackend/... We don't need any further class (for the time being). In practice, most of the backends inherit the NumpyBackend, so it will be sufficient to implement the actual properties in the NumpyBackend and QibolabBackend, and override only if a backend has specific transpilation needs (notice that even QibolabBackend itself is inheriting from NumpyBackend, so the implementation in qibolab will already be an example of an override).

alecandido commented 1 month ago

Since the return value of platform.qubits is QubitMap = dict[QubitId, Qubit] in qibolab, and other hardware backends may have different types of qubit classes, should we create a new class Qubit in Qibo?

In the future, we may need additional qubit information for an advanced transpiler, but currently, using just str seems enough. Is this appropriate?

Qubit = str
...
class Backend(abc.ABC):
...
    @property
    @abc.abstractmethod
    def qubits(self) -> Optional[list[Qubit]]: # pragma: no cover
        """Return the qubits of the backend."""
        raise_error(NotImplementedError)

Here, what to do depends more on Qibo than Qibolab.

Circuits make use of just int in practice, so you may want to ask the backends directly for int, doing the conversions in the backends. However, the circuits also support wire_names https://github.com/qiboteam/qibo/blob/36ed3fe6ceebdf044fa8f9c115f8586c91e3597a/src/qibo/models/circuit.py#L282-L283 Thus, you may decide to accept in Qibo both int and other identifiers, which should be interpreted as wire_names. You could even choose to allow just int and str (the first to be interpreted as bare indices, the second as wire names), or to accept any Python object as a wire name (at that point even ints should be considered wire names, and bare indices will be inaccessible). I would discourage this latter option (you may interpret everything as wire name anyhow, but going beyond int and str may cause problems later on).

In any case, what Qibolab is doing, and how it is internally labelling the qubits, it should not be a concern for Qibo, which should be the one imposing the interface (especially, you should convert to Qibo classes, and do not leak Qibolab classes to the Qibo interface - though QubitId is just an alias, but that's not something you should rely on).

csookim commented 1 month ago

If we allow int and str as qubit types, we don't need to include platform.qubits, since the connectivity already contains qubit names. The implementation will be as follows:

# abstract.py
class Backend(abc.ABC):
    ...
    @property
    @abc.abstractmethod
    def natives(self) -> Optional[NativeGates]: # pragma: no cover
        """Return the native gates of the backend. If :class:`SimulationBackend`, return None."""
        raise_error(NotImplementedError)

    @property
    @abc.abstractmethod
    def connectivity(self) -> Optional[nx.Graph]:
        """Return the available qubit pairs of the backend. If :class:`SimulationBackend`, return None."""
        raise_error(NotImplementedError)
# numpy.py
class NumpyBackend(Backend):
    ...
    @property
    def natives(self) -> Optional[NativeGates]:
        return None

    @property
    def connectivity(self) -> Optional[nx.Graph]:
        return None
# __init__.py
class _Global:
    ...
    @classmethod
    def transpiler(cls):
        ...
        natives = cls._backend.natives
        connectivity = cls._backend.connectivity
        if natives and connectivity:
            # hw transpiler
        else:
            # trivial transpiler

And QibolabBackend implementation in qibolab. Are there additional parts needed?

alecandido commented 1 month ago

NativeGates are a Qibolab class, and essentially not what you need. Same for the graph, at interface level requesting a networkx object would propagate networkx as a direct dependency of all packages implementing backends, which is unneeded.

Most likely, what you need for natives is just a list[str], containing just the names of the allowed native gates (we don't really have an enumeration of all the gates supported by Qibo, so requesting a str is the best you can do), and concerning the connectivity just a list[tuple[QubitIdentifier, QubitIdentifier]], whatever it is the qubit identifier.

Regarding exactly what a QubitIdentifier should be, it's true that the connectivity in the transpiler may already contain non-int names, but I'd double-check whether they are not in one-to-one correspondence with plain integers (at some point, they were just f"q{n}", so they were integers masked as str). Here the relevant bit is not much trusting the bare type used anywhere, which may be unreliable, but making sure that the information contained in Circuit.wire_names is correctly propagated and consumed. I.e. for as long as you ignore Circuit.wire_names, you should just support int and nothing else, since this is what you will get from your circuits. If instead the names are taken into account, you are then allowed to target specific qubit names in the platform (then, you can specify the connectivity with platforms' native names, or by using the indices as well - but if you have to do that manually, it's a bit silly to not use the most familiar names, that are the native ones).

csookim commented 1 month ago

NativeGates are a Qibolab class, and essentially not what you need. Same for the graph, at interface level requesting a networkx object would propagate networkx as a direct dependency of all packages implementing backends, which is unneeded.

I used NativeGates in qibo.transpiler.unroller. Does this class originate from qibolab? However, since it creates dependency issues, it would be better to use just strings instead.

alecandido commented 1 month ago

I used NativeGates in qibo.transpiler.unroller. Does this class originate from qibolab? However, since it creates dependency issues, it would be better to use just strings instead.

Ok, sorry. It exists a class by the same name, but the unroller one is fine, being a Qibo class (if possible, better to expose that top-level in Qibo, if it ought to be used by the other packages providing backends).

csookim commented 1 month ago

Regarding exactly what a QubitIdentifier should be, it's true that the connectivity in the transpiler may already contain non-int names, but I'd double-check whether they are not in one-to-one correspondence with plain integers (at some point, they were just f"q{n}", so they were integers masked as str). Here the relevant bit is not much trusting the bare type used anywhere, which may be unreliable, but making sure that the information contained in Circuit.wire_names is correctly propagated and consumed. I.e. for as long as you ignore Circuit.wire_names, you should just support int and nothing else, since this is what you will get from your circuits. If instead the names are taken into account, you are then allowed to target specific qubit names in the platform (then, you can specify the connectivity with platforms' native names, or by using the indices as well - but if you have to do that manually, it's a bit silly to not use the most familiar names, that are the native ones).

In qibolab, qubit names are handled as QubitId = Annotated[Union[int, str], Field(union_mode="left_to_right")], allowing both str and int. I believe this should be reflected into qibo as well. Also, the definition of wire_names is unclear.

            wire_names (list or dict, optional): Names for qubit wires.
            If ``None``, defaults to (``q0``, ``q1``... ``qn``).
            If ``list`` is passed, length of ``list`` must match ``nqubits``.
            If ``dict`` is passed, the keys should match the default pattern.
            Defaults to ``None``.

Regarding the Placer and Router, they automatically manage qubits by converting their names from str/int to int during their calculations.

alecandido commented 1 month ago

In qibolab, qubit names are handled as QubitId = Annotated[Union[int, str], Field(union_mode="left_to_right")], allowing both str and int. I believe this should be reflected into qibo as well.

Ideally yes, in practice no. I.e. if we want to address qubits by name, for sure we need to be able to propagate the name. However, this choice is up to Qibo, and the presence of names in Qibolab may be ignored, trading them by their indices in the platform.qubits. Which is what is being right now. This is less than ideal, but a proper solution has to involve wire_names, because otherwise you have no way to direct circuits' qubits to platforms' qubits, that is what you want to do, eventually.

Also, the definition of wire_names is unclear.

You may open an issue, or directly a PR to clarify the docstring, but the meaning of the current one is the following:

This usage of f"q{n}" instead of just n is a bit confusing (and it's also what is making them str instead of int, most often). Also, supporting both list and dict seems smart, because one general rule is that you should be compatible with as much user input as possible, and very precise about the return type. However, while this is nice for UI, it often results in overhead for libraries, so I'd rather encourage being precise even in the input types (a list l of names could be easily converted to the required dictionary just using dict(zip(range(len(l)), l))).

In any case, the source of confusion from wire_names is mostly coming from them being mainly used to draw the circuits. We can change them, but it's not the purpose of this PR. If we have to use some symbolic names in the circuits' execution, they have to be the wire_names, since they are already there. Using them may be a bit confusing, but having two sets of symbolic names (beyond the indices) would be exceedingly confusing.

Regarding the Placer and Router, they automatically manage qubits by converting their names from str/int to int during their calculations.

I didn't review all of them, but the ones I know (and I'd extrapolate to the others) are pretty dumb: https://github.com/qiboteam/qibo/blob/36ed3fe6ceebdf044fa8f9c115f8586c91e3597a/src/qibo/transpiler/placer.py#L149 So, you can't rely on them to use a qubit name "B2", because their translation is always mapping 7 to a qubit named "q7", so you then need to map "q2" to "A2" and "q7" to "B2". That's a good reason to always use int inside the transpiler, since it's internal, and only using symbolic names for interfacing (to the user writing the circuit, with the other "user" writing the platform - while internally we're always exchanging int, which are easier and more homogeneous to manipulate).

Note: the original argument for using these f"q{n}" inside the transpiler was that it makes it easier to debug, since when you print the circuit that is what will be drawn - which I consider pretty irrelevant as an argument (we could even drop the q from the drawing, i.e. the wire_names, but that is fully unrelated to using them internally)

csookim commented 1 month ago

In my latest two commits, I proposed a prototype for the hardware connection.

Transpiler Summary

connectivity refers to the hardware configuration and the nodes must be integers. In qibo, nodes i are interpreted as physical qubit names: "q" + str(i).

https://github.com/qiboteam/qibo/issues/1429#issuecomment-2317463590

Before transpilation, the circuit's gates use the logical qubits specified by the user. After transpilation, the gates are updated with the appropriate physical qubits determined by the transpiler.

# Example
connectivity = nx.Graph() # 0 -- 1 -- 2 -- 3 -- 4
circuit.wire_names = ["q0", "q1", "q2", "q3", "q4"]
circuit.queue = [CX(0, 1)]    # logical

# after transpilation
circuit.queue = [SWAP(1, 2), CX(2, 3)]    # physical
# same meaning as [SWAP("q1", "q2"), CX("q2", "q3")]

Default Transpiler

In _Global, the hardware backend's qubit names will be mapped to q{i}, and qibo.Gate.qubits will be used to directly reference qubits in qibolab.platform.qubits.

def _default_transpiler(cls):
...
    qubits: list[str] = list(cls._backend.qubits)  # list of qubit names 
    connectivity_edges: list[tuple[str, str]] = (
        cls._backend.connectivity
    )  # list of edges

...

    # q{i} naming
    node_mapping = {qubits[i]: i for i in range(len(qubits))}
    for e in connectivity_edges:
        connectivity.add_edge(node_mapping[e[0]], node_mapping[e[1]])

qibo.Gate.qubits directly indicates the index of qibolab.platform.qubits.

Example

This maps to ["q0", "q1", "q2", "q3", "q4"], and the connectivity becomes 0 -- 1 -- 2 -- 3 -- 4 in _default_transpiler.

If transpiled_circuit.queue is [SWAP(1, 2), CX(2, 3)], this is equivalent to [SWAP("q1", "q2"), CX("q2", "q3")], which corresponds to [SWAP("A2", "B1"), CX("B1", "B2")].

Discussion

https://github.com/qiboteam/qibolab/blob/31b457122c94371583c25207b4d3c0d91a753a20/src/qibolab/_core/platform/platform.py#L317-L323

alecandido commented 1 month ago
  • Physical qubit i corresponds to the index of wire_names.
  • wire_names are mapped to the hardware backend's qubit names.

The transpiler (specifically the router) is fully ignoring what was originally written in the circuit.wire_names, and just using an attribute by the same name https://github.com/qiboteam/qibo/blob/eba4f5ee6e199055c9924ef123f51018c5ed9e29/src/qibo/transpiler/router.py#L222 initialized from a further CircuitMap parameter https://github.com/qiboteam/qibo/blob/eba4f5ee6e199055c9924ef123f51018c5ed9e29/src/qibo/transpiler/router.py#L197 used to establish the final mapping https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/router.py#L302-L305

So, the Circuit.wire_names attribute, which is already such a mapping, and it would allow the user to select the qubits to be used, is pretty much ignored.

On the one side, it may be argued that it is a bit confusing to have the user assigning physical qubits' names to the individual wires, since they may be reshuffled during transpilation, so you can not really impose to execute a certain gate on a certain physical qubit. Still, the role of initial_layout is the exact same, and it's not stored in the original circuit, so it's a further information beyond the Circuit that the user has to specify to execute, otherwise getting a default for the first nqubits, while a user may certainly want to be able to select which qubits to execute on. And with Circuit.wire_names around, using both would be quite confusing...

Moreover, the initial_layout attribute of CircuitMap is set by the Router, in which is a parameter to Router.__call__() https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/abstract.py#L32-L34 but this is not getting exposed by the Passes interface: https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/pipeline.py#L222 it is only present as an attribute https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/pipeline.py#L200 not present in the initializer parameters https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/pipeline.py#L187-L194 and even if you set it, it would be overwritten at the very beginning of the transpilation process https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/pipeline.py#L228

So, with the current transpiler, there is just no way for the user to select which qubits to use of certain platform (the only option is modifying the platform).

alecandido commented 1 month ago
  • platform.qubits is a dictionary, and the qubit order will be implicitly set by using list(cls._backend.qubits) in _Global._default_transpiler() in qibo. In qibolab, qubit Qubits will be indexed with list(self.qubits.items())[qubit]. Would the order of platform.qubits change during calibration etc?

qiboteam/qibolab@31b4571/src/qibolab/_core/platform/platform.py#L317-L323

As said before, you should not take into account the Qibolab's Platform, just extract from there the information you need to transpile (exposing it in the QibolabBackend). The transpiler should not be tailored on Qibolab.

In particular, you only need to extract the qubit names from the platform, and not the objects. The order in a platform is fixed, and both the names and their order is fixed (the calibration process optimizes the parameters affecting experiments' execution, but the arrangement is just a representation feature, so there is no reason to change it).

csookim commented 1 month ago

So, the Circuit.wire_names attribute, which is already such a mapping, and it would allow the user to select the qubits to be used, is pretty much ignored.

Yes, if you run the router, circuit.wire_names is overwritten by initial_layout. Names in initial_layout(connectivity) take higher priority. (initial_layout should be sorted. I'll add sorting in the code below.)

https://github.com/qiboteam/qibo/blob/eba4f5ee6e199055c9924ef123f51018c5ed9e29/src/qibo/transpiler/router.py#L222

https://github.com/qiboteam/qibo/blob/eba4f5ee6e199055c9924ef123f51018c5ed9e29/src/qibo/transpiler/router.py#L713-L715

So, with the current transpiler, there is just no way for the user to select which qubits to use of certain platform (the only option is modifying the platform).

We also need to enable string names in qibo. There should be a restriction that the names in Circuit.wire_names must match both the connectivity from the hardware and the initial_layout provided by the user, if they want to run the transpiler. Qibo should provide an interface where gates.qubits in circuit.queue correspond to the index of circuit.wire_names, allowing hardware backends to correctly access the physical qubit names.

csookim commented 1 month ago

If the connection is urgent, the current implementation should be enough for qibolab 0.2.0. Also, a function to convert the native gates from str to enum is needed in qibo.NativeGates.

alecandido commented 1 month ago

Yes, if you run the router, circuit.wire_names is overwritten by initial_layout. Names in initial_layout(connectivity) take higher priority. (initial_layout should be sorted. I'll add sorting in the code below.)

Pay attention: the input Circuit.wire_names should be left unaltered (you can confirm it), the manipulated initial_layout (the one worked out by the Router) is written to the output circuit, which is always a new one https://github.com/qiboteam/qibo/blob/9863ca1cb9a44125262aa17f0681d151e7c6f503/src/qibo/transpiler/blocks.py#L153-L156 you can check starting from the Router.__call__() method: https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/router.py#L409 https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/router.py#L300

We also need to enable string names in qibo.

The string names are the Circuit.wire_names (currently unused by the transpiler, but they are implementing the feature).

There should be a restriction that the names in Circuit.wire_names must match both the connectivity from the hardware and the initial_layout provided by the user, if they want to run the transpiler.

Well, this is a transpiler check that should be implemented. Note that it may be already implemented somewhere https://github.com/qiboteam/qibo/blob/d29060164515e620b007ff3c3cf854a688ba6fac/src/qibo/transpiler/placer.py#L40 But it may be necessary to use it even in further places, split, or improved (or written from scratch, if the check you have in mind is not yet there).

Qibo should provide an interface where gates.qubits in circuit.queue correspond to the index of circuit.wire_names, allowing hardware backends to correctly access the physical qubit names.

That's already the case. The qubits in the gates are just ints, and they are intended to run from 0 to nqubits - 1. They are the only ones used right now (both in simulation and on hardware), and Circuit.wire_names is simply ignored during execution. So, it would be sufficient to make a consistent use of Circuit.wire_names in the transpiler, which is the yet missing part.

I should also review the QibolabBackend, since str names are supported in the platform, but most likely not all in the Circuit (i.e. wire_names is fully ignored even there).

csookim commented 1 month ago

So, it would be sufficient to make a consistent use of Circuit.wire_names in the transpiler, which is the yet missing part.

So, in terms of qubit representation, qibo should retrieve qubits: list[str] and connectivity: list[Tuple[str, str]] from the hardware backend and return circuit.queue[i].qubits: list[int] and circuit.wire_names: list[str]. The circuit.queue[i].qubits: list[int] will indicate the index of the target qubit names in circuit.wire_names: list[str]. Is this correct?

Then we need to modify both qibo.transpiler and qibolab.platform to incorporate wire_names. It is better to leave _Global as it is since it works well, and instead open a new PR to update the transpiler code.

alecandido commented 1 month ago

So, in terms of qubit representation, qibo should retrieve qubits: list[str] and connectivity: list[Tuple[str, str]] from the hardware backend and return circuit.queue[i].qubits: list[int] and circuit.wire_names: list[str]. The circuit.queue[i].qubits: list[int] will indicate the index of the target qubit names in circuit.wire_names: list[str]. Is this correct?

Since all are pieces of Qibo (including the circuits, and so on), I'm not sure which part is realizing the retrieval attributed to "qibo" in your picture. Moreover:

  1. instead of "hardware backend" you should just talk about "backend", without any attribute (since, again, hardware should be no special wrt the transpilation process).
  2. the Qibolab platform is not at all part of the game, since we're only talking about the interaction between circuits (Circuit), the transpiler (Passes), and the backend (any AbstractBackend subclass) - the existence of a Platform is an internal detail of one of the backends, not concerned by the process
  3. the backend is not involved in wire_names, since that's the way Circuit expresses the address, while we already established the mechanism for backend, which is the qubits property (that can specify the physical names of the qubits)

Inference and proposal

So, I'm not sure about the various pieces of your picture, since I disagree on the various attributions, but my proposal would be the following.

Let me throw away the current situation and specific names (forget one moment about AbstractBackend, Passes, and whatever else), and let's introduce a standalone terminology.

QId = ... # Qubit identifier
NativeGate = ...

class Backend:
    @property
    def qubits(self) -> Optional[list[QId]]:
        ...

    @property
    def connectivity(self) -> Optional[list[tuple[QId, QId]]]:
        ...

    @property
    def native_gates(self) -> Optional[list[NativeGate]]:
        # in principle this could be qubit and pair dependant, since they may have
        # different available natives - but for the time being let's not deal with
        # this additional complication

class Transpiler:
    # containing the transpiler configurations
    # i.e.:
    # - which steps are involved,
    # - using which implementations,
    # - with which parameters

class Circuit:
    nqubits: int
    gates: list[Gate]
    wire_names: Optional[list[QId]]  # len(wire_names) == nqubits

def transpile(circuit: Circuit, configs: Transpiler, backend: Backend) -> Circuit:
     if circuit.wire_names is not `None`:
         if incompatible with the `Backend.qubits`:
             # (since `circuit.wire_names` is not a subset of `Backend.qubits`, or
             # routing is impossible because of the connectivity)
             raise # an informative error
         # the return value `.wire_names` should contain exactly the same elements
         # of the input `circuit.wire_names` - but with a potentially different
         # association
     else:
         # the return value should have `circuit.wire_names` set anyhow, but the content
         # will be unconstrainedly optimized on the available `backend.qubits`
csookim commented 1 month ago

There are many functions that need to be implemented in Backend. Should we keep only the essential properties and functions, like execute_circuit and execute_circuits, and move the rest to a new class SimulationBackend(Backend):? Some functions seem unnecessary for hardware backends such as QibolabBackend.

Backend
├─ SimulationBackend
│  ├─ NumpyBackend
│  │  ├─ PytorchBackend
│  │  ├─ ...
├─ QibolabBackend
├─ HWBackend1
├─ HWBackend2
alecandido commented 1 month ago

There are many functions that need to be implemented in Backend. Should we keep only the essential properties and functions, like execute_circuit and execute_circuits, and move the rest to a new class SimulationBackend(Backend):? Some functions seem unnecessary for hardware backends such as QibolabBackend.

This is the exact goal of qibo-core, so we'll do this there (and it will require a major effort).

In general, the need to implement this function is not that ubiquitous, because most of them just subclass NumpyBackend, including QibolabBackend (that would fall under SimulationBackend in your diagram).

I'd suggest you to keep subclassing the NumpyBackend for the time being (in case you need to do anything), and give up on the refactoring in the short term (since it will affect many parts of Qibo).

csookim commented 1 month ago

@alecandido There is a function _check_backend() that performs a similar role to get_backend(). Is it okay to keep it, or should we replace it with the current functions? I think, at least, we should rename it.

https://github.com/qiboteam/qibo/blob/8c02e7886af05d92bf7b0e7a13b9c371efc8b952/src/qibo/backends/__init__.py#L294-L298

alecandido commented 1 month ago

@alecandido There is a function _check_backend() that performs a similar role to get_backend(). Is it okay to keep it, or should we replace it with the current functions? I think, at least, we should rename it.

The leading _ should be removed for sure, since it's not intended to be used just within its containing module.

However, notice that is pretty different from get_backend(), as you can immediately notice from the signature: get_backend() accepts no argument, while check_backend accepts a backend argument. Essentially, the first is meant to access the global backend currently set, no matter which one, while the second is only accessing if the backend argument is not specified (it is somehow more akin to set_backend(), but without acting globally if the backend is specified).

_check_backend() is just a shortcut for the conditional backend if backend is not None else get_backend().

However, I expect this _check_backend() to be much more specific than get_backend()/set_backend(), and it should not be used by users in general (which will rely on an existing non-null backend, or on the global one), and it's just used internally in Qibo (most likely in tests). So, it may be worth to scan Qibo for _check_backend() usage, and understand why this backend argument could be None in the first place (most likely this can be addressed in a single place, without the need of repeated usage of this function, which could be inlined as the conditional).

But I'd say it's a separate effort, decoupled from this PR. If you wish, open an issue, and feel free to do the investigation and refactor in a separate PR.

alecandido commented 1 month ago

@stavros11 this PR is essentially complete, though we may refine just the last few bits. Whenever you have time, please start taking a look, as it may affect the execution in many places (possibly more than those covered by tests).

alecandido commented 1 month ago

Btw, thanks @csookim for the nice diagram (now in https://github.com/qiboteam/qibo/issues/1490#issuecomment-2413701517).

In case it could be useful to you, maybe you'd like to know that GitHub supports Mermaid, and you could realize similar diagrams just typing them in your comment:

classDiagram
    note "From Duck till Zebra"
    Animal <|-- Duck
    note for Duck "can fly\ncan swim\ncan dive\ncan help in debugging"
    Animal <|-- Fish
    Animal <|-- Zebra
    Animal : +int age
    Animal : +String gender
    Animal: +isMammal()
    Animal: +mate()
    class Duck{
        +String beakColor
        +swim()
        +quack()
    }
    class Fish{
        -int sizeInFeet
        -canEat()
    }
    class Zebra{
        +bool is_wild
        +run()
    }

with the following syntax:

```mermaid
classDiagram
    note "From Duck till Zebra"
    Animal <|-- Duck
    note for Duck "can fly\ncan swim\ncan dive\ncan help in debugging"
    Animal <|-- Fish
    Animal <|-- Zebra
    Animal : +int age
    Animal : +String gender
    Animal: +isMammal()
    Animal: +mate()
    class Duck{
        +String beakColor
        +swim()
        +quack()
    }
    class Fish{
        -int sizeInFeet
        -canEat()
    }
    class Zebra{
        +bool is_wild
        +run()
    }

(the example is taken from the Mermaid docs, https://mermaid.js.org/syntax/classDiagram.html - which also showcase how to use different types of diagrams and contain the full reference)
stavros11 commented 1 month ago

@stavros11 this PR is essentially complete, though we may refine just the last few bits. Whenever you have time, please start taking a look, as it may affect the execution in many places (possibly more than those covered by tests).

Thanks for letting me know and thanks @csookim for the implementation. Regarding affecting other places, I think qibolab and qibocal will both be affected:

Then of course we need to make sure to update version requirements accordingly.

I do not expect qibojit to be affected by this.

Other than that, I had a quick look over the changes and looks good to me. I will try to have a more detailed look, but I do not expect any major request for change from my side. As mentioned above, merging this could be considered breaking change, due to the removal of GlobalBackend, but strictly speaking Qibo does not have a well defined public interface so it is debatable whether that change is public or no.

@alecandido There is a function _check_backend() that performs a similar role to get_backend(). Is it okay to keep it, or should we replace it with the current functions? I think, at least, we should rename it.

So, it may be worth to scan Qibo for _check_backend() usage, and understand why this backend argument could be None in the first place (most likely this can be addressed in a single place, without the need of repeated usage of this function, which could be inlined as the conditional).

I had the same impression (that _check_backend() is almost duplicating get_backend()) while looking at the changes, and then found this discussion. I haven't done the scan, but I believe _check_backend() is used in many places in the quantum info module and I believe was introduced for that. I am not sure if there is an easy alternative, other than inlining the conditional which will end up being more code (in characters). I believe it is actually used in many quantum info functions which accept an Optional[Backend].

But I'd say it's a separate effort, decoupled from this PR. If you wish, open an issue, and feel free to do the investigation and refactor in a separate PR.

I certainly agree with this, given that this PR is already large and the two things are not entirely related.

alecandido commented 1 month ago
  • Qibolab: the new methods added in the abstract backend (qubits, connectivity, natives) should be implemented for the QibolabBackend. This is not exactly breaking change, [...]

This is not mentioned in this PR (nor the companion issue), but we already decided with @csookim a couple of follow-ups PRs to make the whole infrastructure working

  1. it will involve the transpiler, since the current usage in the global backend may not be compatible/suitable for our hardware needs (before there was no transpiler integration in the global, so it's not breaking)
  2. we need to match the routing with physical names with the QibolabBackend, which is currently completely ignoring Circuit.wire_names (and not accepting any other arguments to replace it)

I believe it is actually used in many quantum info functions which accept an Optional[Backend].

An alternative could be to accept a Backend, which is not optional at all (and feed them top-level with get_backend() - leaving it to the user, if needed...).

alecandido commented 1 month ago

Of course, we should fix the tests before merging. I'm checking them.

alecandido commented 1 month ago

@csookim sorry if I've not been careful before: the tests' breakage has been triggered by the commit fixing Pylint (i.e. 933f08c).

However, that commit was not really required, since the error was resulting from a Pylint false positive. You could realize just by running the relevant test locally (which I guess you also did while writing it). Unfortunately, there is a non-negligible amount of false positives in Pylint, this should be related to https://github.com/pylint-dev/pylint/issues/1498

Sometimes, locally disabling is just the best option (when you're confident that the error reported is not an error, e.g. by testing it - after understanding its meaning). For some error, even globally white-listing some components may be an option...

I will try to transition the workflows to Ruff, also in the hope to solve part of these (which may still introduce other complications, but it's very actively developed - much more than Pylint recently).

alecandido commented 1 month ago

Regarding 731b8a1 (and the triggering 5d260e3)

Function and Method Arguments

Always use self for the first argument to instance methods. Always use cls for the first argument to class methods.

https://peps.python.org/pep-0008/#function-and-method-arguments

In this case, despite being a class, it is an instance in the scope of the metaclass (because it will also be a class, being both, but wrt to the metaclass is an instance).

But I see that CPython's standard library is already inconsistent itself...

https://github.com/python/cpython/blob/37e533a39716bf7da026eda2b35073ef2eb3d1fb/Lib/enum.py#L778

which may trigger warnings or issues with the type checker (which may complain that the method overridden by the subclass had an argument by a different name, i.e. cls...).

So better to keep with cls, since - despite being conceptually wrong - it will be more familiar to the static analysis... (the problem was generated elsewhere)

csookim commented 1 month ago

1493

scarrazza commented 4 weeks ago

@BrunoLiegiBastonLiegi could you please test this branch on hardware?

alecandido commented 4 weeks ago

@BrunoLiegiBastonLiegi could you please test this branch on hardware?

@scarrazza @BrunoLiegiBastonLiegi actually, this can not work on its own, since it's changing the AbstractBackend to expose some other properties used to define the transpiler.

The NumpyBackend has been updated, effectively setting a default for all the other ones (since they almost all inherit from it). But without a tiny patch in Qibolab to expose the Platform connectivity through the QibolabBackend, it will be treated as any other simulation backend.

@csookim should be able to provide the patch for 0.1 very soon (it's just a matter of defining 3 properties to just expose properties of the Platform).

scarrazza commented 4 weeks ago

Thanks @alecandido for the information.

csookim commented 4 weeks ago

@csookim should be able to provide the patch for 0.1 very soon (it's just a matter of defining 3 properties to just expose properties of the Platform).

Ok. I will open a PR for the 3 additional properties and the use of wire_names.

csookim commented 4 weeks ago

@alecandido Could you give me permission? or do I need to fork it to make changes?

scarrazza commented 4 weeks ago

Done, please go ahead.

igres26 commented 3 weeks ago

This breaks Qibocal. We have to change ALL the instances where GlobalBackend was called in the other libraries that rely on Qibo before making this change.

alecandido commented 3 weeks ago

This breaks Qibocal. We have to change ALL the instances where GlobalBackend was called in the other libraries that rely on Qibo before making this change.

Yes, Qibocal also makes direct use of the GlobalBackend. But, as I said, this even breaks indirectly first because of Qibolab (so, you could not execute any circuit even on hardware, until https://github.com/qiboteam/qibolab/pull/1076/files).

It was ready to be merged from Qibo's point of view. But it should have been tested first...

alecandido commented 3 weeks ago

@BrunoLiegiBastonLiegi was partially doing it, and there was his review pending.

In any case, I should be able to hotfix Qibocal immediately, just replacing direct usage of GlobalBackend with the user-facing function get_backend()/set_backend(). (before both of them were available, and while the convention in Qibo and Qibo's examples were based on those functions, Qibolab and Qibocal opted for the other one...)