-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add model_name, agent_name, and response_id to RequestUsage for better tracking #2114
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
base: main
Are you sure you want to change the base?
Conversation
| """The base interface for calling an LLM.""" | ||
|
|
||
| # The model name. Subclasses can set this in __init__. | ||
| model: str = "" |
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.
in fact, it seems available models have self.model but this could be potentially a breaking change for some custom models. it will require a minor version upgrade and release note annoucements
src/agents/usage.py
Outdated
| def add( | ||
| self, | ||
| other: Usage, | ||
| model_name: str, |
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.
these properties are in the request usage data, so i don't think these data should be passed this way. at least, we should avoid passing these in order to keep flexibility of future enhancement (meaning having additional-properties argument, which can accept more properties in the future)
|
@seratch Thanks for the code review. I’ve updated the PR so that all of these fields go into a single Regarding Are you concerned about custom models that don’t inherit from If we want to avoid touching the Model class to fully eliminate this risk, I can switch to a runtime check when calling add(), something like: if hasattr(model, "model") and model.model:
metadata["model_name"] = model.modelI can update the PR if you prefer this approach. Just let me know. Thanks! |
Resolved: #2100
This PR adds three useful fields to
RequestUsage:model_name,agent_name, andresponse_id.These fields make it easier to:
Note
model_nameis taken from theModel#modelattribute. All real subclasses ofModel(such asOpenAIResponsesModel,OpenAIChatCompletionsModel, andLitellmModel) do initialize amodelattribute.However, the parent
Modelclass does not define this field, which leads to type-checking errors. So this PR adds the attribute to the base class. To avoid breaking customModelimplementations (for example,FakeModelin tests that do not set a model name), the default value is an blank string.