Return a single results object instead of always a list - IonQ #7285
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7285 +/- ##
=======================================
Coverage 99.38% 99.38%
=======================================
Files 1090 1090
Lines 98300 98376 +76
=======================================
+ Hits 97695 97771 +76
Misses 605 605 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| assert service.remote_host == 'http://example.com' | ||
|
|
||
|
|
||
| def test_service_run_unwraps_single_result_list(): |
There was a problem hiding this comment.
can you add a test for the correctness of the order of multiple results being returned?
|
@splch Thank you for this work! Could you let us know the status of this? For example, there was the comment from @Cynocracy to add a test, but it's not immediately clear if that was resolved. |
Cynocracy
left a comment
Cynocracy
left a comment
There was a problem hiding this comment.
The new run batch preserves order test covers what I was hoping for :) lgtm
dstrain115
left a comment
dstrain115
left a comment
There was a problem hiding this comment.
Will this cause any backwards incompatibility for programs people have already written?
mhucka
added
kind/health
mhucka
changed the title
|
@dstrain115 this is a good question. The answer is unfortunately yes, but it's attempting to revert a change in interface caused by #6652 (which made it so that jobs that previously returned a single Result were returning an Iterable of them). FWIW, I am conflicted on whether it makes sense to change back, as imo, both changes are somewhat burdensome to users. |
Is there a way to consult users (perhaps some especially major users) and ask them for their input? (I guess this is a question for @Cynocracy ) |
|
@splch There's also a cirq-ionq issue at #5216 that we were wondering about. It's unrelated to this PR, but if you're doing work on cirq-ionq and have a few moment to look at that, we'd welcome your input. It's not clear to us whether the issue still exists, so ti might be a very quick matter of just closing the issue. |
github-actions
bot
added
size: L
docs/hardware/ionq/jobs.md
Outdated
| > **Note - result shape of `Job.results()`:** For jobs created from a **single circuit**, | ||
| > `job.results()` returns a **single** `ionq.QPUResult` or `ionq.SimulatorResult`. | ||
| > For **batch** jobs, it returns a **list** of those results. To write code that | ||
| > works with either shape: | ||
| > | ||
| > ```python | ||
| > r = job.results() | ||
| > results_list = r if isinstance(r, list) else [r] | ||
| > ``` | ||
| > | ||
| > Each entry can be converted to a `cirq.Result` via `.to_cirq_result(...)`. | ||
| > (`Service.run(...)` continues to return a single `cirq.Result`.) | ||
There was a problem hiding this comment.
@splch I know this PR was approved, but before merging, I wanted to check if the use of a Markdown blockquote is intentional here. The result in the formatted output is not great (e.g., it has hard line breaks). I would prefer to see this without the blockquote (i.e., without the leading > characters) unless I'm missing something.
There was a problem hiding this comment.
Ah sounds good - I was going for a note-looking block but I'm happy to change it to normal formatting :)
There was a problem hiding this comment.
Ah sounds good - I was going for a note-looking block but I'm happy to change it to normal formatting :)
Lemme see if something can be done to keep that aspect yet avoid the hard newlines …
There was a problem hiding this comment.
@splch My apologies for the long delay getting back to this. I see now that it looks like you updated the format. I looked into the options for doing callouts or highlighted notes in a way compatible with both GitHub-flavored Markdown and the system used to produce the Cirq documentation on quantumai.google, but could not come up with a straightforward solution short of using raw HTML. However, I had another idea: slightly restructuring the end of that page and putting the Job.results() info into its own subsection. I pushed the proposed changes to this branch to demonstrate it.
Tell me if this be a reasonable approach, and whether the new text is accurate and clear.
This is a proposed alternative to the previous formatting and text.
github-actions
bot
added
size: M
github-actions
bot
added
size: L
|
@splch there were some test failures in CI; these might have been due to some changes in Cirq since the PR was originally submitted. I tried to fix them, and took the liberty of pushing the changes to your PR branch in an effort to help make up for lost time. Hopefully it's correct. Please feel free to make any changes necessary. |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
LGTM, but please check if you agree with the changes I made (and feel free to make any corrections).
5 participants
ionq results api can return a list of results or a single result, so this change allows for either a list or single element to be returned from the results endpoint