qiboteam / qibolab

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

Rust wrapper #778

Closed alecandido closed 7 months ago

alecandido commented 7 months ago
codecov[bot] commented 7 months ago

Codecov Report

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

Comparison is base (c5bdd9d) 62.48% compared to head (0d4d034) 62.48%. Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #778 +/- ## ======================================= Coverage 62.48% 62.48% ======================================= Files 47 47 Lines 5872 5872 ======================================= Hits 3669 3669 Misses 2203 2203 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/778/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/778/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `62.48% <ø> (ø)` | | 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 7 months ago

@scarrazza @scarlehoff This I've been able to run even on MacOS, just setting the $PYTHONPATH to the environment site-packages (+ src/, because of editable mode not working out of the box - in principle it should, because of the qibolab.pth file in site-packages, not sure what is happening...)

PYTHONPATH=$(realpath qibolab/env/lib/python3.10/site-packages/):$PYTHONPATH
PYTHONPATH=$(realpath qibolab/src/):$PYTHONPATH
scarlehoff commented 7 months ago

Out of curiosity, if you don't use the virtual environment, does it work in the workflow?

alecandido commented 7 months ago

Out of curiosity, if you don't use the virtual environment, does it work in the workflow?

I guess it could, but I should use the user-wide installation.

(and right now it's not even working in the venv, for trivial reasons, but I should fix them first...)

alecandido commented 7 months ago

If someone happens to be running this in macos they will just find a cryptic error. If you are lucky it will be a failure to import qibolab but more often than not will be _cffi_backend, if you are able to catch it before that happens I think it would be a great QOL improvement for the one user using rust in a mac.

It won't be _cffi_backend, since there is no CFFI (PyO3 has its own binding mechanics).

EDIT: I understood the second part: you expected a Python script to generate the C library, like in CFFI; but there is none, because PyO3 works differently (it's closer to writing manually the bindings in C, without CFFI)

https://github.com/qiboteam/qibolab/blob/138a9696b7dc7c67511495dcd11816f2992e9c24/crate/src/lib.rs#L13-L16

I guess one of the main reasons why it's more convenient in Rust is because you don't have to distribute and install shared libraries, most of the time.

alecandido commented 7 months ago

@scarrazza: as @scarlehoff pointed, I just forgot using a virtual environment at all.

However, in this way I (accidentally) replied to @scarlehoff point above. And yes, it does work without a virtual environment.

I know that locally (on MacOS) we have the exact same problem of the C API (eventually Rust is producing code respecting the C ABI, and PyO3 is using Python's C ABI to bind - so at some level is just the same, there is no RustPython != CPython, i.e. I'm not using it).

I documented the solution patching the $PYTHONPATH in the README, and it works also for the C API (unpleasant, but at least it is a solution). Should we implement it even in the workflow? Or is it fine to strip the virtual env shenanigans from the bindings test?

scarlehoff commented 7 months ago

you expected a Python script to generate the C library...

Ah... yes. I was thinking about the pdfflow case. The point still applies in that if you can catch it before it happens so much the better.

scarrazza commented 7 months ago

Both options look good (= bad) to me, just select the one which looks simpler and clear for a potential user of these APIs.

alecandido commented 7 months ago

Ah... yes. I was thinking about the pdfflow case. The point still applies in that if you can catch it before it happens so much the better.

I could catch (in PyO3 Python errors are converted to Rust errors, i.e. the error variant of the Result enum - that is then propagated from the library), but in my experience too many user layers are making maintenance more complicated and expensive, while quickly aging.

I'd rather keep the original error, and provide the docs to explain why qibolab might be missing...

alecandido commented 7 months ago

Both options look good (= bad) to me, just select the one which looks simpler and clear for a potential user of these APIs.

Then it is fine as it is: virtual env installation is the most useful for other developers (I would not imagine any longer to have packages user wide...) but I expect a lot of people not to do it.

Even though the single person using this could use a venv... (but until we'll have users, much better to keep it lighter, at least we'd spend less time on it)

alecandido commented 7 months ago

@scarrazza: I can not merge, but I would merge as it is. Should I ask someone else for a review (i.e. I do not want to actually force @scarlehoff to make a review to this)

scarlehoff commented 7 months ago

I'm afraid my input wouldn't be worth a lot before of what I already did. I don't know much about the rest of qibolab.

alecandido commented 7 months ago

I'm afraid my input wouldn't be worth a lot before of what I already did. I don't know much about the rest of qibolab.

@scarlehoff here there was practically nothing Qibolab related, despite the repo ^^