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.
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
[Data] Async batch fetching for
map_batches
#31576[Data] Async batch fetching for
map_batches
#31576Changes from 3 commits
326376b
58d592f
8f53df8
2952c58
adb942a
b761421
ddf78b8
61be086
6859012
fe88520
973fe12
e37e46b
3cf414b
9273b01
bc3b39c
a5c9184
1c18dd7
1e1a49a
0dcd525
adc206e
2424de4
346bb49
ef60f11
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is a necessary change needed to get the performance improvements for batch prediction.
Before, we would only bundle blocks up to batch size and submit each bundle as a separate actor task. This means we cannot do prefetching when batch size is greater than block size since each bundle is a separate task.
Instead, if the max actor pool size is set, then we bundle up to min(batch size, max actor pool size).
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.
Hopefully once we switch to a fully iterator-based implementation, these type of special cases are no longer necessary.
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.
This code will become deprecated with new executor backend, cc @clarkzinzow.
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.
Just to make sure that I understand the motivation, this is giving us stratified chunking, where a given chunk consists of an equal number of blocks from each of the original bundles (modulo the number of chunks), right? Might be worth leaving a comment as much for those that are less familiar with this pattern.
Two potential issues with this chunking scheme:
bundles = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
(pretend the numbers are block IDs) andnum_chunks = 2
, and suppose that blocks with odd IDs are much larger than blocks with even IDs; this chunking would produce bundles[[1, 3, 5, 7, 9], [2, 4, 6, 8]]
, where the first bundle is way, way larger than the second bundle.Could solve (1) by changing the rechunking to merge adjacent chunks without breaking ordering, but (2) would require rebundling while taking the block sizes into account. I think that (1) is probably a blocker but (2) is not, cc @matthewdeng @c21 for more opinions.
If we are only wanting to solve (1) for now, we could do the simple thing of merging adjacent bundles until we either (1) are at the specified number of chunks (max pool size), or (2) all would-be merged bundles exceed the max target block size threshold (currently 512 MiB by default).
Could do something like the following progressive merging of adjacent bundles, which should preserve block/row order:
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.
When porting this to the new executor, we should try to consolidate
prefetch_batches
andprefetch_blocks
into a singleprefetch_batches
argument, where we always prefetch enough blocks to satisfyprefetch_batches
, which should be simple enough to implement since we have the size for each to-be-fetched block on hand.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.
yep +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.
shall we add a test for
map_batches
as well?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 tried it but there's too much time variance for a deterministic small-scale map_batches CI test. I'll confirm the performance improvements via running the batch inference release tests.