-
Notifications
You must be signed in to change notification settings - Fork 6
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
Handling global backend for protocols involving circuits #1076
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1076 +/- ##
=======================================
Coverage 97.30% 97.31%
=======================================
Files 124 123 -1
Lines 9884 9844 -40
=======================================
- Hits 9618 9580 -38
+ Misses 266 264 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments, but in general I agree with the overall idea (since we're now focusing on the platform, assuming the backend to be Qibolab, there is no need to require everywhere to create the entire backend)
src/qibocal/auto/transpile.py
Outdated
"""Qibolab platforms.""" | ||
try: | ||
platforms = list(MetaBackend().list_available()) | ||
except RuntimeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a comment on this PR, but just a point raised as a consequence.
Maybe we should actually drop this exception: if there is no path, we should already return an empty list.
It should be sufficient to do it here:
https://github.com/qiboteam/qibolab/blob/6b2a7b790e8d58d52be937c2f41b8606459a3bf1/src/qibolab/_core/platform/load.py#L23-L26
This should result in .list_available()
returning an empty list, and .load()
raising a different error.
src/qibocal/auto/transpile.py
Outdated
qubit_maps[i] = list(qubit_map) | ||
if backend.name == "qibolab": | ||
platform_nqubits = backend.platform.nqubits | ||
if platform.name in AVAILABLE_PLATFORMS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I just forgot, but in which case is this condition supposed to be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be False when we transpile circuits that are executed on simultation so no transpilation at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be False when we transpile circuits that are executed on simultation so no transpilation at all.
Ah, I see. But in that case there should be no Platform
at all, since it is the qibolab.Platform
.
My advice would be to drop any simulation reference, and assume it is always Qibolab (possibly the emulator, but still Qibolab).
I would reintroduce the option for simulation as part of a larger refactor, taking into account #913. I'd expect there should be other as well not supporting a backend other than Qibolab.
0674c61
to
8ea8342
Compare
I realized that by properly dropping the possibility to run on simultation we don't need many shenaningans involving |
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@stavros11 I believe that with this approach we don't need anymore #1063, feel free to have a look and eventually we can just merge this PR if you agree. |
c2d5df9
to
29bc178
Compare
29bc178
to
59f95e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stavros11 I believe that with this approach we don't need anymore #1063, feel free to have a look and eventually we can just merge this PR if you agree.
Thanks @andrea-pasquale. Indeed #1063 was just a temporary solution, not intended to be merged, so feel free to close it after this is merged.
@@ -28,20 +28,18 @@ def transpile_circuits( | |||
are all string or all integers. | |||
""" | |||
transpiled_circuits = [] | |||
|
|||
qubits = list(backend.platform.qubits) | |||
platform = backend.platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle here you could add something like
from qibolab.platform import Platform
if not isinstance(platform, Platform):
transpiled_circuits = circuits
to retain the previous behavior when the backend is not qibolab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep assuming it is Qibolab, to make everything simpler. Even though in some places may be simpler to deal with other backends, it may fail in other places. We will overhaul this consistently later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the expected behavior would be to have a qibolab platform
qibocal/src/qibocal/cli/run.py
Lines 22 to 23 in 59f95e3
if platform is None: | |
raise ValueError("Qibocal requires a Qibolab platform to run.") |
tests/conftest.py
Outdated
def pytest_configure(): | ||
os.environ["QIBO_PLATFORM"] = "dummy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are avoiding the GlobalBackend
, why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some tests which are specifically looking at this env variable:
qibocal/tests/test_executor.py
Line 179 in bc7a7ba
def test_default_executor(tmp_path: Path, fake_platform: str, monkeypatch): |
I noticed it because when I wan running tests on the cluster they were trying to connect to QM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad.
I introduced that test when I implemented the scripting mechanism, because that was the context at the time. However, there is no special reason to keep it, so you could just drop the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid trigerring the error we can also decide to not rely on QIBO_PLATFORM
anymore and just use dummy
as default platform given that the behavior of QIBO_PLATFORM
is mostly undocumented.
Closes #1065, by directly instatiating a local backend in each acquisition function.
Other changes implemented in this PR:
QIBO_PLATFORM
to properly run tests on the cluster