Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41051 +/- ##
============================================
- Coverage 57.48% 57.46% -0.02%
Complexity 2492 2492
============================================
Files 1204 1204
Lines 171265 171181 -84
Branches 2103 2103
============================================
- Hits 98456 98375 -81
+ Misses 70157 70154 -3
Partials 2652 2652
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly deduplicates the single-base ModelHandler wrapper classes (MaybeKeyedModelHandler, _PrebatchedModelHandler, _PreProcessingModelHandler, and _PostProcessingModelHandler) by introducing a shared _DelegatingModelHandler base class. It also properly delegates set_environment_vars() across wrappers and single-model KeyedModelHandler instances, with solid unit test coverage.
🔍 General Feedback
- The
_DelegatingModelHandlerabstraction significantly reduces boilerplate (~140 net lines removed) and ensures futureModelHandlerhooks only need to be forwarded in one place. - Forwarding
set_environment_vars()and preserving_model_identifierin_DelegatingModelHandler.__init__improves consistency across wrapper chains. - Updating
_is_multi_model_keyed_handlerto unwrap any_DelegatingModelHandlermakes the check more general and coversMaybeKeyedModelHandleras well.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @tvalentyn for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Follow-up to #40906. We were duplicating a ton of logic there and in other PRs, so this is trying to simplify things.
Refactors the single-base
ModelHandlerwrapper classes (MaybeKeyedModelHandler,_PrebatchedModelHandler,_PreProcessingModelHandler, and_PostProcessingModelHandler) to inherit from a shared_DelegatingModelHandlerbase class instead of duplicating ~15 pass-through methods on each wrapper class.Also forwards
set_environment_vars()to the underlyingModelHandlerin_DelegatingModelHandler(and single-modelKeyedModelHandler) so customset_environment_vars()overrides on wrapped handlers are preserved alongside_env_vars.