Integrate StreamManager with run_sweep() #6285

verult · 2023-09-11T20:47:47Z

This PR enables the streaming RPC for end users by default, while also providing a feature flag to turn it off.

Added a feature flag defaulting to True. My preference for a feature flag mechanism is not to add a method parameter, since it is configuration instead of being part of the API, and the flag could be removed in the future.
- Defaulting to True because otherwise it's unlikely that users will try out streaming.
- When a user uses this feature, a message will be printed describing how to turn it off.
Added logic to temporarily rewrite processor_ids to processor_id prior to full deprecation of processor_ids in Engine.run_sweep(), so that EngineClient.run_job_over_stream() can omit the processor_ids parameter.

* Added a feature flag defaulting to True. * Added logic to temporarily rewrite `processor_ids` to `processor_id` prior to full deprecation of `processor_ids` in `Engine.run_sweep()`.

cirq-google/cirq_google/engine/engine_job_test.py

maffoo · 2023-09-11T21:14:06Z

cirq-google/cirq_google/engine/engine_processor_test.py

@@ -798,22 +799,13 @@ def test_list_reservations_time_filter_behavior(list_reservations):

 @mock.patch('cirq_google.engine.engine_client.EngineClient', autospec=True)
 def test_run_sweep_params(client):


I think having to change these tests because of a feature flag is a bit of a code-smell, especially since they would have to be changed back if we flip the default value of the flag. You might consider instead putting the stream configuration on EngineContext, which has other things like the protocol version and serializer that can be changed. Then for tests we can exercise both the streaming and non-streaming execution by creating an appropriate context, and those tests will continue to work even if we change the default value of the flag.

Thanks for the catch! Yeah I agree this is a code smell. Using EngineContext sounds good to me as long as it's not user-facing. Is this true for this class?

EngineContext is not typically used directly by end-users. It's used to share API state between the various resource classes.

wcourtney · 2023-09-18T23:33:51Z

cirq-google/cirq_google/engine/engine.py

@@ -59,6 +59,11 @@
 _R = TypeVar('_R')


+# Feature gate for making Quantum Engine requests using the stream RPC.
+# TODO(#5996) Remove the flag once the feature is stable.
+_STREAM_FEATURE_FLAG = True


Mentioned this offline - can we make this False initially so that we can explicitly enable this in a followup PR from the feature code itself?

As we discussed offline, I'll leave this as True and open another PR to turn this off in case things break.

cirq-google/cirq_google/engine/engine.py

wcourtney · 2023-09-18T23:41:26Z

cirq-google/cirq_google/engine/engine.py

