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

[Not finished][Do not review] #6823

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

AutoLTX
Copy link

@AutoLTX AutoLTX commented Feb 19, 2025

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

Give a summary of what the PR does, explaining any non-trivial design decisions


Link of any specific issues this addresses


# Add local metrics to observation
if self.state and self.state.local_metrics:
observation.llm_metrics = copy.deepcopy(self.state.local_metrics)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we added it, instead of observation, to the action? Somewhere

  • after we got it from the agent at line 689
action = self.agent.step(self.state)
  • and just before it's added to the event stream, like before this line 751
self.event_stream.add_event(action, action._source)  # type: ignore [attr-defined]

Then it would be saved in the stream, so the server would be able to read it in its dict it sends to frontend. 🤔

I think we have some difficult problem atm with sending extra bits of information to the frontend, I'm sorry about that. This idea might be one way to do it, and it should work for our purpose.

Copy link
Author

Choose a reason for hiding this comment

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

🤗I tried to hard-code llm_metrics in session.py before sending the "oh_event" response in send, and it successfully appeared on the frontend. Then, I went back to check where preparing the related event and found that adding llm_metrics in actions with EventSource.AGENT allows the content to be successfully pushed into the event stream.

As a result, all subscribers, including EventStreamSubscriber.SERVER in session.py, can fetch it. Ultimately, it gets wrapped in the "oh_event" and sent to the frontend.

I guess I understand the full logic now.Including your advice's backend reason

🤔But initially when I tried to add the code following your advice to test, it doesn't work. Let me check again and continue the debugging process. Thanks for your kindness. I have idea now I think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think eventstream.add_event saves the event, with everything in it, including _llm_metrics if we set it on the event. So the event gets transmitted to subscribers, including SERVER, with its contents.

@enyst
Copy link
Collaborator

enyst commented Feb 22, 2025

@AutoLTX Thank you for the work on this!

Please don't worry about the fact that it's unfinished, it's fine, that's why it's a draft. Everyone does temporary stuff in a draft.

This PR ends up much more complex than we first thought, I apologize about that! We seem to have two difficulties here:

  • (backend) sending extra information to the frontend is non-obvious. There is no channel other than events themselves, and an error callback channel, I think.
  • (frontend) frontend just had or has a redesign, and frontend folks will need to check changes so that we don't introduce inconsistencies or issues with the rest.

What do you say if we split this PR in two, maybe three:

  • backend stuff, see that the info is actually sent; unit tests
  • frontend - display accumulated cost
  • frontend - page with details on cost

That may make it easier to keep in sync with the high activity here, and easier to review and approve.

@AutoLTX
Copy link
Author

AutoLTX commented Feb 25, 2025

@AutoLTX Thank you for the work on this!

Please don't worry about the fact that it's unfinished, it's fine, that's why it's a draft. Everyone does temporary stuff in a draft.

This PR ends up much more complex than we first thought, I apologize about that! We seem to have two difficulties here:

  • (backend) sending extra information to the frontend is non-obvious. There is no channel other than events themselves, and an error callback channel, I think.
  • (frontend) frontend just had or has a redesign, and frontend folks will need to check changes so that we don't introduce inconsistencies or issues with the rest.

What do you say if we split this PR in two, maybe three:

  • backend stuff, see that the info is actually sent; unit tests
  • frontend - display accumulated cost
  • frontend - page with details on cost

That may make it easier to keep in sync with the high activity here, and easier to review and approve.

@enyst Thanks for the advice! I'd like to breakdown the PR into 3 parts like following for quick review. This can also avoid solving conflict many times.

  1. Frontend -> Expose usage first and show it in details page, and adding a button to choose whether to display cost related info. As usage is already contained in event, we don't need to change backend side. This can make sure reviewers can see all the UI experience of Display cost.
  2. Backend -> Pass throught the accumulate cost
  3. Frontend -> Add 1 additonal line to show accumulate cost

Do you think this is an acceptable way to do the breakdown?

@enyst
Copy link
Collaborator

enyst commented Feb 25, 2025

  1. Backend -> Pass throught the accumulate cost

Definitely, and I think this one is conceptually first, we need the data, right?

  1. Frontend -> Add 1 additonal line to show accumulate cost

Can you please help me understand, where will that line be?

@AutoLTX
Copy link
Author

AutoLTX commented Feb 25, 2025

  1. Backend -> Pass throught the accumulate cost

Definitely, and I think this one is conceptually first, we need the data, right?

  1. Frontend -> Add 1 additonal line to show accumulate cost

Can you please help me understand, where will that line be?

Yes we need the data. It's a little bit hard to describe it in words. Hmm.. Maybe the layout is similar to this in detailed page.? Use 4 single lines to show numbers.
image

BTW, it's ok for me to do the backend side first. I propose the idea initially just because I was blocked by understanding the full logic of backend side. Now I haven't checked in code in this branch but I think I'm unblocked.(I modify the response in the code and adding the logic I understand.)

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.

2 participants