Internal global backend _Global

[csookim]

This covers issue #1309.

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

Todo:

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

[csookim]

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

qibo/src/qibo/backends/__init__.py

Lines 142 to 147 in 2419d7d

	def resolve_global(cls):
	if cls._backend is None:
	cls._backend = cls.backend()
	if cls._transpiler is None:
	# TODO: add default transpiler for hardware backends
	cls._transpiler = cls.transpiler()

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.

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?

qibo/src/qibo/models/error_mitigation.py

Lines 1099 to 1103 in 2419d7d

	elif backend.name == "qibolab": # pragma: no cover
	# TODO: Use _Global.set_transpiler
	backend.transpiler.passes[1] = Custom(
	initial_map=qubit_map, connectivity=backend.platform.topology
	)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1440   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          81       81           
  Lines       11795    11859   +64     
=======================================
+ Hits        11787    11851   +64     
  Misses          8        8           

Flag	Coverage Δ
unittests	99.93% <100.00%> (+<0.01%)	⬆️

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.

[alecandido]

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]

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]

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]

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.

[alecandido]

@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
pylint-dev/pylint#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]

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]

#1493

[scarrazza]

@BrunoLiegiBastonLiegi could you please test this branch on hardware?

[alecandido]

@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]

Thanks @alecandido for the information.

[csookim]

@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]

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

[scarrazza]

Done, please go ahead.

[igres26]

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]

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]

@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...)