Repository navigation
Conversation
Add ModelHandler.set_model_identifier(), which RunInference calls on each worker before the first load_model() with the step's model_identifier (the value passed to RunInference, or the uuid it generates). Handlers that publish num_loaded_models_<identifier> themselves, such as handlers that front an out-of-process model server, can then use the exact tag the step is annotated with. The value is stable across side-input model updates, matching the metric tag ModelManager uses since apache#40714. The hook is a no-op by default and is forwarded by the keyed, maybe-keyed, prebatched, preprocessing and postprocessing wrappers.
Contributor
Author
|
R: @damccorm |
Contributor
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40906 +/- ##
==========================================
- Coverage 79.83% 79.82% -0.02%
==========================================
Files 494 494
Lines 83575 83597 +22
==========================================
+ Hits 66722 66731 +9
- Misses 16853 16866 +13
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #40760, following review feedback to separate the generic pieces from the vLLM-specific ones. This PR is a small generic RunInference hook.
Why
Runners that autoscale on
num_loaded_models_<model_identifier>(Dataflow's GPU autoscaling) look the metric up by the identifier RunInference annotates the inference step with: themodel_identifierpassed toRunInference, or a uuid it generates.ModelManagerpublishes under that identifier (and since #40714 keeps it stable across side-input model updates). But a model handler that publishes the metric itself, e.g. one that fronts an out-of-process model server, has no way to learn that value.What
ModelHandler.set_model_identifier(model_identifier): a no-op by default._RunInferenceDoFn.setup()calls it on each worker, before the firstload_model(), with the same tag it annotates the step with and givesModelManager.KeyedModelHandlerforwards to every per-key handler).No behavior change for existing handlers.
Testing
base_test.py:model_identifierreaches the handler beforeload_model().model_identifierannotation onRunInference/BeamML_RunInference.KeyedModelHandlerand pre/post-processing wrappers.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123orfixes #456).CHANGES.mdwith noteworthy changes.