Repository navigation
[RunInference] Reject use_model_manager with a multi-model KeyedModelHandler - #40574
schizophrenicmaniac wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40574 +/- ##
=============================================
+ Coverage 56.16% 79.93% +23.76%
=============================================
Files 1124 488 -636
Lines 178336 82353 -95983
Branches 1489 0 -1489
=============================================
- Hits 100170 65829 -34341
+ Misses 75645 16524 -59121
+ 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: @jrmccluskey 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). |
|
Looping in Valentyn who has more context on this bug |
|
R: @AMOOOMA mind taking a look? Thanks! |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
@schizophrenicmaniac could you resolve the conflicts please? thanks! |
When a KeyedModelHandler is built from a list of KeyModelMappings, load_model() returns a _ModelHandlerManager. That object then loads and caches the per-key models itself, using its own count-based LRU. With use_model_manager=True, ModelManager only sees that wrapper, so the real models skip its memory tracking and eviction, and KeyModelPathMapping side-input updates don't work either.
This PR takes the validation route from the issue. RunInference now raises a ValueError at construction time when use_model_manager=True is used with a multi-model KeyedModelHandler, including when it's wrapped by with_preprocess_fn, with_postprocess_fn or with_no_batching. The error suggests max_models_per_worker_hint instead. Single-model KeyedModelHandlers are unaffected. I also documented use_model_manager and model_manager_args in the RunInference docstring and added a note to KeyedModelHandler.
Added a test in base_test.py that covers the plain and wrapped cases, plus the combinations that should still be allowed.
Fixes #40539