+
+        if _STREAM_FEATURE_FLAG:
+            print(
+                '\nRunning using the Quantum Engine stream RPC. To revert to unary RPCs, '


It seems annoying to see this for every call to run_sweep_async. Can we just notify out oncall/team and/or the cirq team to be aware of this?

I could see that, although given that I don't expect the feature flag to stay around for long (probably no longer than 3 months), I'm erring towards over-notifying and having the switch readily accessible when the user needs it most. WDYT?

I'll also add a TODO for removing the feature gate.

If you must notify, doing this at most once should be sufficient. e.g., this could be done when the engine or context is created, but I would prefer not to notify go general users since the "stream" vs "unary" feature is opaque to them.

Chatted offline - will remove the log message.

cirq-google/cirq_google/engine/engine_job.py

wcourtney · 2023-09-19T00:27:47Z

cirq-google/cirq_google/engine/engine_job.py

-               batched results or the result of a focused calibration.
+                batched results or the result of a focused calibration.
+            stream_job_response_future: If set, the job is sent over the Quantum Engine
+                QuantumRunStream bidirectional stream, and the future is completed when the Engine


Nit: is it important that the future come from a bidirectional stream, or is there a more general way to frame what parameter does? e.g., is this just a future that resolves to the job result?

cirq-google/cirq_google/engine/engine_job.py

wcourtney · 2023-09-20T23:39:40Z

cirq-google/cirq_google/engine/engine_processor_test.py

@@ -798,22 +799,13 @@ def test_list_reservations_time_filter_behavior(list_reservations):

 @mock.patch('cirq_google.engine.engine_client.EngineClient', autospec=True)
 def test_run_sweep_params(client):


EngineContext is not typically used directly by end-users. It's used to share API state between the various resource classes.

verult · 2023-09-21T23:24:15Z

@maffoo Sorry I haven't fully addressed all of your comments just yet. Will add tests based on the EngineContext feature gate.

…ing feature flag

…t/end-user-integration-final

verult · 2023-09-21T23:37:22Z

Added tests and ready for review

wcourtney · 2023-09-22T22:15:27Z

cirq-google/cirq_google/engine/engine_job.py

@@ -69,6 +69,9 @@ def __init__(
        context: 'engine_base.EngineContext',
        _job: Optional[quantum.QuantumJob] = None,
        result_type: ResultType = ResultType.Program,
+        stream_job_response_future: Optional[


Copied from a prior review:

Is it important that the future come from a bidirectional stream, or is there a more general way to frame what parameter is used for? e.g., is this just a future that resolves to the job result? If so, there's probably not a reason to restrict it to the QuantumRunStream in the documentation.

Updated docstring for the initializer, and changing the field name to job_response_future.

wcourtney · 2023-10-03T22:24:04Z

cirq-google/cirq_google/engine/engine_job.py

-                when the Engine responds over the stream. When a caller asks for the job result,
-                EngineJob will await this future to wait for the result to be available from the
-                Engine.
+            job_response_future: A future to be completed when the job result is available.


Nit: should this be job_result_future since it resolves to the job_result?

wcourtney

LGTM % the nit.

codecov · 2023-10-04T18:01:29Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (c6f60bc) 97.89% compared to head (7f41e3b) 97.89%.

❗ Current head 7f41e3b differs from pull request most recent head 6fe1fb2. Consider uploading reports for the commit 6fe1fb2 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #6285    +/-   ##
========================================
  Coverage   97.89%   97.89%            
========================================
  Files        1104     1104            
  Lines       95897    96038   +141     
========================================
+ Hits        93876    94016   +140     
- Misses       2021     2022     +1

Files	Coverage Δ
cirq-google/cirq_google/engine/engine.py	`98.30% <100.00%> (+0.14%)`	⬆️
cirq-google/cirq_google/engine/engine_client.py	`100.00% <100.00%> (ø)`
...rq-google/cirq_google/engine/engine_client_test.py	`100.00% <100.00%> (ø)`
cirq-google/cirq_google/engine/engine_job.py	`97.59% <100.00%> (+0.12%)`	⬆️
cirq-google/cirq_google/engine/engine_job_test.py	`100.00% <100.00%> (ø)`
...google/cirq_google/engine/engine_processor_test.py	`100.00% <100.00%> (ø)`
cirq-google/cirq_google/engine/engine_test.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

Integrate StreamManager with run_sweep()

abb41a3

* Added a feature flag defaulting to True. * Added logic to temporarily rewrite `processor_ids` to `processor_id` prior to full deprecation of `processor_ids` in `Engine.run_sweep()`.

verult requested review from wcourtney and dstrain115 September 11, 2023 20:47

verult requested review from vtomole, cduck and a team as code owners September 11, 2023 20:47

CirqBot added the size: L 250< lines changed <1000 label Sep 11, 2023

maffoo reviewed Sep 11, 2023

View reviewed changes

[WIP] Addressed maffoo's comments

aa09e10

wcourtney requested changes Sep 20, 2023

View reviewed changes

Addressed wcourtney's comments

8c56aaa

verult requested review from maffoo and wcourtney September 21, 2023 23:19

verult added 2 commits September 21, 2023 23:32

Modified engine_processor_test to take into account the enable_stream…

bd397c9

…ing feature flag

Merge branch 'master' of github.com:quantumlib/Cirq into stream-clien…

7242a57

…t/end-user-integration-final

wcourtney reviewed Sep 22, 2023

View reviewed changes

Address wcourtney's comments

091bef9

verult requested a review from wcourtney October 3, 2023 21:36

wcourtney reviewed Oct 3, 2023

View reviewed changes

wcourtney approved these changes Oct 3, 2023

View reviewed changes

Rename job_response_future to job_result_future

df72eaa

verult enabled auto-merge (squash) October 3, 2023 23:12

Fix coverage check

7f41e3b

Merge branch 'master' into stream-client/end-user-integration-final

6fe1fb2

verult merged commit 4dc36d5 into quantumlib:master Oct 4, 2023

verult mentioned this pull request Oct 5, 2023

Create separate unary and streaming RPC tests in engine_test.py #6311

Merged

verult mentioned this pull request Oct 20, 2023

Streaming client: end user integration prototype #6120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate StreamManager with run_sweep() #6285

Integrate StreamManager with run_sweep() #6285

verult commented Sep 11, 2023

maffoo Sep 11, 2023

verult Sep 11, 2023

wcourtney Sep 20, 2023

wcourtney Sep 18, 2023

verult Sep 21, 2023

wcourtney Sep 18, 2023

verult Sep 21, 2023

wcourtney Sep 21, 2023

verult Oct 3, 2023

wcourtney Sep 19, 2023

wcourtney Sep 20, 2023

verult commented Sep 21, 2023

verult commented Sep 21, 2023

wcourtney Sep 22, 2023

verult Oct 2, 2023

wcourtney Oct 3, 2023

wcourtney left a comment

codecov bot commented Oct 4, 2023 •

edited

Loading

		@@ -798,22 +799,13 @@ def test_list_reservations_time_filter_behavior(list_reservations):

		@mock.patch('cirq_google.engine.engine_client.EngineClient', autospec=True)
		def test_run_sweep_params(client):

Integrate StreamManager with run_sweep() #6285

Integrate StreamManager with run_sweep() #6285

Conversation

verult commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Sep 21, 2023

verult commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcourtney left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

codecov bot commented Oct 4, 2023 •

edited

Loading