Skip to content
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

Integrate StreamManager with run_sweep() #17

Conversation

verult
Copy link
Owner

@verult verult commented Jul 31, 2023

Looking for approval on the general approach first before updating run_batch() and run_calibration() and adding tests.

  • Connects the user interface of engine circuit execution with the StreamManager introduced in Add StreamManager quantumlib/Cirq#6199.
  • QuantumRunStream supports CreateQuantumProgramAndJob requests, so the implementation here bypasses EngineProgram altogether, which will continue to use unary RPCs. This also aligns with our eventual goal of removing programs as a resource.
  • ProcessorSampler, the recommended interface to run circuits on QuantumEngine, uses Engine.run_sweep() under the hood.

@dstrain115

@verult
Copy link
Owner Author

verult commented Jul 31, 2023

@maffoo

Copy link

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

Seems fine to me as long as it works.

)

run_sweep = duet.sync(run_sweep_async)

# TODO update

Choose a reason for hiding this comment

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

I would either explain what needs to be updated, connect it to a github issue, or just delete the comment.
As is, it does not give much useful information.

Same for below.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will do - I left these here as a reminder for myself to implement this in the full PR.


Sends the request over the Quantum Engine QuantumRunStream bidirectional stream, and returns
a future for the stream response. The future will be completed with a `QuantumResult` if
the job is successful; otherwise, it will be completed with a QuantumJob.

Choose a reason for hiding this comment

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

Missing args.

run_context: any_pb2.Any,
program_description: Optional[str] = None,
program_labels: Optional[Dict[str, str]] = None,
priority: Optional[int] = None,

Choose a reason for hiding this comment

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

What's this priority about? I don't recall this ever being used. Is it new?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's in the existing create_job_async() call: https://github.com/quantumlib/Cirq/blob/master/cirq-google/cirq_google/engine/engine_client.py#L382C40-L382C40

Not sure how it's used today. Happy to remove if we actually don't use this. cc @wcourtney

Copy link
Owner Author

@verult verult left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

)

run_sweep = duet.sync(run_sweep_async)

# TODO update
Copy link
Owner Author

Choose a reason for hiding this comment

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

Will do - I left these here as a reminder for myself to implement this in the full PR.

run_context: any_pb2.Any,
program_description: Optional[str] = None,
program_labels: Optional[Dict[str, str]] = None,
priority: Optional[int] = None,
Copy link
Owner Author

Choose a reason for hiding this comment

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

It's in the existing create_job_async() call: https://github.com/quantumlib/Cirq/blob/master/cirq-google/cirq_google/engine/engine_client.py#L382C40-L382C40

Not sure how it's used today. Happy to remove if we actually don't use this. cc @wcourtney

@verult
Copy link
Owner Author

verult commented Jul 31, 2023

I manually tested this. There seems to be some problems on the server side, but after a long time a create job eventually returns a QuantumJob, so client code seems to be working. Will add more unit tests here.

@verult
Copy link
Owner Author

verult commented Aug 1, 2023

Added tests and updated docstrings. I also deferred migrations for run_batch and run_calibration because this PR is getting too big.

Ready for full review. Thanks!

@verult verult changed the title Integrate StreamManager with run() methods Integrate StreamManager with run_sweep() Aug 4, 2023
* Move AsyncioExecutor to separate file.

* Add StreamManager

* Fix test failures

* Fixed cancellation races and asyncio.Queue loop issue

* Remove the redundant unsubscribe

* Addressed wcourtney's comments

* Fix linter and coverage failures; added done check to _manage_stream_loop_future

* Address maffoo's comments
@verult verult closed this Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants