-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Microbatch parallelism #10958
Microbatch parallelism #10958
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10958 +/- ##
==========================================
- Coverage 89.13% 89.10% -0.04%
==========================================
Files 183 183
Lines 23646 23760 +114
==========================================
+ Hits 21078 21171 +93
- Misses 2568 2589 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
|
… will need to be re-written with new structure
…crobatch handling, TestMicrobatchWithInputWithoutEventTime failing
…batch execution We had a unit test. However that unit test broke upon our work to parallelize things. The parallelization work made it _possible_ for this test to be an integration test, which is actually what we always wanted for this functionality. So we removed the broken unit test, and added a new integration test :)
…this` to determine batch parallelism Microbatch batches can be run in parallel, but won't _always_ be run in parallel. There are 3 general cases 1. The batch's model relation doesn't exist -> It can't be run in parallel 2. The relation exists and the model has a `this` invocation -> Assume it can't be run in parallel 3. The relation exists and the model _doesn't_ have a `this` invocation -> Assume it can be run in parallel However there are some exceptions: a. the `this` reference is actually safe (maybe the needed `this` data is guaranteed to exist) b. the batches are small enough (data wise) that it's actually faster to run sequentally Because of (a) and (b) we needed a way to escape out of (1) and (2). Thus we added the `parallel_batches` config. This config defaults to `None`. If it is set though, its value takes precedence over the presence of `this`.
…ity" This reverts commit 8bf1504.
@cla-bot check |
The cla-bot is refusing to show up 🤔 |
@cla-bot check |
@cla-bot check |
Merging and bypassing branch protection for |
Resolves #10853
Resolves #10855
Problem
Part of the benefit of microbatch, is updating a model in smaller (micro) batches. However, before the work in this PR, those batches were run sequentially. Having them run sequentially didn't negate all benefits of splitting into batches (partial instead of complete failures, better data guarantees, easier backfills, etc), however because they were running serially there weren't any speed gains. We wanted the ability to run batches in parallel. This PR does that.
Solution
First we pulled all the batch execution logic into a new
MicrobatchModelRunner
, which is called from theModelRunner
when a microbatch model is encountered. Handwaving all the complexities of creating a new Runner class, the class determines whether things can be run in parallel in the following.First, if any of the following are true, a batch is run sequentially
If neither (1) nor (2) was hit, then we check if a config
concurrent_batches
is set for the model. If the value for that config isTrue
then we run the batches in parallel, ifFalse
we run the batches sequentially.If however
concurrent_batches
isNone
(i.e. not set), then we check if the model jinja contains a reference tothis
. If it referencesthis
then we run the batches sequentially. Otherwise, we run them in parallel.Checklist