All-Hands-AI
[Not finished][Do not review]
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
@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 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.
- 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.
- Backend -> Pass throught the accumulate cost
- Frontend -> Add 1 additonal line to show accumulate cost
Do you think this is an acceptable way to do the breakdown?
- Backend -> Pass throught the accumulate cost
Definitely, and I think this one is conceptually first, we need the data, right?
- Frontend -> Add 1 additonal line to show accumulate cost
Can you please help me understand, where will that line be?
- Backend -> Pass throught the accumulate cost
Definitely, and I think this one is conceptually first, we need the data, right?
- 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.
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.)
Hi, @enyst I've finished the backend PR, could you please help review it? https://github.com/All-Hands-AI/OpenHands/pull/7082
Notice: Use tilin/testE2E in my repo for test if you want to verify e2e working status. It has implemented an alert for validating the simplest e2e feature. Don't use this draft PR.
@AutoLTX just to see if we can move this PR along, are you waiting for a review or still working on this?
Hi, @mamoodi Finally I use 2 seperate PR for the feature: Backend: https://github.com/All-Hands-AI/OpenHands/pull/7082 Frontend: https://github.com/All-Hands-AI/OpenHands/pull/7099 This is only a draft with initial discussion and I won't change it anymore.