feat!: make wrap_in_loop more efficient and correct#504
Conversation
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
|
| start_target: Target, | ||
| end_target: Target, | ||
| iterations: u32, | ||
| ) -> Self { |
There was a problem hiding this comment.
For backwards compatibility, we can keep the end_target parameter and simply ignore it, or better yet, report a deprecation warning if it's passed.
I'm already in the process of adding some of these for other cases where I'd prefer to point PyQuil at the quil binding directly while maintaining some backwards compatibility, so I am planning to add a macro to make that easier. For now, you can just use PyErr:warn directly.
Because iterations comes after end_target, it requires a little trickery to support both at once:
#[cfg_attr(feature="python", derive(pyo3::FromPyObject))]
enum EndTargetParam {
#[allow(dead_code, reason="the field isn't read, but we still want it type-checked")]
Target(Target),
Iterations(u32),
}Then you can create a new Python wrapper around the original Rust method:
#[pymethods]
impl Program {
#[pyo3(
name = "wrap_in_loop",
signature = (
loop_count_reference,
start_target,
end_target=None,
iterations=None,
))]
fn py_wrap_in_loop(
&self,
py: Python<'_>,
loop_count_reference: MemoryReference,
start_target: Target,
end_target: Option<EndTargetParam>,
iterations: Option<u32>,
) -> PyResult<Self> {
let iterations = match (end_target, iterations) {
(Some(EndTargetParam::Iterations(iterations)), None)
| (None, Some(iterations)) => iterations,
(Some(EndTargetParam::Target(_)) | None, Some(iterations)) => {
PyErr::warn(py, &PyDeprecationWarning::type_object(py),
c"`end_target` is deprecated and will be ignored", 1)?;
iterations
},
(Some(EndTargetParam::Target(_)) | None, None) => {
return Err(PyTypeError::new_err("wrap_in_loop missing required positional argument: 'iterations'"));
},
(Some(EndTargetParam::Iterations(_)), Some(_)) => {
return Err(PyTypeError::new_err("wrap_in_loop got multiple values for argument: 'iterations'"));
},
};
Ok(self.wrap_in_loop(loop_count_reference, start_target, iterations))
}
}You could get more careful with those cases/warnings/errors, but the main thing you want (to prevent a breaking change) is to ensure that you can accept those arguments both by position and keyword. If you really wanted to be more careful, you can accept (*args, **kwargs), but then the type-checker is less useful (I think in pure python, we'd write the stubs with @overloads and mark some of them @deprecated, but I'm not actually certain if mypy or pyright handle that).
We can (later) simplify this once we're satisfied the deprecated version has been released in at least one version (with a breaking change, of course).
There was a problem hiding this comment.
Oh this is very very clever! It's also fairly heavy-duty, though – what's the benefit vs. just putting the workaround code on the Python side, where it's maybe (?) easier to write?
There was a problem hiding this comment.
Good question, and it's one I've gone back and forth on a bit. My current stance is we benefit from having more on the Rust side when we can. Doing it here is easier to make static guarantees (in this particular example, it's really nice having the compiler check that the match is exhaustive; mypy/pyright can sometimes handle that, but not always). It also means our tooling can alert on API breaking changes that aren't flagged as such, which is not something we currently have built in to our PyQuil CI, as far as I'm aware.
The times I think it's more sensible to go in PyQuil are, as you rightly point out, when the logic is easier to express in pure-Python. There are also some cases where a class/type should go away completely in favor of some alternative, and so it's more sensible to leave that out of the Rust code, and just have the Python version report the warning while wrapping the alternative.
But the main reason I'm suggesting something like this is that, at present, we're working on changes to PyQuil to make more direct use of quil/quil-rs's bindings. The goal is to remove large parts of PyQuil's codebase that currently simply wraps existing quil classes without adding any particular benefits. Although that will involve some breaking changes, we'd like to retain backwards compatibility where reasonable.
This specific case is a bit of an outlier in that you want to remove an argument in the middle of the argument list, whereas most of the changes that I've been looking at are simply differences in the parameter names, and often they can be done with minimal effort.
I'll think on this for a moment and see what the Python version looks like, and where it would slot into PyQuil, if it were to go there. I agree that this instance requires a fair bit of code just to handle the deprecation warning, and realistically, we already state that quil is intentionally moving quickly/likely to have breaking changes, so I think that's an argument in favor of putting it at the PyQuil layer.
There was a problem hiding this comment.
Also, it occurs to me that likely the "right" way to handle the stubs when we do something like this is to just override the signature with the "correct" one. In particular, without intervention, the change I propose would generate a signature that indicates iterations is optional/that None is a valid value, but really we only want to use that as a sentinel, so it'd be better to have the signature that got generated with the stubs in your original version. If you make the changes I suggest, let me know if you need help with the stubs part.
There was a problem hiding this comment.
Alright, I think I got this all set up. Thanks for the pointers!
| if iterations == 0 { | ||
| return self.clone(); | ||
| match iterations { | ||
| 0 => return self.clone_without_body_instructions(), |
There was a problem hiding this comment.
Is this really equivalent to an empty program (as documented)?
In either case, in order to avoid a breaking change, this behavior needs to stay the same.
There was a problem hiding this comment.
It should be! clone_without_body_instructions keeps the calibrations etc. but has no code, which is the same as running a loop zero times. Since this is a bug fix, I'd like to include it in the Rust release, where we're making a breaking change anyway; can we get away with a technique like in your other answer to either (a) preserve the behavior on 0 but raise a warning, (b) change the behavior on 0 but raise a warning, or (c) throw an error on 0? It's my suspicion that no code is out there that's using 0 because the behavior is wrong.
There was a problem hiding this comment.
Ah, I see your point. We are already of the position that we don't consider it a breaking change if the old behavior was not correct in the first place.
As for your suggestions about alternatives, I think I'd rank those options as (c), (a), (b), where the value of (c) is only if we want to prevent mistakes. I don't think (a) is all that valuable unless we plan to change the behavior later, but I think (b) is probably just confusing.
I will concede that "run this 0 times" should mean the body never executes, but now I'm questioning a bit whether removing the instructions is technically the right thing to do. Certainly I'll agree I'd expect the resulting Quil programs to be functionally equivalent, but arguably using 0 here should result in a program that looks something like:
JUMP @skip-body
# original program instructions...
LABEL @skip-body
I don't expect that using this to generate Quil would be useful to anyone, but I might could imagine why someone would expect the Program itself to keep those instructions around, namely for later manipulation and programmatic analysis. In addition, even though I'd expect a functional equivalency here, are there aspects of the machine implementations that might make a meaningful observable difference if the generated Quil does/doesn't have "useless" instructions?
This feels a bit purist, though, so practically speaking, I'm not opposed to this change, but I'd like your feedback. At the very least, I recommend some additional documentation; instead of just saying it returns an empty Program, it should say what does/doesn't get retained (likely just by gesturing towards that method call anyway).
There was a problem hiding this comment.
In practice, the routines like this I've seen for generating fixed-iteration loops want to generate empty programs for 0-iteration loops the same way that 1-iteration loops become
...
BODY
...
instead of
LABEL @start
...
BODY
...
or worse
DECLARE i INTEGER[1]
MOVE i[0] 1
LABEL @start
...
BODY
...
SUB i[0] 1
JUMP-WHEN @start i[0]
That said, you're 100% right that this needs to be clearer, so I updated the docs to hopefully explain this better!
| start_target: Target, | ||
| end_target: Target, | ||
| iterations: u32, | ||
| ) -> Self { |
There was a problem hiding this comment.
Also, it occurs to me that likely the "right" way to handle the stubs when we do something like this is to just override the signature with the "correct" one. In particular, without intervention, the change I propose would generate a signature that indicates iterations is optional/that None is a valid value, but really we only want to use that as a sentinel, so it'd be better to have the signature that got generated with the stubs in your original version. If you make the changes I suggest, let me know if you need help with the stubs part.
|
Addressing the The solution to this, I think, is to simply allow duplicate versions of winnow? It's a parser library that seems to be used at two different versions by The other two errors, however, are RUSTSEC advisories for PyO3 < 0.29. They're not exploitable by our code, AFAICT, but we certainly can't trivially upgrade to 0.29. How do you want to handle them? |
2 participants
This PR changes the behavior of
Program::wrap_in_loopto generate only one jump at the end of the loop. This is an improvement both because it's less code and because we avoid generating an empty do-nothing basic block.Concretely,
wrap_in_loopcurrently generates code like this:The
JUMP-UNLESSandJUMPtogether say "if iterations ≠ 0, then continue after the loop; otherwise, go back to the start". But we can express this even more simply as "if iterations == 0, go back to the start; otherwise, continue to the next instruction", which is a singleJUMP-WHEN.The only catch with this change is that it's a breaking change for the Python API. I think the right fix there is to, in PyQuil, take the loop end label and ignore it, possibly with a warning, but I don't know how to coordinate the rollout.
Additionally, the old behavior of setting
iterations = 0was to run the code in the loop body once, which is incorrect; I changed the behavior to causeiterations = 0to run no code. I think this is unlikely to cause any actual changes to existing code sinceiterations = 0is not useful and had the wrong behavior before.