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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(python): chunked_batch helper #3069

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

kai687
Copy link
Contributor

@kai687 kai687 commented May 7, 2024

馃Л 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:

  • < 1,000 records: only the first record (0 % batch_size == 0) will be sent.
  • > 1,000 records: only the first 1 + n x 1,000 records will be sent. The records after the last integer multiple of batch_size won't be sent.

Changes included:

  • Send the request when the length of the request array reaches the batch_size, or when the end of the objects list is reached.
  • Change the type of the objects "list" in replace_all_objects and chunked_batch to be an Iterable 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).

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 7, 2024

鉁旓笍 Code generated!

Name Link
馃敤 Triggered by 513b0e5914e0782b2ea7192cf300ffc8452c57fc
馃攳 Generated code 50327831490a56a4c45df0831abd70ba79b1c195
馃尣 Generated branch generated/fix/replace_all_objects

@kai687 kai687 marked this pull request as ready for review May 7, 2024 16:48
@kai687 kai687 requested a review from a team as a code owner May 7, 2024 16:48
@kai687 kai687 requested review from morganleroi and millotp May 7, 2024 16:48
Copy link

github-actions bot commented May 16, 2024

@github-actions github-actions bot temporarily deployed to pull request May 16, 2024 09:54 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 16, 2024 16:21 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 17, 2024 07:42 Inactive
@kai687
Copy link
Contributor Author

kai687 commented May 17, 2024

@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:
Copy link
Collaborator

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

@shortcuts
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants