-
Notifications
You must be signed in to change notification settings - Fork 342
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
feat: Bootstrap experimental, toggleable high-performance backend for Quil programs. #1755
base: master
Are you sure you want to change the base?
Conversation
#[setter] | ||
fn set_offsets(self_: PyRefMut<'_, Self>, offsets: Bound<'_, PyAny>) -> PyResult<()> { | ||
let mut instruction = self_.into_super(); | ||
let offsets = conversion::py_to_offsets(&offsets)?; |
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.
We have to manually run the conversion inside #[setter] methods because using them with the #[from_py_with="..."]
parameter attribute causes a compilation error. Opened PyO3/pyo3#3992 to track.
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.
Looking really good! Could use some cleaning up, but the foundations look good.
|
||
[tool.poetry.dev-dependencies] | ||
black = "^22.8.0" | ||
flake8 = "^3.8.1" | ||
pytest = "^7.4.0" | ||
pytest-cov = "^4.1.0" | ||
mypy = "^1.5.0" | ||
ruff = "^0.3.2" |
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.
❤️
Side note, we can replace black
and flake8
with ruff
, as it contains the functionality of both.
// RsInstruction(quil_rs::instruction::Instruction), | ||
Serialized(String), | ||
Program(Program), | ||
// RsProgram(quil_rs::Program), | ||
// Sequence(Vec<InstructionDesignator>), | ||
// Tuple | ||
// Generator |
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.
Delete comments?
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.
Also below
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.
Will make sure to add and/or delete any doc comments that need addressing. These ones need proper implementing to maintain compatibility with the current Program API though.
Co-authored-by: Michael Bryant <[email protected]>
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.
Looking great. Thanks for the killer MR description, that made review much faster and clearer.
To be transparent, I am hesitant about the approach for these reasons:
- in our migration to Rust and pyQuil v4, we intended to leave pyquil "as-is" as much as possible. This is a clear departure from that and appears to be an unintended consequence of the move to Rust. That said: it should still serve the purposes of its users, so long as all of the helper functions and python internals that they rely on today are upheld by the new Rust-based classes.
- it's a lot of work to keep the API and functionality roughly the same
That said, the current performance limitations (within the python-side instruction processor) are not things we can leave unfixed, and this is a principled way of fixing them.
my only recommendation would be to expand the tests to cover all the pythonic ways that users might use or query instructions - all the dunder methods, etc. I'd also want to validate this version through our power users' notebooks before merge. I know that some are covered today, but from the conversation last week it sounds like others may be missing.
Also:
- The benchmark you have in the MR description is great - thanks for making that result & program available and obvious - can you add the other function from the internal report,
copy_everything_except_instructions
, for a similar program as reported? - needs a linked and prefixed issue. Every MR needs one.
I think we share the same concerns. I would add that even though it is a departure from keeping pyQuil "as-is", in hindsight, the compatibility layer is a thin illusion that makes it seem like we kept things the same more than we actually did.
Agreed, I think test_quilbase.py is the right approach, but I need to add test cases for the default dunder methods Python provides that I took for granted. Fortunately, the proposed backend will make this much easier, since we can actually implement common functionality on the Instruction base class, which isn't something we can do with quil-py today.
Same, and I think I can make this easy in a low risk way. I want to introduce a configuration option (e.g. a
The one downside is that we have to maintain two backends, but changes are relatively infrequent, and I think that is the right tradeoff to make for the benefits above.
Will do!
Right, created #1760 |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
tl;dr: Make pyQuil a hybrid Rust package that wraps quil-rs directly, get better performance and code quality.
I'll start with some leading questions to inform the review:
Description
When we built pyQuil v4, we started by building
quil
, a Python package with Rust bindings toquil-rs
. However,quil-rs
was re-built from first principles, with all the lessons of pyQuil, to serve solely as a reference to the Quil spec. Its approach and API is different, and in order to not break pyQuil, we added a compatibility layer as to not rock the boat.This compatibility layer is implemented in Python and has a negligible performance impact when dealing with a small amount of instructions, however, it scales poorly as instruction count grows. Recently, we investigated the performance of iterating over a
Program's
instructions
property, and saw less than ideal performance as instructions got into the thousands (see #1754).Currently, the path from quil-rs, to pyQuil's public facing API looks something like this:
The compatibility layer, being in Python, is slow. Not only that, but
quil
is really just a middle-man between quil-rs and pyQuil. The changes in this PR illustrate a pattern that would look something like this:By porting all of pyQuil's core Quil logic into Rust and wrapping quil-rs directly, we eliminate the need for
quil
entirely, simplifying the dependency chain. In addition, the transformation logic is all performed in the more performant Rust. This improves performance (receipts below), and I think, improves the quality of the code. Due to not wanting to sacrifice the quality of quil-py, we made certain compromises in pyQuil. For example, we use a custom metaclass to make all quil-rs types appear as AbstractInstruction types. This mostly works, but has some odd corner cases if you start to mix quil-rs types with pyQuil types. That problem stems from the fact that thequil
Instruction class can't be used as a subclass because it's an enum (a limitation with pyo3/C ABI), which also means we have lots of duplicated code for every instruction to implement things like__copy__
or pickling. Support for other things like pickling are inconsistent because of this. Moving the compatibility layer into Rust solves all of these problems.Performance improvements
For each backend, parse a program from a string containing 9001 lines, then iterate over every instruction 1000 times. Benchmark with hyperfine.
Data collected on a 2021 Macbook Pro M1 Max
Benchmark script
Tracking issue #1760