Repository navigation
Fix missing method delegations in RunInference wrapper ModelHandlers - #40602
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR properly ensures that wrapper ModelHandler classes (KeyedModelHandler, MaybeKeyedModelHandler, _PrebatchedModelHandler, _PreProcessingModelHandler, and _PostProcessingModelHandler) delegate all relevant hooks (update_model_paths, override_metrics, should_skip_batching, and should_garbage_collect_on_timeout) to their inner model handlers. It also fixes a logging format bug in KeyedModelHandler.__init__ and introduces comprehensive unit tests, including a reflection-based meta-test to prevent future delegation omissions.
🔍 General Feedback
- The delegation implementations are clean, consistent across all wrapper classes, and properly handle single-model vs. multi-model cases in
KeyedModelHandler. - Adding
test_wrapper_model_handlers_override_all_model_handler_methodsis a great safeguard against future drift when new methods are added to the baseModelHandlerclass.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40602 +/- ##
=============================================
+ Coverage 56.16% 79.92% +23.75%
=============================================
Files 1124 488 -636
Lines 178336 82416 -95920
Branches 1489 0 -1489
=============================================
- Hits 100170 65874 -34296
+ Misses 75645 16542 -59103
+ Partials 2521 0 -2521
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:
|
|
Assigning reviewers: R: @shunping 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). |
Added some of the newer hooks to all wrapper ModelHandler classes so they forward these hooks to
self._baseorself._unkeyed.Added a meta-test
test_wrapper_model_handlers_override_all_model_handler_methodsto verify that wrappers override all methods from the base ModelHandler class to prevent similar omissions.Fix a copypasting error.