[Serving] Add ModelRunnerSelector to route models and outlets #9114
Conversation
| if isinstance(event, dict): | ||
| return event.get("error") is not None | ||
| else: | ||
| for model in event: |
There was a problem hiding this comment.
Bug: The return statement is inside the loop, so it returns on the first model checked regardless of result.
Example: If first model has no error but second model has error → returns False incorrectly.
Suggested fix:
else:
for model in event:
body_by_model = event.get(model)
if isinstance(body_by_model, dict) and "error" in body_by_model:
return True
return FalseThere was a problem hiding this comment.
It is the way I need it to be :)
There was a problem hiding this comment.
Maybe then something like this?
def _is_error(self, event: dict) -> bool:
if len(self.runnables) == 1:
return isinstance(event, dict) and event.get("error") is not None
if not event:
return False
body_by_model = next(iter(event.values()))
return isinstance(body_by_model, dict) and "error" in body_by_model
The 'for' loop is really confusing
There was a problem hiding this comment.
@alxtkr77 the error can come from a single model in the end of the list, we want the error raiser step will handle it and 1 is enough
There was a problem hiding this comment.
But you never get to the end of the list - you unconditionally return on the first iteration
There was a problem hiding this comment.
"error" field is in the body only if error was raised. else the body is the user model answer, there might be an issue if a user adding "error" to his body first layer
There was a problem hiding this comment.
@alxtkr77 sorry missed this one.. will fix in follow up pr
Test Coverage GapsThe happy path is well covered, but error handling and edge cases are missing:
Suggested additions: def test_model_runner_error_routing():
"""Test that errors route to error_raise outlet"""
...
def test_model_runner_selector_select_models():
"""Test select_models() method of new ModelRunnerSelector"""
class MyModelsSelector(ModelRunnerSelector):
def select_models(self, event, available_models):
return [m for m in available_models if m.name in event.get("models", [])]
...
def test_is_error_multiple_models():
"""Test _is_error detects error in any model, not just first"""
... |
royischoss
left a comment
royischoss
left a comment
There was a problem hiding this comment.
LGTM small comment and what we discussed
liranbg
changed the title
3 participants
📝 Description
Add user ability to select the outlets after the model execution - relevant to agents (tool selection).
🛠️ Changes Made
ModelRunnerSelector- new user interface that have 2 method one for models selection and one for outlets selection.ModelRunnerErrorRaiseris a separate branch that comes after the MRS step and not pull in to the graph.ModelSelectorclass andmodel_selectorandmodel_selector_parametersparams ofMRS.__init__✅ Checklist
🧪 Testing
Add unit tests include mm tests
🔗 References
🚨 Breaking Changes?
🔍️ Additional Notes