-
Notifications
You must be signed in to change notification settings - Fork 1k
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
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FMI - how was this value chosen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock the user, but let's still see if we can get a less explicit chunking if possible as a followup?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pragma: no cover
? We expect this to be exercised by the tests, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Related to b/372751875
Throttle in-flight requests which otherwise would cause a
Quota exceeded exception
Where as before sending
50 circuits
tosampler.run_batch
would cause an exception, now it completes in~31s
,100 jobs in ~61s
, and500 jobs in ~276s
for longer circuits.