feat!: change InstructionHandler to be a trait and require it everywhere in Rust
#484
Conversation
|
antalsz
changed the title
InstructionalHandler to be a trait and require it everywhere in RustInstructionHandler to be a trait and require it everywhere in Rust
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (5)
- quil-rs/src/program/mod.rs (568-579) The new `simplify()` method doesn't follow the same naming convention as the removed `into_simplified()` method. The old method was intentionally named with the `into_` prefix (as indicated by the `#[allow(clippy::wrong_self_convention)]` annotation), but the new method doesn't maintain this convention. This could be confusing for users migrating from the old API, especially since the method doesn't actually consume `self`.
- quil-rs/src/instruction/mod.rs (970-1283) The `memory_accesses` method in `DefaultHandler` is quite large (over 200 lines). Consider breaking it down into smaller methods grouped by instruction type (e.g., `handle_classical_instructions`, `handle_quantum_instructions`, etc.) to improve maintainability.
- quil-rs/src/instruction/mod.rs (904-909) The implementation of `is_scheduled` in `DefaultHandler` now uses `self.role(instruction) == InstructionRole::RFControl` for most cases, which is different from the previous implementation that used a large match statement. This change in logic should be carefully tested to ensure it maintains the same behavior for all instruction types.
- quil-rs/src/program/scheduling/graph.rs (216-217) The parameter order for `memory_accesses` is inconsistent with other handler methods. While `role` and `matching_frames` have the instruction as the last parameter, `memory_accesses` has it as the second parameter. Consider making the parameter order consistent across all methods.
- quil-rs/src/program/scheduling/schedule.rs (213-213) For consistency with the renamed `instruction_duration_seconds` method, consider renaming `get_waveform_duration_seconds` to just `waveform_duration_seconds` (removing the 'get_' prefix).
💡 To request another review, post a new comment with "/windsurf-review".
Previously, `ADD x 1` was considered to have a write dependency on `x` and no read dependencies; however, it needs a read dependency on `x` as well.
…e it everywhere in Rust
antalsz
force-pushed
the
instruction-handler-improvements
branch
from
519d58d to
be6c9ff
Compare
Have you verified any potential improvements with benchmarks? |
asaites
left a comment
asaites
left a comment
There was a problem hiding this comment.
Mostly, this looks great. Just one change that I see: this removed quil.program.Program.into_simplified. I recommend adding it in quilpy and regenerating the stubs.
| } | ||
| } | ||
|
|
||
| pub trait ClassicalOperand: Clone + std::fmt::Debug { |
There was a problem hiding this comment.
Why explicitly have these bounds on the trait?
There was a problem hiding this comment.
I think because they were in practice true and functionally this is a sealed trait. Plus having a Debug supertrait is always useful for debugging.
| /// If you need custom instruction handling during simplification, | ||
| /// use [`InstructionHandler::simplify_program`] instead. | ||
| #[allow(clippy::wrong_self_convention)] // It's a Breaking Change to change it now. | ||
| pub fn into_simplified(&self) -> Result<Self> { |
There was a problem hiding this comment.
We likely want to have a version of this method still in quil, though I'm indifferent about whether it should have a name change. I would lean towards calling the Python version something like to_simplified instead, but keeping it into_simplified prevents breaking quil. Regardless, we just want program/quilpy.rs to add this to the impl Program block:
#[pyo3(name = "into_simplified")]
fn py_simplify(&self) -> Result<Program> {
self.simplify(&DefaultHandler)
}And likely it should copy over the documentation as well.
There was a problem hiding this comment.
And of course, the pyo3(name = ...) isn't strictly necessary, if you prefer setting the function name directly.
There was a problem hiding this comment.
Done – I also marked it as #[deprecated] so that we can't accidentally use it in Rust. Let me know what you think of this approach!
There was a problem hiding this comment.
I'm not convinced the python_only module or #[deprecated] annotations are needed:
- Essentially, everything in
quilpymodules exist only for the benefit of Python bindings, and I see little reason to mark it asdeprecated. - The module itself is only available if you're building with the
pythonfeature enabled, so misuse seems unlikely. - Moreover, the
py_*naming convention is itself a pretty good hint that the method exists as a wrapper. - It's possible marking it
#[deprecated]will have consequences for how it's handled by PyO3 (or other libraries) in the future.
There was a problem hiding this comment.
Yeah I think you're right, I just got a bit overzealous. We could see about setting up a Clippy lint to forbid calls to methods named py_ but that's probably not necessary. I'll make the change!
Co-authored-by: Alexander Saites <[email protected]>
This PR removes the
InstructionHandlertype that is defined in terms of boxed closures, and replaces it with anInstructionHandlertrait. This has the potential to provide signficant performance improvements (and I think it's probably cleaner regardless).This PR also removes every Rust method that could use
InstructionHandlerbut doesn't, requiring a deliberate use of an instruction handler. However, because we don't expose instruction handlers to Python, this is not true for our Python bindings. Ideally, this should not be a change for Python users, but I would appreciate close review of this part.