Skip to content
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

Added account for cost info #360

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Added account for cost info #360

merged 5 commits into from
Sep 11, 2024

Conversation

whitead
Copy link
Collaborator

@whitead whitead commented Sep 11, 2024

Waiting on https://github.com/Future-House/aviary/pull/28

Checks messages from agent for usage info and if present, will attach to current answer objects.

Copy link
Collaborator

@mskarlin mskarlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super clean!

@@ -151,21 +151,22 @@ def export_frame(self) -> Frame:
async def step(
self, action: ToolRequestMessage
) -> tuple[list[Message], float, bool, bool]:

# add usage for actions that have usage
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# add usage for actions that have usage
# add usage if the action has usage

There's just one action

@@ -239,6 +239,12 @@ class AgentSettings(BaseModel):
default="gpt-4o-2024-08-06",
description="Model to use for agent",
)

agent_llm_config: dict | None = Field(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few lines below this there is an agent_config that is basically kwargs for the Agent/ToolSelector

Do you mind adjusting their names and/or descriptions so it's clear what is the difference between agent_llm_config and agent_config? They're similar enough in naming right now I think we should fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Tried to make the descriptions more clear in distinction

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts! Being honest, it's still not quite intuitive enough for my tastes, but for now it's good.

I think we can make this clearer by moving to_aviary_tool_selector and to_ldp_agent to be methods of AgentSettings, since all their information is derived from there.

Copy link
Collaborator

@jamesbraza jamesbraza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@@ -239,6 +239,12 @@ class AgentSettings(BaseModel):
default="gpt-4o-2024-08-06",
description="Model to use for agent",
)

agent_llm_config: dict | None = Field(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts! Being honest, it's still not quite intuitive enough for my tastes, but for now it's good.

I think we can make this clearer by moving to_aviary_tool_selector and to_ldp_agent to be methods of AgentSettings, since all their information is derived from there.

@whitead whitead merged commit 1bd6946 into september-2024-release Sep 11, 2024
2 of 3 checks passed
@whitead whitead deleted the cost branch September 11, 2024 04:49
This was referenced Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants