Fix EngineProgram.get_circuit(_async) empty code detection#7850
Conversation
706ffe9 to
b68f25f
Compare
| if self._program is None or self._program.code is None: | ||
| # The code field is an any_pb2.Any and is always set. But if the program has not | ||
| # been fetched this field may be empty, which we can see by checking the type_url. | ||
| if self._program is None or not self._program.code or not self._program.code.type_url: |
There was a problem hiding this comment.
Should it be at the second alternative or not self._program.code.value?
The bool(any_pb2.Any()) is True so the middle expression is effectively or False or ...
Also can we add a test that would cover the fixed failure?
There was a problem hiding this comment.
This is mostly defense in depth because I didn't want to rely on the code attribute being set. As for whether we check type_url or value I think either is probably ok. I don't know if valid proto messages can ever have an empty byte representation, but I presume not? Do you have a particular reason to prefer value over type_url?
There was a problem hiding this comment.
If we trust the type annotation self._program can be either None or a QuantumProgram which is guaranteed to have a .code field of the any_pb2.Any type.
I was puzzled because or not self._program.code is effectively or False and thus redundant. I wondered if the intent was to have some non-const expression there so I suggested the value field. Otherwise I have no preference, dropping or not self._program.code would be fine with me too.
There was a problem hiding this comment.
Yes, it's the types of QuantumProgram itself that concerns me, because the proto-plus library does some metaclass magic so it's not clear to me how its type annotations work. I'd prefer to keep this for defense in depth for now.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7850 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 1104 1104
Lines 98988 98990 +2
=======================================
+ Hits 98566 98569 +3
+ Misses 422 421 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Updated tests to include case with a |
eliottrosenberg
left a comment
There was a problem hiding this comment.
Fixes b/477955102
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with an optional inline suggestion.
No description provided.