qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
43 stars 14 forks source link

Add support for multiple platform paths #903

Closed alecandido closed 4 months ago

alecandido commented 4 months ago

This is mainly an extension of the platform loading feature that I had in mind since long (i.e. just treat $QIBOLAB_PLATFORMS as most of the other $PATH-like variables, with multiple paths and fallback).

I started implementing this as a service for #849, to have both dummy_qrc and emulators in scope. However, I doubt it will be useful there, since I will just implement a separate fixture for that (though I could convert dummy_qrc to a more generic platforms fixture, up for voting).

In the process, I started narrowing the exports of Qibolab, since I was already touching __init__.py. More or less going in the direction of #790.

Ok, this is usually not a great idea, and I'm already kind of regretting... I hope that is not breaking anything (things not listed in __all__ are exported by accident, and should not be used), but I might split it anyhow...

alecandido commented 4 months ago

@Jacfomg, since you added them: why are these exported top-level? https://github.com/qiboteam/qibolab/blob/b93ee1b029df36a9b4df48c04ce9459764957b93/src/qibolab/__init__.py#L9-L13

We could export even more, but currently we are not doing it, so it's rather asymmetric (e.g. Pulse and PulseSequence are not exported top-level, though they are arguably as relevant as ExecutionParameters, or even more).

@andrea-pasquale removing this would break Qibocal? In any case, we could do it in 0.2, and that would not be a big deal (wrt everything else there...)

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 98.57143% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 66.90%. Comparing base (485f416) to head (4972ed9).

Files Patch % Lines
src/qibolab/platform/load.py 96.77% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #903 +/- ## ========================================== + Coverage 66.81% 66.90% +0.08% ========================================== Files 55 58 +3 Lines 5970 5983 +13 ========================================== + Hits 3989 4003 +14 + Misses 1981 1980 -1 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/903/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/qibolab/pull/903/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.90% <98.57%> (+0.08%)` | :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.

andrea-pasquale commented 4 months ago

@andrea-pasquale removing this would break Qibocal? In any case, we could do it in 0.2, and that would not be a big deal (wrt everything else there...)

Yes, it will break qibocal but it should be an easy fix.

alecandido commented 4 months ago

Yes, it will break qibocal but it should be an easy fix.

At the moment, I'm not touching it.

I will do it in 0.2

alecandido commented 4 months ago

Despite a bunch of reshuffling, this is intended to target 0.1.x (I added the milestone to make this clear).

For this reason, we should test that it's not breaking Qibocal, nor any platform working with the current Qibolab main. I would kindly ask the reviewers (@stavros11 and @andrea-pasquale) to do this, since, for you, it should be just a matter of reinstalling Qibolab with this branch in an environment you're already using for something else.

All the things I've moved around should have been considered internal, so it is formally non-breaking (but I hope it's non-breaking even in practice).

Jacfomg commented 4 months ago

@Jacfomg, since you added them: why are these exported top-level?

I don't remenber who, but it was for the qibocal imports. Yes, changing would requiere changing it the import on each routine but only that.

stavros11 commented 4 months ago

For this reason, we should test that it's not breaking Qibocal, nor any platform working with the current Qibolab main. I would kindly ask the reviewers (@stavros11 and @andrea-pasquale) to do this, since, for you, it should be just a matter of reinstalling Qibolab with this branch in an environment you're already using for something else.

For now I get an error coming from this line in qibocal: https://github.com/qiboteam/qibocal/blob/37d060599c739b3b70602505d29509af295aedb0/src/qibocal/protocols/randomized_benchmarking/utils.py#L9 This is expected given the changes in this PR, however it is clearly a qibocal issue: why is defaultdict imported from qibolab instead of collections (standard library)?

Also, unrelated with the above point, but I see that tests here are failing for windows. Maybe some path related convention?

EDIT: Apart from the defaultdict issue, qibocal seems to work with this branch, at least the single shot and allXY that I tried (but is unlikely that some other routine is broken, given the changes).

Jacfomg commented 4 months ago

For this reason, we should test that it's not breaking Qibocal, nor any platform working with the current Qibolab main. I would kindly ask the reviewers (@stavros11 and @andrea-pasquale) to do this, since, for you, it should be just a matter of reinstalling Qibolab with this branch in an environment you're already using for something else.

For now I get an error coming from this line in qibocal: https://github.com/qiboteam/qibocal/blob/37d060599c739b3b70602505d29509af295aedb0/src/qibocal/protocols/randomized_benchmarking/utils.py#L9 This is expected given the changes in this PR, however it is clearly a qibocal issue: why is defaultdict imported from qibolab instead of collections (standard library)?

Also, unrelated with the above point, but I see that tests here are failing for windows. Maybe some path related convention?

EDIT: Apart from the defaultdict issue, qibocal seems to work with this branch, at least the single shot and allXY that I tried (but is unlikely that some other routine is broken, given the changes).

Yes the import should not be like that, I will fix it.

alecandido commented 4 months ago

Ok, great!

I just have to fix a test (because I need the path to be escaped for Windows), and on my side this is ready.

@stavros11 in case you're interested in, the problem is that the path separator on Windows is the same as the escape character, i.e. \. Thus, the path has to be escaped, but only on Windows... (in any case, it is only a problem with the tests, Qibolab is working perfectly fine with both, if the path is suitably specified according to the platform)

alecandido commented 4 months ago

but if you are planning to do the rebase I don't have any objection.

I have to take care of my own mess :P I wanted to move even backends.py under platforms/ (before the backend and the platform have a very similar role, essentially the backend is just an interface for the platform), but I saved that for 0.2.

alecandido commented 4 months ago

@stavros11 @andrea-pasquale: I've just rebased, so, as soon as the tests will pass, and given the double approval, I'll directly merge.