Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

splch
Copy link
Contributor

@splch splch commented Apr 18, 2025

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 splch requested review from dabacon, vtomole and a team as code owners April 18, 2025 21:04
Copy link

codecov bot commented Apr 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (63b0f17) to head (5474b73).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7285   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files        1108     1108           
  Lines       96453    96485   +32     
=======================================
+ Hits        95175    95207   +32     
  Misses       1278     1278           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@@ -296,3 +296,28 @@ def test_service_remote_host_default():
def test_service_remote_host_from_env_var_cirq_ionq_precedence():
service = ionq.Service(api_key='tomyheart')
assert service.remote_host == 'http://example.com'


def test_service_run_unwraps_single_result_list():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test for the correctness of the order of multiple results being returned?

@mhucka
Copy link
Contributor

mhucka commented Apr 24, 2025

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

Copy link
Contributor

@Cynocracy Cynocracy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new run batch preserves order test covers what I was hoping for :) lgtm

Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause any backwards incompatibility for programs people have already written?

@mhucka mhucka added kind/health For CI/testing/release process/refactoring/technical debt items interface/cirq-ionq labels Apr 29, 2025
@mhucka mhucka changed the title return a single results object instead of always a list - IonQ Return a single results object instead of always a list - IonQ Apr 29, 2025
@Cynocracy
Copy link
Contributor

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

@mhucka
Copy link
Contributor

mhucka commented Apr 30, 2025

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

@github-actions github-actions bot added the size: M 50< lines changed <250 label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface/cirq-ionq kind/health For CI/testing/release process/refactoring/technical debt items size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants