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

Rust wrapper #778

Merged
merged 16 commits into from
Jan 25, 2024
Merged

Rust wrapper #778

merged 16 commits into from
Jan 25, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Jan 22, 2024

  • test on Linux
  • add testing workflow

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@           Coverage Diff           @@
##             main     #778   +/-   ##
=======================================
  Coverage   62.48%   62.48%           
=======================================
  Files          47       47           
  Lines        5872     5872           
=======================================
  Hits         3669     3669           
  Misses       2203     2203           
Flag Coverage Δ
unittests 62.48% <ø> (ø)

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 alecandido marked this pull request as ready for review January 22, 2024 18:23
@alecandido alecandido requested a review from scarrazza January 22, 2024 18:24
@alecandido
Copy link
Member Author

alecandido commented Jan 22, 2024

@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
Copy link

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

@alecandido
Copy link
Member Author

alecandido commented Jan 22, 2024

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

Base automatically changed from execute_from_capi to main January 23, 2024 08:28
The missing realpath command is replaced by a Python script

Since the package is not installed in editable mode in the workflow, there is no need for the explicit src/ path
Copy link

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

I'd recommend to add a try-catch block first time the wrapper tries to do something with the message that you put in the readme.

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.

from pathlib import Path;
print(Path("../.venv/lib/python3.11/site-packages/").resolve())
')
echo "PYTHONPATH=$site:$PYTHONPATH" >> "$GITHUB_ENV"

Choose a reason for hiding this comment

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

Couldn't the python wrapper itself update the sys.path at runtime when running inside a virtual environment?

This would only work if cffi is installed in a way in which it can be accessed by the wrapper (otherwise the wrapper won't start I guess?... not sure... maybe if it is the first thing you do...)

Something like

import sys, os, subprocess
if "VIRTUAL_ENV" in os.environ:
    sys.path.append( subprocess.run("/path/to/env/python3 -c etc").split() )

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, there is no Python wrapper here, it's everything in Rust (calling Qibolab eventually, but it has to find it first).

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, I put the wrong path in the variable, and it worked. So it seems is not required on GitHub MacOS runner.

@alecandido
Copy link
Member Author

alecandido commented Jan 23, 2024

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)

qibolab/crate/src/lib.rs

Lines 13 to 16 in 138a969

let qibolab = PyModule::import(py, "qibolab")?;
let pyarray: &PyArray2<i32> = qibolab
.getattr("execute_qasm")?
.call((), Some(kwargs))?

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member

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

@alecandido
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@scarrazza scarrazza merged commit 3063105 into main Jan 25, 2024
24 checks passed
@scarlehoff
Copy link

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
Copy link
Member Author

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 ^^

@alecandido alecandido added this to the Qibolab 0.1.5 milestone Jan 25, 2024
@alecandido alecandido deleted the pyo3 branch February 28, 2024 14:54
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.

3 participants