-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setup model routing config and plan routing to o1 #6189
base: main
Are you sure you want to change the base?
Changes from all commits
cec10a1
fd1b4ec
c33ba45
7b08724
910ba8c
b73f3ec
54d4401
06db2d6
b5973cd
e3c8a9e
27a83db
6f86ad9
ec2d162
9bf5a7f
8e05f3f
ddc3248
472d95c
fc5040c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
from dataclasses import dataclass, fields | ||
|
||
from openhands.core.config.config_utils import get_field_info | ||
|
||
|
||
@dataclass | ||
class ModelRoutingConfig: | ||
reasoning_model: str = 'o1-preview-2024-09-12' | ||
|
||
def defaults_to_dict(self) -> dict: | ||
"""Serialize fields to a dict for the frontend, including type hints, defaults, and whether it's optional.""" | ||
dict = {} | ||
for f in fields(self): | ||
dict[f.name] = get_field_info(f) | ||
return dict | ||
|
||
def __str__(self): | ||
attr_str = [] | ||
for f in fields(self): | ||
attr_name = f.name | ||
attr_value = getattr(self, f.name) | ||
|
||
attr_str.append(f'{attr_name}={repr(attr_value)}') | ||
|
||
return f"ModelRoutingConfig({', '.join(attr_str)})" | ||
|
||
@classmethod | ||
def from_dict(cls, model_routing_config_dict: dict) -> 'ModelRoutingConfig': | ||
return cls(**model_routing_config_dict) | ||
|
||
def __repr__(self): | ||
return self.__str__() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
|
||
import requests | ||
|
||
from openhands.core.config import LLMConfig | ||
from openhands.core.config import LLMConfig, ModelRoutingConfig | ||
|
||
with warnings.catch_warnings(): | ||
warnings.simplefilter('ignore') | ||
|
@@ -72,6 +72,7 @@ | |
'gpt-4o-mini', | ||
'gpt-4o', | ||
'o1-2024-12-17', | ||
'o1', | ||
] | ||
|
||
REASONING_EFFORT_SUPPORTED_MODELS = [ | ||
|
@@ -94,6 +95,7 @@ def __init__( | |
self, | ||
config: LLMConfig, | ||
metrics: Metrics | None = None, | ||
model_routing_config: ModelRoutingConfig | None = None, | ||
): | ||
"""Initializes the LLM. If LLMConfig is passed, its values will be the fallback. | ||
|
||
|
@@ -102,13 +104,15 @@ def __init__( | |
Args: | ||
config: The LLM configuration. | ||
metrics: The metrics to use. | ||
model_routing_config: The model routing configuration. | ||
""" | ||
self._tried_model_info = False | ||
self.metrics: Metrics = ( | ||
metrics if metrics is not None else Metrics(model_name=config.model) | ||
) | ||
self.cost_metric_supported: bool = True | ||
self.config: LLMConfig = copy.deepcopy(config) | ||
self.model_routing_config = model_routing_config | ||
|
||
self.model_info: ModelInfo | None = None | ||
|
||
|
@@ -175,6 +179,7 @@ def wrapper(*args, **kwargs): | |
|
||
messages: list[dict[str, Any]] | dict[str, Any] = [] | ||
mock_function_calling = kwargs.pop('mock_function_calling', False) | ||
use_reasoning_model = kwargs.pop('use_reasoning_model', False) | ||
|
||
# some callers might send the model and messages directly | ||
# litellm allows positional args, like completion(model, messages, **kwargs) | ||
|
@@ -207,6 +212,15 @@ def wrapper(*args, **kwargs): | |
kwargs['stop'] = STOP_WORDS | ||
mock_fncall_tools = kwargs.pop('tools') | ||
|
||
if use_reasoning_model: | ||
if self.model_routing_config is None: | ||
raise ValueError( | ||
'Model routing config is required for model routing.' | ||
) | ||
|
||
# Replace the model with the reasoning model | ||
kwargs['model'] = self.model_routing_config.reasoning_model | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could design the reasoning model not as a part of an LLM instance, but as a second LLM instance in the agent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we also need to allow user to set these, especially if they don't use via a llm proxy 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
|
||
# if we have no messages, something went very wrong | ||
if not messages: | ||
raise ValueError( | ||
|
@@ -654,7 +668,7 @@ def __str__(self): | |
return f'LLM(model={self.config.model}, api_version={self.config.api_version}, base_url={self.config.base_url})' | ||
elif self.config.base_url: | ||
return f'LLM(model={self.config.model}, base_url={self.config.base_url})' | ||
return f'LLM(model={self.config.model})' | ||
return f'LLM(model={self.config.model},reasoning_model={self.model_routing_config.reasoning_model if self.model_routing_config else None})' | ||
|
||
def __repr__(self): | ||
return str(self) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from abc import ABC, abstractmethod | ||
|
||
|
||
class BaseRouter(ABC): | ||
@abstractmethod | ||
def should_route_to_custom_model(self, prompt: str) -> bool: | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
from openhands.router.plan.llm_based import LLMBasedPlanRouter | ||
from openhands.router.plan.rule_based import RuleBasedPlanRouter | ||
|
||
__all__ = ['RuleBasedPlanRouter', 'LLMBasedPlanRouter'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import copy | ||
|
||
from openhands.core.config import LLMConfig | ||
from openhands.llm.llm import LLM | ||
from openhands.router.base import BaseRouter | ||
from openhands.router.plan.prompts import ( | ||
TRAJECTORY_JUDGE_REASONING_SYSTEM_PROMPT, | ||
TRAJECTORY_JUDGE_REASONING_USER_PROMPT, | ||
) | ||
|
||
|
||
class LLMBasedPlanRouter(BaseRouter): | ||
""" | ||
Router that routes the prompt that is judged by a LLM as complex and requires a step-by-step plan. | ||
""" | ||
|
||
JUDGE_MODEL = 'gpt-4o' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be interesting to see if we can experiment with cheaper model for that 🤔 |
||
|
||
def __init__(self, llm_config: LLMConfig): | ||
super().__init__() | ||
|
||
judge_llm_config = copy.deepcopy(llm_config) | ||
self.judge_llm = LLM(judge_llm_config) | ||
|
||
def should_route_to_custom_model(self, prompt: str) -> bool: | ||
messages = [ | ||
{ | ||
'role': 'system', | ||
'content': TRAJECTORY_JUDGE_REASONING_SYSTEM_PROMPT, | ||
}, | ||
{ | ||
'role': 'user', | ||
'content': TRAJECTORY_JUDGE_REASONING_USER_PROMPT.format( | ||
interaction_log=prompt | ||
), | ||
}, | ||
] | ||
|
||
response = self.judge_llm.completion( | ||
messages=messages, | ||
model=self.JUDGE_MODEL, | ||
) | ||
return int(response['choices'][0]['message']['content'].strip()) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is also another approach, my thought is for now we only use reasoning models specifically for model routing, so I put it in this config group (with other values in the future). When we also use them for other purposes, we can probably move to llm-specific groups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that we can reuse the way we define a model (which will implicitly take care of the correct loading and init all
base_url
etc).It doesn't say which component of
openhands
loads the definition of[llm.reasoning_model]
, it can be the routing component.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, if a user wants to use a reasoning model today, for the agent, they can do so. They just choose a reasoning model and configure it. Ability to use it isn't new?
We can just avoid to duplicate LLMConfig settings ("reasoning_model", "reasoning_model_base_url", "reasoning_model_api_key", "reasoning_model_aws...something" etc) into the new routing section, instead we can reference existing configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds good to me, thanks for the suggestion! I'll try to address this after getting the routing behavior to work