Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Return a single results object instead of always a list - IonQ

Open splch opened this issue 7 months ago • 6 comments

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

splch avatar Apr 18 '25 21:04 splch

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 99.38%. Comparing base (34f1f99) to head (23ce8a9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7285   +/-   ##
=======================================
  Coverage   99.38%   99.38%           
=======================================
  Files        1089     1089           
  Lines       97551    97583   +32     
=======================================
+ Hits        96950    96982   +32     
  Misses        601      601           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Apr 21 '25 16:04 codecov[bot]

@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.

mhucka avatar Apr 24 '25 22:04 mhucka

@dstrain115 this is a good question. The answer is unfortunately yes, but it's attempting to revert a change in interface caused by https://github.com/quantumlib/Cirq/pull/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.

Cynocracy avatar Apr 29 '25 17:04 Cynocracy

@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 )

mhucka avatar Apr 30 '25 03:04 mhucka

@Cynocracy Do we have a consensus as to whether to proceed on this change? (Regarding previous comment: "FWIW, I am conflicted on whether it makes sense to change back") If there is backwards incompatibility, maybe we can at least document it so users are aware?

dstrain115 avatar May 14 '25 22:05 dstrain115

@dstrain115 Documenting sounds like a good strategy to me here. Sorry for the lack of response, we've been trying to reach out to users to get some feedback, but we've been somewhat delayed by other priorities.

@splch would you mind adding documentation for the (previous) change to our docs?

Cynocracy avatar May 14 '25 23:05 Cynocracy

I believe this change is still missing documentation as per the previous comment by @Cynocracy

dstrain115 avatar Aug 06 '25 13:08 dstrain115

@splch do you still want to move forward with this PR?

Cynocracy avatar Aug 07 '25 19:08 Cynocracy

@Cynocracy @spich

Checking in on this PR since it seems to have stalled out. Is this still something we want to do? I think it was almost ready to go pending some documentation tweaks and confirmation from IonQ side.

dstrain115 avatar Oct 08 '25 13:10 dstrain115

hi @dstrain115 yes this is still something we want to do! sorry about the delay. ill get to documenting and resolving merge conflicts :)

splch avatar Oct 09 '25 19:10 splch

@splch There's also a cirq-ionq issue at https://github.com/quantumlib/Cirq/issues/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.

mhucka avatar Oct 09 '25 20:10 mhucka