Add ability to run per key inference #27857
Conversation
Codecov Report
@@ Coverage Diff @@
## master #27857 +/- ##
==========================================
+ Coverage 70.89% 70.92% +0.03%
==========================================
Files 861 861
Lines 105040 105053 +13
==========================================
+ Hits 74463 74511 +48
+ Misses 29015 28980 -35
Partials 1562 1562
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @AnandInguva for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
assign to next reviewer |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @tvalentyn for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
@tvalentyn FYI, here's an integration test built on top of this PR - #27880 I'm keeping it separate for now to keep this one easier to review |
| # Use a dataclass instead of named tuple because NamedTuples and generics don't | ||
| # mix well across the board for all versions: | ||
| # https://github.com/python/typing/issues/653 | ||
| class KeyMhMapping(Generic[KeyT, ExampleT, PredictionT, ModelT]): |
There was a problem hiding this comment.
Other possible names: KeyModelEntry, KeyModelMapping, KeyMH
Also please add a docstring for this class.
There was a problem hiding this comment.
I added a docstring, I don't think anything that doesn't have a ModelHandler reference is right though since they're mapping keys to model handlers
There was a problem hiding this comment.
I agree it's less precise; but ultimately users care about the key->model mapping, and MH is an intermediary. I was trying to come up with a more readable and speakable name , esp since it's public api; also it's more common in python to cap all letters in the acronym (e.g. HTTP vs Http, but i i'd rather have MhM than MHM in this case i think...). i don't have a better name though. Maybe others have ideas.
There was a problem hiding this comment.
That's what I ended up with in the design doc so I think I'll stick with it for now. If we come up with something better we do have 6 weeks before the next cut
| def __init__( | ||
| self, | ||
| unkeyed: Union[ModelHandler[ExampleT, PredictionT, ModelT], | ||
| List[KeyMhMapping[KeyT, ExampleT, PredictionT, ModelT]]]): |
There was a problem hiding this comment.
KeyToModelMapping? or KeyModelMapping?
This builds on top of #27603 to add support for running inference against a cohort of different models using the KeyedModelHandler. Full design is here - https://docs.google.com/document/d/1kj3FyWRbJu1KhViX07Z0Gk0MU0842jhYRhI-DMhhcv4/edit#heading=h.wskna8eurvjv
This piece adds basic support. In future prs we will need to cover:
Given these gaps, I don't think we should merge before the release cut on Wednesday, but we can get this in after and iterate with small PRs
Part of #27628
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.