add request limiter when polling for results #6774
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6774 +/- ##
=======================================
Coverage 97.84% 97.84%
=======================================
Files 1077 1077
Lines 92745 92791 +46
=======================================
+ Hits 90742 90788 +46
Misses 2003 2003 ☔ View full report in Codecov by Sentry. |
senecameeks
requested review from
wcourtney,
vtomole,
verult and
a team
as code owners
|
This is still in a draft state, as I verify the solution |
|
@wcourtney PTAL |
| @@ -34,6 +36,8 @@ | |||
| class Sampler(metaclass=value.ABCMetaImplementAnyOneOf): | |||
| """Something capable of sampling quantum circuits. Simulator or hardware.""" | |||
|
|
|||
| CHUNK_SIZE: int = 16 | |||
There was a problem hiding this comment.
FMI - how was this value chosen?
There was a problem hiding this comment.
I added a comment: Users have a rate limit of 1000 QPM for read/write requests to the Quantum Engine. 1000/60 ~= 16 QPS. So requests are sent in chunks of size 16 per second..
And empirically I verified chunk sizes over 16 lead to Quota exceeded exceptions for large batches.
| _chunked(repetitions, self.CHUNK_SIZE), | ||
| ): | ||
| # Run_sweep_async for the current chunk | ||
| await duet.sleep(1) # Delay for 1 second between chunk |
There was a problem hiding this comment.
Is this delay needed? If we wait for completion of the chunk (as it appears we do), then we should be able to immediately begin another chunk.
There was a problem hiding this comment.
Yes, without it we still run into the quota exception when polling for results (e.g for 100+ jobs with relatively shallow circuits).
Empirically, I found that sleeping for 1s removes the error for both shallow and deep circuits
cirq-core/cirq/work/sampler_test.py
Outdated
| @duet.sync | ||
| @mock.patch('duet.pstarmap_async') | ||
| @pytest.mark.parametrize('call_count', [1, 2, 3]) | ||
| async def test_run_batch_async_sends_circuits_in_chunks(mock, call_count): |
There was a problem hiding this comment.
Can you add a test that verifies that the multiple chunks aren't all running at once? e.g., check that the no calls are made with chunk N before check N-1 completes. That's an important requirement for this feature.
cirq-core/cirq/work/sampler.py
Outdated
| @@ -449,3 +484,8 @@ def _get_measurement_shapes( | |||
| ) | |||
| num_instances[key] += 1 | |||
| return {k: (num_instances[k], qid_shape) for k, qid_shape in qid_shapes.items()} | |||
|
|
|||
|
|
|||
| def _chunked(iterable: Sequence[Any], n: int) -> Iterator[tuple[Any, ...]]: # pragma: no cover | |||
There was a problem hiding this comment.
Why pragma: no cover? We expect this to be exercised by the tests, right?
There was a problem hiding this comment.
it's a private method so it shouldn't be explicitly tested, but it is implicitly tested in the added tests.
cirq-core/cirq/work/sampler.py
Outdated
| @@ -449,3 +484,8 @@ def _get_measurement_shapes( | |||
| ) | |||
| num_instances[key] += 1 | |||
| return {k: (num_instances[k], qid_shape) for k, qid_shape in qid_shapes.items()} | |||
|
|
|||
|
|
|||
| def _chunked(iterable: Sequence[Any], n: int) -> Iterator[tuple[Any, ...]]: # pragma: no cover | |||
There was a problem hiding this comment.
The typing allows the function to take a Sequence[int] and convert it to Iterator[tuple[str, ...]], but we can make a stricter guarantee that forbids the int -> str conversion. Can we convey that with generics?
https://mypy.readthedocs.io/en/stable/generics.html#generic-functions
Related to b/372751875
Throttle in-flight requests which otherwise would cause a
Quota exceeded exceptionWhere as before sending
50 circuitstosampler.run_batchwould cause an exception, now it completes in~31s,100 jobs in ~61s, and500 jobs in ~276sfor longer circuits.