-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add cost
to RunUsage
#2684
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?
Add cost
to RunUsage
#2684
Conversation
Docs Preview
|
# If we can't calculate the price, we don't want to fail the run. | ||
with suppress(LookupError): | ||
ctx.state.usage.cost += calc_price( | ||
model_response.usage, |
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.
Can't you use ModelResponse.price()?
Ah, I need to add the warning, and the param in the agent. I forgot that. |
@@ -243,6 +245,7 @@ def __init__( | |||
instrument: InstrumentationSettings | bool | None = None, | |||
history_processors: Sequence[HistoryProcessor[AgentDepsT]] | None = None, | |||
event_stream_handler: EventStreamHandler[AgentDepsT] | None = None, | |||
ignore_warning_cost: bool = True, |
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.
I changed this to True to make tests pass easily, since many things were failing where we didn't know the price.
I think that Agent.run
itself shouldn't create a warning by default, but it should note when a price is unknown. The warning should be emitted when the user explicitly requests the cost of a run.
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.
I left it false because I think it's the right behavior, but also to help me understand what we didn't support yet.
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.
Is the warning useful if it's opt-in to actually have it?
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.
the parameter wouldn't be here, it would always collect and store the information about future warnings during the run. there would be a parameter on some other method for retrieving the cost, where the warning is emitted by default.
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.
Makes sense
# NOTE(Marcelo): We can allow some kind of hook on the provider level, which we could retrieve via | ||
# `ctx.deps.model.provider.calculate_cost`, but I'm not sure how would the API look like. Maybe a new parameter | ||
# on the `Provider` classes, that parameter would be a callable that receives the same parameters as `genai_prices`. | ||
if response.model_name not in ('test', 'function') and not ignore_warning_cost: |
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.
I was thinking that test/function provider names would somehow make this special casing easier, I didn't even think about the model names. Since we can just use this, maybe the provider names aren't worth it. I especially don't like how the examples and docs now show model_name='gpt-4o',
but also provider_name='function',
No description provided.