perf: Parallelize read calls by table and batch #4619
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robhowley
changed the title
robhowley
force-pushed
the
rh-parallel
branch
from
cbf0773
to
66d5328
Compare
robhowley
changed the title
Signed-off-by: Rob Howley <[email protected]>
Signed-off-by: Rob Howley <[email protected]>
Signed-off-by: Rob Howley <[email protected]>
Signed-off-by: Rob Howley <[email protected]>
robhowley
force-pushed
the
rh-parallel
branch
from
667bdbd
to
e6fcd78
Compare
robhowley
commented
Oct 12, 2024
Comment on lines
+312
to
+322
| batches = [] | ||
| entity_id_batches = [] | ||
| while True: | ||
| batch = list(itertools.islice(entity_ids_iter, batch_size)) | ||
| if not batch: | ||
| break | ||
| entity_id_batch = self._to_client_batch_get_payload( | ||
| online_config, table_name, batch | ||
| ) | ||
| batches.append(batch) | ||
| entity_id_batches.append(entity_id_batch) |
There was a problem hiding this comment.
construct the batches of ids/entity_ids that we'll be looking up
robhowley
commented
Oct 12, 2024
Comment on lines
+325
to
+332
| response_batches = await asyncio.gather( | ||
| *[ | ||
| client.batch_get_item( | ||
| RequestItems=entity_id_batch, | ||
| ) | ||
| for entity_id_batch in entity_id_batches | ||
| ] | ||
| ) |
There was a problem hiding this comment.
make those batch requests in parallel.
note: gather maintains order
robhowley
commented
Oct 12, 2024
Comment on lines
+335
to
+343
| for batch, response in zip(batches, response_batches): | ||
| result_batch = self._process_batch_get_response( | ||
| table_name, | ||
| response, | ||
| entity_ids, | ||
| batch, | ||
| to_tbl_response=to_tbl_resp, | ||
| ) | ||
| result_batches.append(result_batch) |
There was a problem hiding this comment.
format the responses to the final format. we iterate through the list three times in stead of one, but make up for it in asyncing the batches
robhowley
commented
Oct 12, 2024
Comment on lines
+263
to
+272
| all_responses = await asyncio.gather( | ||
| *[ | ||
| query_table(table, requested_features) | ||
| for table, requested_features in grouped_refs | ||
| ] | ||
| ) | ||
|
|
||
| for (idxs, read_rows), (table, requested_features) in zip( | ||
| all_responses, grouped_refs | ||
| ): |
There was a problem hiding this comment.
when requesting features across multiple tables, we can parallelize the calls to each.
robhowley
requested review from
shuchu,
franciscojavierarceo and
tokoko
and removed request for
a team
| @@ -240,7 +240,7 @@ async def get_online_features_async( | |||
| native_entity_values=True, | |||
| ) | |||
|
|
|||
| for table, requested_features in grouped_refs: | |||
| async def query_table(table, requested_features): | |||
There was a problem hiding this comment.
can you add type hints?
|
lgtm |
franciscojavierarceo
approved these changes
Oct 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Improve the use of async batch calls to dynamo db for
get_online_features_asyncWhich issue(s) this PR fixes:
Misc