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

Remove folder argument from platform create #784

Merged
merged 13 commits into from
Mar 20, 2024
Merged

Remove folder argument from platform create #784

merged 13 commits into from
Mar 20, 2024

Conversation

stavros11
Copy link
Member

@stavros11 stavros11 commented Jan 26, 2024

Following the discussion in qiboteam/qibolab_platforms_qrc#108 (comment).

I believe some lines in qibo (and maybe also qibocal) need to be updated.

Checklist:

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

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stavros11, indeed it is better update the documentation accordingly given that we dropped the folder argument in https://github.com/qiboteam/qibolab_platforms_qrc

@stavros11
Copy link
Member Author

Thanks, the only issue is that doctest requires qibo to be updated, because qibo.set_backend is currently using the removed path argument.

@andrea-pasquale
Copy link
Contributor

Thanks, the only issue is that doctest requires qibo to be updated, because qibo.set_backend is currently using the removed path argument.

Ahhh, too many repositories.

@andrea-pasquale
Copy link
Contributor

Btw, shall we release qibolab now that the milestone is complete? @scarrazza @stavros11 @alecandido
I know that maybe some changes were not tested on hardware but I will be able to merge some PRs in qibocal as soon as we release qibolab.

@alecandido
Copy link
Member

Btw, shall we release qibolab now that the milestone is complete? @scarrazza @stavros11 @alecandido I know that maybe some changes were not tested on hardware but I will be able to merge some PRs in qibocal as soon as we release qibolab.

Fine by me :)

Copy link
Member

@alecandido alecandido left a 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 suggestions. Other than that, is perfectly fine.

@@ -611,8 +628,10 @@ in this case ``"twpa_pump"``.
from qibolab.instruments.dummy import DummyInstrument
from qibolab.instruments.oscillator import LocalOscillator

FOLDER = Path.cwd()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FOLDER = Path.cwd()
FOLDER = Path(__file__).absolute().parent

Since you never know in which directory is the person launching it.

Copy link
Member Author

@stavros11 stavros11 Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't like this, because it's not what the user is supposed to use, but Path(__file__) was causing doctest to fail, I guess because it's not really a file.

https://github.com/qiboteam/qibolab/actions/runs/7669260031/job/20902792936

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right after writing this, I realized that could be the reason...

I thought a while about that, but the only two proposals I have are:

  • avoid testing or
  • leave it as it is, but add a comment explaining

It's not great to see a comment for internal testing reasons, but you should do in this other way. But I would leave a simpler comment anyhow

Suggested change
FOLDER = Path.cwd()
FOLDER = Path.cwd() # Path(__file__).absolute().parent

at least the reader has a chance of understanding the difference.

@scarrazza
Copy link
Member

@andrea-pasquale fine by me too.

@stavros11 stavros11 added the qibo-release Requires newer qibo release label Jan 26, 2024
@scarrazza scarrazza added this to the Qibolab 0.1.6 milestone Feb 15, 2024
@stavros11
Copy link
Member Author

@andrea-pasquale @alecandido I updated qibo to 0.2.6. As soon as CI passes, I will merge this.

@alecandido
Copy link
Member

@andrea-pasquale @alecandido I updated qibo to 0.2.6. As soon as CI passes, I will merge this.

 = 46 failed, 2198 passed, 104 skipped, 47 deselected, 3 xfailed, 3 xpassed, 5 warnings in 18.20s =

bad luck...

@andrea-pasquale andrea-pasquale removed the qibo-release Requires newer qibo release label Mar 20, 2024
@stavros11
Copy link
Member Author

stavros11 commented Mar 20, 2024

@alecandido in 2603611 I dropped the default transpiler from QibolabBackend. This is a significant change because it will no longer be possible to run circuits that contain non-native gates out-of-the box, including through the openqasm, C and Rust APIs (btw I also pushed a fix for these tests but for some reason github doesn't show it).

The reason for dropping is that otherwise we are testing the transpiler in qibolab tests while the code is in qibo (which leads to the failures above). It is still possible to add a transpiler of choice manually:

backend = QibolabBackend("my_platform")
backend.transpiler = ... # my transpiler imported from qibo

or even better the user transpiles their circuit manually:

backend = QibolabBackend("my_platform")

circuit = Circuit(...)
...

native_circuit = my_transpiler(circuit)

result = backend.execute_circuit(native_circuit)

@stavros11
Copy link
Member Author

After https://github.com/qiboteam/qibolab/actions/runs/8360122834/job/22885054536?pr=784 I think I should officially give up

Cannot install setuptools.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

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

Project coverage is 66.57%. Comparing base (535dcdb) to head (21369b5).

Files Patch % Lines
src/qibolab/backends.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #784   +/-   ##
=======================================
  Coverage   66.56%   66.57%           
=======================================
  Files          50       50           
  Lines        6063     6061    -2     
=======================================
- Hits         4036     4035    -1     
+ Misses       2027     2026    -1     
Flag Coverage Δ
unittests 66.57% <90.90%> (+<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
Copy link
Member

Yes, please, just merge.

All in all, Qibolab is the low-level layer, it is fine to require circuits to be transpiled by someone else (i.e. Qibo).

@stavros11 stavros11 merged commit 503c3b5 into main Mar 20, 2024
24 checks passed
@stavros11 stavros11 deleted the platform_folder branch March 20, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants