-
Notifications
You must be signed in to change notification settings - Fork 991
pyosys: rewrite using pybind11 #5370
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
base: main
Are you sure you want to change the base?
Conversation
d0dfbcc
to
9db2f79
Compare
Just as a data point, some years ago nextpnr moved from boost::python to pybind11 and it was a very good decision, the number of weird Python-related build issues we had dropped significantly. |
53ef903
to
0786b90
Compare
@donn During the Dev JF where you proposed this, I brought up the inherent issue with the object lifetime of The idea would roughly be to add a (I still think what I suggested during the call, eventually changing the API to track module item references by name instead of by reference would be a viable strategy, but I think it makes sense to consider both options and pick whatever what would make maintaining the bindings easier) |
@jix Thanks for writing it down, will investigate before I mark this PR ready 🫡 |
e5a88f1
to
dd5fd2e
Compare
Note that I don't want to make fixing all currently present lifetime issues a requirement for merging a first pybind11 based version. It's the other way around in that I don't think we could realistically address the lifetime issues with the current python bindings setup, so I see this more as an opportunity for follow up improvements this would enable. |
f513284
to
4615169
Compare
@jix So I looked into it - noting it down for myself in the future but your implementation is basically this code block here: Line 2847 in 7ebd972
I think if that's not a blocker then yeah maybe I can tackle Wire/Cell lifecycles in a separate PR. |
f9f3186
to
4c32379
Compare
I tested it with my projects, librelane and difetto. Both make heavy usage of pyosys and yosys plugins, and everything's working! Some notes:
@georgerennie Would also appreciate testing from your side. |
4c32379
to
86b98cc
Compare
@donn how would you feel about writing some documentation for using pyosys? If you're not familiar with the rst format that we use, markdown would also be fine and then I can convert it. At the moment it looks like the only thing we have is the |
@KrystalDelusion I can try putting something under "Using Yosys (advanced)." |
That would be great, thanks! |
Thanks for your hard work, tested locally and that works fine. |
That's correct. Right now it just does what bufnorm needs, but ideally we would end up with a single mechanism that allows different parts of yosys to "retain" and "release" named objects within a module. That can then be shared by pyosys and bufnorm. Let me know when you want to work on this, so we can discuss the requirements for a more general way to do this and how to best implement that. |
@KrystalDelusion Done, I wrote a small guide. Didn't go too in-depth because it kind of requires knowledge of Yosys internals and that overlaps with other parts of the documentation. |
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.
Thanks for the documentation! I have left a couple minor comments on some of the wording, but otherwise it looks great.
docs/source/using_yosys/pyosys.rst
Outdated
Getting Pyosys | ||
-------------- | ||
|
||
Pyosys supports Python 3.8.1 or higher. You can access Pyosys using one of two |
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.
Interesting. I think officially we require at least 3.11, but I'm not sure how much of that is for Yosys and how much is for the frontends like SBY.
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.
Tbh it might be less of a headache to drop 3.8 and 3.9 especially, the problem is people on like Debian Bullseye (Python 3.9/not EOL) and Ubuntu 20.04 (Python 3.8/EOL) start complaining...
pybind11 3.0.0 officially supports CPython 3.8+, so it seems prudent to just match its compatibility unless we specifically need functionality that's not 3.8-compatible
- Rewrite all Python features to use the pybind11 library instead of boost::python. Unlike boost::python, pybind11 is a header-only library that is just included by Pyosys code, saving a lot of compile time on wheels. - Factor out as much "translation" code from the generator into proper C++ files - Fix running the embedded interpreter not supporting "from pyosys import libyosys as ys" like wheels - Move Python-related elements to `pyosys` directory at the root of the repo - Slight shift in bridging semantics: - Containers are declared as "opaque types" and are passed by reference to Python - many methods have been implemented to make them feel right at home without the overhead/ambiguity of copying to Python and then copying back after mutation - Monitor/Pass use "trampoline" pattern to support virual methods overridable in Python: virtual methods no longer require `py_` prefix - Create really short test set for pyosys that just exercises basic functionality
[skip ci]
There is so much templating going on that compiling wrappers.cc now takes 1m1.668s on an Apple M4…
For consistency. Also trying a new thing: only rebuilding objects that use the pybind11 library. The idea is these are the only objects that include the Python/pybind headers and thus the only ones that depend on the Python ABI in any capacity, so other objects can be reused across wheel builds. This has the potential to cut down build times.
Primarily address feedback from @KrystalDelusion (thanks!)
What are the reasons/motivation for this change?
pybind11 is a reasonably more lightweight library that is subjectively a bit more intuitive to use. But more importantly, it does not require an external library (boost) to be installed on the machine, and does not require boost to be compiled for every version of Python.
Explain how this is achieved.
saving a lot of compile time on wheels[EDIT: as it turns out not really, I used enough C++ templates to offset the time gain from not compiling boost]pyosys
directory at the root of the repoPyosys API Breaks
from pyosys import libyosys as ys
like wheelspy_
prefixPass.call__YOSYS_NAMESPACE_RTLIL_Design__std_vector_string_
is justPass.call
(pybind11 handles overload resolution)ys.Yosys
has been renamedys.Globals
to more adequately communicate its purposeIf applicable, please suggest to reviewers how they can test the change.
pip3 install -v . python3 ./tests/pyosys/run_tests.py
Resolves #4591
TODO:
and addressmemory concerns raised by @jix