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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(python): chunked_batch helper #3069
base: main
Are you sure you want to change the base?
Conversation
鉁旓笍 Code generated!
|
@millotp @morganleroi don't want to bother you, but could you review this PR? It should fix a bug in the Python API client. |
@@ -223,7 +223,7 @@ | |||
responses: List[BatchResponse] = [] | |||
for i, obj in enumerate(objects): | |||
requests.append(BatchRequest(action=action, body=obj)) | |||
if i % batch_size == 0: | |||
if len(requests) == batch_size or i == len(objects) - 1: |
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 don't think you can take the length of a Iterable
type, because the length is determined after iterating over it
I think we have that issue in other clients too, i'll add a ticket to our backlog just to be sure alternatively, a universal solution, might be to first build a list of list of batch by iterating over all objects, then iterating on the list to do the requests |
馃Л What and Why
Fix a bug in the
chunked_batch
helper for the Python API client.The bug shows when using the
replace_all_objects
helper鈥攖he new index won't have all the records:0 % batch_size == 0
) will be sent.1 + n x 1,000
records will be sent. The records after the last integer multiple ofbatch_size
won't be sent.Changes included:
request
array reaches thebatch_size
, or when the end of theobjects
list is reached.objects
"list" inreplace_all_objects
andchunked_batch
to be anIterable
instead of an explicit list. This makes it possible to use also with things like list comprehensions.馃И Test
No additional test added (didn't quite know how we could test this, as the API is mocked in these tests... we would have to test whether the replaced index has the correct records in it).