Refactor BackendEstimator
#9074
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
pedrorrivero
requested review from
a team,
ikkoham and
t-imamichi
as code owners
|
Once this is okay to go, I will substitute All tests from |
| """Backend to use for circuit measurements.""" | ||
| return self._backend | ||
|
|
||
| @backend.setter |
There was a problem hiding this comment.
Do we need setter for backend?
There was a problem hiding this comment.
Technically no, you are right! But now that all the logic is decoupled we can provide this functionality. If you think it is better to disallow this I am happy to remove it, let me know.
On top of that, I use the setter to (pseudo-)normalize attributes as explained in the previous thread 🙂
There was a problem hiding this comment.
It is best not to make anything that does not have a use case. It makes maintenance difficult and may cause bugs.
There was a problem hiding this comment.
Could you remove this method?
|
I agree with Hamamura-san's comment #9074 (comment) about performance. Since this PR rewrote |
There was a problem hiding this comment.
This isn't a proper review of the code (which I'm not equipped to do properly), more of a flick through and some comments.
A major thing: why is this being done? It's not clear to me why this PR should be accepted at all; the previous code was working, and there didn't appear (to me, who admittedly wasn't following closely) to be any structural issues causing problems for others, or bugs that couldn't be fixed due to poor structure of the code. If anything, this PR seems to add a lot of complexity to a class that's simply meant as a transitional object to get people running on the new APIs as fast as possible, while they're waiting for their providers to catch up properly.
Refactors aren't a priori a good thing - they offer far more chance for bugs to creep in, and they typically erase the institutional knowledge that was gained over the creation of the previous implementation. This doesn't appear to be fulfilling a new requirement, doesn't appear to be fixing bugs, and doesn't appear to simplify, so it's not clear to me why it should be merged.
At the very least, please can you update the top comment of the PR to explain exactly why this is being done for future knowledge? I'll then back off this PR - it's not my domain, and I'll leave any decision on approval / whether it's suitable to merge to @ikkoham and @t-imamichi as code owner.
| ################################################################################ | ||
| ## IMPLEMENTATION | ||
| ################################################################################ | ||
| # TODO: caching |
There was a problem hiding this comment.
These large feature-request todos are likely to stay that way for ever unless it becomes separately clear that there's a need for them. It's probably best not to put them in at all.
There was a problem hiding this comment.
Good point, thanks! I plan to go over all the todos before merging, I'll make sure to remove as many as possible 🙂
| # Pre-processing | ||
| circuit_bundles = self._preprocess(circuits, observables, parameter_values) | ||
| metadata_bundles = tuple(tuple(c.metadata for c in bun) for bun in circuit_bundles) | ||
| job_circuits = list(c for bun in circuit_bundles for c in bun) # TODO: accept tuple |
There was a problem hiding this comment.
this todo is simple; better to either do it (though I'm not sure why) or leave the comment.
There was a problem hiding this comment.
Oh, but it is not that simple because it is Backend.run() what does not accept Sequences other than list. Fixing this here is out of scope, hence the TODO (again, I'll open issues before merging and remove this from here).
| """Test observable decomposer property.""" | ||
| estimator = BackendEstimator(Mock(Backend)) | ||
| self.assertTrue(estimator.abelian_grouping) | ||
| self.assertIsInstance(estimator._observable_decomposer, AbelianDecomposer) |
There was a problem hiding this comment.
Testing private components like this is generally not a good idea. They aren't part of the public interface, and writing hyper-explicit tests like this means that the structure of the underlying code cannot change without the tests also changing, which very easily masks bugs. There's a good amount of confidence given by a PR that modifies code, and all the tests easily pass, and it's very concerning when a PR needs to modify the tests to get them to pass, unless the test is clearly buggy.
There was a problem hiding this comment.
Interesting 🤔 this is my way of making sure that the behavior for abelian_grouping stays consistent. True, it is a private property, but merely a reflection of a user configurable attribute.
I understand what you are saying and think that it may apply to other places, but I am not so sure here. If we change a what abelian_grouping means, shouldn't the tests fail? How would yo handle this otherwise?
There was a problem hiding this comment.
Presumably the user setting abelian_grouping=True should cause some changes to the output they see from public methods? I'd test that, rather than deep specifics of the internal implementation.
Let's say you accept mine and Takashi's arguments above about the ObservableDecomposer class and remove it. You could keep all the public behaviour exactly the same while inlining NaiveDecomposer and AbelianDecomposer, but all your tests will suddenly fail, and (I could well be wrong on this) I'm not certain there are any end-to-end tests that would fail if the abelian_grouping option were suddenly to disappear. In that case, are these tests helping us catch bugs, or are they just making busy work for anybody changing the code? Users don't care about how the internals are implemented, and user-facing behaviour is what we care about not breaking, primarily.
There was a problem hiding this comment.
Presumably the user setting abelian_grouping=True should cause some changes to the output they see from public methods?
Negative... it only changes the way things are done internally. This is inherited from the original code.
There was a problem hiding this comment.
Since transpiled_circuits are exposed, it can be tested from the length of transpiled QuantumCircuit. (or preprocessed circuits)
| @unpack | ||
| def test_measurement_coefficient(self, pauli, bitstring, expected): | ||
| """Test measurement coefficients.""" | ||
| pauli = Pauli(pauli) | ||
| coeff = BackendEstimator._measurement_coefficient(bitstring, pauli) | ||
| self.assertEqual(coeff, expected) |
There was a problem hiding this comment.
Here's another case where a private method is being unit-tested (there's lots more in this file). This sort of thing really tightly constrains the structure of the underlying code, whereas what we really need is for all publicly visible behaviour to remain the same. It should not be a problem if a later implementation wanted to inline this function, or move it off the class, or move it to Rust, or have it return some different object, as long as the public behaviour of the entry points remained the same.
| def circuit_composition_examples() -> Iterator[ | ||
| tuple[QuantumCircuit, QuantumCircuit, QuantumCircuit] | ||
| ]: |
There was a problem hiding this comment.
This is a one-use function for parametrisation, and the objects it creates just get passed to a single builder function each - it would be better for the iterator just to be inlined in the data parametrisation for the specific test case. That would keep the data local to the test case, making it easier to read what's being tested.
Personally, I also find the building here very hard to read and track. I have to chase the logic through 5 different functions, which result in three different QuantumCircuits being created for the test case, and I have to remember the details of all of them to know whether I believe the test is correct or not. In general, test code should be obviously correct just from reading the test function, and I definitely wouldn't know that here.
(There's also the thing that all this complexity seems to just be in service of testing a private method anyway, which I already argued against.)
There was a problem hiding this comment.
Somewhat answered here: #9074 (comment)
|
Thank you for your comments, Jake. Actually, we have not discussed this refactoring beforehand. I think Pedro has some enhancement requests of |
|
Hi all, thanks for your reviews and comments. Just a few things to get us all in the same page:
Let's keep it up! |
pedrorrivero
requested review from
ikkoham
and removed request for
mtreinish
pedrorrivero
force-pushed
the
backend-estimator
branch
from
97e103b
to
b9041df
Compare
There was a problem hiding this comment.
I'm concerned that my understanding of your reasons aren't compatible with Terra's position as a fully open-source project, and with its position in the software stack.
You say you can't speak publicly about why you need some of these changes. If that's the case, how are we going to maintain those behaviours as invariant moving forwards, since we can't document the reasons? If things needed for IBM's "secret sauce", why are they being implemented here in Terra, and not in IBM-specific implementations of Estimator - one of the main reasons for the Estimator interface to exist in the first place? If there's things you can say publicly, it would be good to copy them into the PR context here - in 6 months' time, nobody will remember what was said in a meeting.
I actually think that "expanding the API" of this class is a bad thing, and I would be weakly opposed to it (but as I mentioned before, I won't attempt to exert control over something @ikkoham and @t-imamichi own). We don't really want people to use this class. It's here to ease users' transition into relying on the primitives, so they can take existing backends whose providers might not yet offer custom primitives, and raise them to the new API. We don't want people to be depending on this long term, and we certainly don't want them building advanced functionality off it specifically (as opposed to the general Estimator interface). Internal IBM code shouldn't be - we've got direct implementations of the primitives, and while we might use a class very similar to this internally, that's an internal choice, and not one that ought to be maintained in Terra's public interface.
I also don't agree that this has improved readability or maintainability of the class - you can see the evidence for that in the surprise that I, and to a lesser extent Takashi and Ikko, have had to some of the new structures and abstractions here, and all of us have experience as software developers. I get the impression that you might be a lot more comfortable with pure OO programming than the mixed-paradigm style that Python offers, but pure OO is not always (or even imo often) the right tool when you're not forced into it by the language. Here, I worry that overuse of OO-specific patterns is making things overcomplicated.
I've largely said what I had to say that's relevant, and there's clearly a big risk that we end up just talking about programming style, which is always going to vary between people. I don't agree with some of the design choices and I would be mildly opposed to merging any significant refactor of this class for the reasons I've given in previous comments (refactors should be "why?" not "why not?", and I don't buy the given "why") but my opinion is not very important in this area of Terra. Please don't consider this review a blocker despite my opposition and regardless of how you answer the questions I asked in various places - that decision is entirely Ikko and Takashi's as the code owners.
| ################################################################################ | ||
| ## OBSERVABLE DECOMPOSER | ||
| ################################################################################ | ||
| class ObservableDecomposer(ABC): |
There was a problem hiding this comment.
"Design Patterns" is specifically about pure OOP, and Python is not a pure OO language - we have first-class functions and far easier composition.
If we're talking programming principles, two absolute classics are "you aren't going to need it", and "keep it simple". For this 2-choice case, imo this inherited class structure is harder to read than basic conditional methods on the same class, since now the setup/teardown code for the abstract methods is separate from the rest of the logic; I have to chase the grouping logic through 3 possible places now to understand what might be happening. If we were implementing loads of options, it would be sensible to define an interface and invert the control (which isn't done here), but while there's only two, it's maybe overkill.
| """Test observable decomposer property.""" | ||
| estimator = BackendEstimator(Mock(Backend)) | ||
| self.assertTrue(estimator.abelian_grouping) | ||
| self.assertIsInstance(estimator._observable_decomposer, AbelianDecomposer) |
There was a problem hiding this comment.
Presumably the user setting abelian_grouping=True should cause some changes to the output they see from public methods? I'd test that, rather than deep specifics of the internal implementation.
Let's say you accept mine and Takashi's arguments above about the ObservableDecomposer class and remove it. You could keep all the public behaviour exactly the same while inlining NaiveDecomposer and AbelianDecomposer, but all your tests will suddenly fail, and (I could well be wrong on this) I'm not certain there are any end-to-end tests that would fail if the abelian_grouping option were suddenly to disappear. In that case, are these tests helping us catch bugs, or are they just making busy work for anybody changing the code? Users don't care about how the internals are implemented, and user-facing behaviour is what we care about not breaking, primarily.
| @property | ||
| def _observable_decomposer(self) -> ObservableDecomposer: | ||
| """Observable decomposer based on object's config.""" | ||
| if self.abelian_grouping: | ||
| return AbelianDecomposer() | ||
| return NaiveDecomposer() |
There was a problem hiding this comment.
Creating a new class every time is fairly surprising behaviour for a property (it makes it unusually costly to query). I'm guessing it's done here because abelian_grouping is exposed as explicitly mutable state, but we'd still avoid these construction costs if decompose and build_pauli were just inlined into methods on this same class that dispatched on abelian_grouping in the same way.
The extra abstractions in this class makes it difficult to spot that this actually causes n_circuits * (n_observables + 1) individual decomposer classes to be instantiated on every call to _compute. This is the kind of thing I'm talking about when I say that too many properties kill us by a thousand papercuts; you'd always think that the expression self._observable_decomposer should be fine to call in a loop (as is done here), but we're actually allocating a whole new class every time it happens. Couple that with the _observable_decomposer property triggering a second internal property (abelian_grouping) on every lookup as well, and you can see how this stuff adds up. (I apply similar logic in my opposition of too many validation checks of type-hinted attributes that are supposed to be mutated / constructed quickly in Python too.)
Of course, removing these particular instances is unlikely to have any massive impact on performance in this case, but it's the general principle that this kind of abstraction-heavy code weighs things down, and it's easily avoidable.
There was a problem hiding this comment.
Do you have any metrics on how costly it is to instantiate a new class, and what is the performance gain by not doing it?
I ask because this saves memory by not having to maintain one of these objects in the instance dictionary, it reduces issues with mutability as you mention and some other minor things. So I would be interested to know what is the tradeoff.
Also, do you have data on this slow-down that properties cause as apposed to plain attributes? I'd like to address this on that footing rather than in the abstract.
There was a problem hiding this comment.
I have started the review but will submit it in the middle of the review due to the volume.
Basically, all refactors are not recommended because of the risk. It would have been more acceptable if it were partial. It takes an enormous amount of time for both coding and review, but if you want to do it, I will do my best to review it.
| """Backend to use for circuit measurements.""" | ||
| return self._backend | ||
|
|
||
| @backend.setter |
There was a problem hiding this comment.
It is best not to make anything that does not have a use case. It makes maintenance difficult and may cause bugs.
| def __init__( | ||
| self, | ||
| backend: Backend, | ||
| *, # TODO: allow backend as positional after removing deprecations |
| # pylint: disable=missing-raises-doc | ||
| def __init__( | ||
| self, | ||
| backend: Backend, |
There was a problem hiding this comment.
Backend is broader than BackendV1 | BackendV2. It means this class supports BackendV3 in the future.
There was a problem hiding this comment.
As it should 🙂
It is equivalent now, nonetheless
There was a problem hiding this comment.
It's not the same. Clearly, the code only supports BackendV2 https://github.com/Qiskit/qiskit-terra/pull/9074/files#diff-15ffd51433f241eb453c3dc3eb62f89d82b1c3fbeeb43eea8c305a72f5d81054R111.
It's going to be an error in the future.
There was a problem hiding this comment.
Okay, I have a few questions regarding this:
- Is there any other class implementing
Backendother thanBackendV1andBackendV2as of today? - Are there any ongoing plans for
BackendV3, and when is it expected to release?
The self.backend property is currently normalized to BackendV2, which does not mean that it does not support BackendV1 or could not support other types in the future. As a matter of fact, it does handle BackendV1 through BackendV2Converter (i.e. through backend.setter), and when a new version comes out,BackendV2Converter should be updated in order to account for that (i.e. to convert to V2 from this new class) and this code will not error.
There was a problem hiding this comment.
Can you remember to update when a new version is released? If you are unaware and a new version is released, this will be a bug. We can't guarantee the future, so we do the best we can at this time.
| ################################################################################ | ||
| ## OBSERVABLE DECOMPOSER | ||
| ################################################################################ | ||
| class ObservableDecomposer(ABC): |
There was a problem hiding this comment.
It is a premature abstraction. When we want to implement a Grouper (Decomposer) other than them, this design cannot handle it? Only one qubit measurement basis can be represented.
| max_circuits: int = getattr(self.backend, "max_circuits", None) or total_circuits | ||
|
|
||
| # Raw results | ||
| jobs: tuple[Job] = tuple( |
There was a problem hiding this comment.
Unnecessary type conversion to tuple makes difficult to read.
There was a problem hiding this comment.
No type conversion is happening in this line
There was a problem hiding this comment.
Yes. I know. But it is simply difficult to read. It's not Pythonic.
There was a problem hiding this comment.
Original:
jobs = [
backend.run(circuits[pos : pos + max_circuits], **run_options)
for pos in range(0, len(circuits), max_circuits)
]Refactor:
jobs: tuple[Job] = tuple(
self.backend.run(circuits[split : split + max_circuits], **run_options)
for split in range(0, total_circuits, max_circuits)
)I don't see much of an stylistic difference other than the type annotation, and use of an immutable data structure instead of a mutable one (since the resulting instance should not be altered). Not sure how this makes it less pythonic, but what would you propose instead?
There was a problem hiding this comment.
The default in the language specification is list, and there is no reason to include unnecessary conversions. I also find the latter more difficult because I have to convert it once in my head.
| """Backend to use for circuit measurements.""" | ||
| return self._backend | ||
|
|
||
| @backend.setter |
There was a problem hiding this comment.
Could you remove this method?
| # pylint: disable=missing-raises-doc | ||
| def __init__( | ||
| self, | ||
| backend: Backend, |
There was a problem hiding this comment.
Can you remember to update when a new version is released? If you are unaware and a new version is released, this will be a bug. We can't guarantee the future, so we do the best we can at this time.
| counts_iter = (counts for job_counts in job_counts_iter for counts in job_counts) | ||
| counts_list: list[Counts] = [] | ||
| for counts, circuit in zip(counts_iter, circuits): | ||
| counts.metadata = circuit.metadata # TODO: add `Counts.metadata` attr |
There was a problem hiding this comment.
Yes, this is in fact the worst thing from this design.
I hate it.
But at the same time it makes the code significantly cleaner and easier to follow by simply making the metadata in the executed circuits be ported to the associated counts metadata. I would like to propose this as new PR.
| self, | ||
| observable: SparsePauliOp, | ||
| ) -> tuple[SparsePauliOp]: | ||
| return tuple(observable) |
There was a problem hiding this comment.
I prefer list to tuple for variable length. (not strong opinion but I can't find the reason of immutable)
There was a problem hiding this comment.
Main reason for this to be immutable is that the decomposition is fixed, and it does not make sense to mutate it. If you you change it will no longer be the decomposition so better to have it as immutable to avoid accidental updates.
|
I opened the PR to reflect comments: pedrorrivero#1 |
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
|
After careful consideration and offline discussions I have decided to drop this PR and open a new repo for The code has now been successfully migrated, so I proceed to close this PR. Thank you very much to everyone for your contributions! |
Summary
Holistic refactor of the
BackendEstimatorclass:To-do
max_experimentsis surpassedTODOsbackend_estimator_devtobackend_estimator(and same thing for tests)