-
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
Changes from all commits
a91cfd1
dd883fd
40041e6
e03fa1a
46629d1
30d41be
9155263
0ee4be2
5943f56
e397836
0969e0a
7639022
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 |
---|---|---|
|
@@ -187,6 +187,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 = False, | ||
) -> None: ... | ||
|
||
@overload | ||
|
@@ -216,6 +217,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 = False, | ||
) -> None: ... | ||
|
||
def __init__( | ||
|
@@ -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 commentThe 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 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. 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense |
||
**_deprecated_kwargs: Any, | ||
): | ||
"""Create an agent. | ||
|
@@ -295,6 +298,7 @@ def __init__( | |
Each processor takes a list of messages and returns a modified list of messages. | ||
Processors can be sync or async and are applied in sequence. | ||
event_stream_handler: Optional handler for events from the model's streaming response and the agent's execution of tools. | ||
ignore_warning_cost: If `True`, the agent will ignore warnings about token cost. | ||
""" | ||
if model is None or defer_model_check: | ||
self._model = model | ||
|
@@ -365,6 +369,8 @@ def __init__( | |
|
||
self._event_stream_handler = event_stream_handler | ||
|
||
self._ignore_warning_cost = ignore_warning_cost | ||
|
||
self._override_deps: ContextVar[_utils.Option[AgentDepsT]] = ContextVar('_override_deps', default=None) | ||
self._override_model: ContextVar[_utils.Option[models.Model]] = ContextVar('_override_model', default=None) | ||
self._override_toolsets: ContextVar[_utils.Option[Sequence[AbstractToolset[AgentDepsT]]]] = ContextVar( | ||
|
@@ -527,6 +533,7 @@ async def main(): | |
usage=RequestUsage(input_tokens=56, output_tokens=7), | ||
model_name='gpt-4o', | ||
timestamp=datetime.datetime(...), | ||
provider_name='function', | ||
) | ||
), | ||
End(data=FinalResult(output='The capital of France is Paris.')), | ||
|
@@ -590,6 +597,7 @@ async def main(): | |
usage=usage, | ||
retries=0, | ||
run_step=0, | ||
ignore_warning_cost=self._ignore_warning_cost, | ||
) | ||
|
||
# Merge model settings in order of precedence: run > agent > model | ||
|
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 alsoprovider_name='function',