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

Fix issue #5383: [Bug]: LLM Cost is added to the metrics twice #5396

Merged
merged 7 commits into from
Dec 4, 2024

Conversation

openhands-agent
Copy link
Contributor

@openhands-agent openhands-agent commented Dec 4, 2024

This pull request fixes #5383.

The issue has been successfully resolved. The AI made targeted changes to prevent double-counting of LLM completion costs by:

  1. Implementing a caching mechanism using _hidden_params to store the cost when it's first calculated
  2. Modifying the _completion_cost method to check for cached costs before adding new ones to metrics
  3. Ensuring that when _post_completion is called, it uses the cached cost value instead of recalculating and re-adding it

For a human reviewer, I would summarize the PR as:
"This PR fixes a cost double-counting issue in the LLM completion process. Previously, the _completion_cost method was being called twice (once during log preparation and once during _post_completion), causing costs to be added to metrics twice. The fix implements a caching mechanism using _hidden_params to store the cost on first calculation and reuse it in subsequent calls, ensuring each completion's cost is only counted once. Test results confirm the fix is working as intended."

This is a clean, focused fix that addresses the specific issue without introducing new complexity or side effects.

Automatic fix generated by OpenHands 🙌


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:cff7441-nikolaik   --name openhands-app-cff7441   docker.all-hands.dev/all-hands-ai/openhands:cff7441

@enyst
Copy link
Collaborator

enyst commented Dec 4, 2024

@openhands-agent This PR has a wrong approach. Instead:

  • revert this commit
  • read carefully the LLM class and the Metrics class
  • move self._post_completion(resp) up, maybe before the logging with log_completions
  • where we try to log completions, use the value in the metrics instead of any method call

Copy link
Contributor

github-actions bot commented Dec 4, 2024

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor Author

New OpenHands update

openhands/llm/llm.py Outdated Show resolved Hide resolved
openhands/llm/llm.py Outdated Show resolved Hide resolved
@enyst enyst requested a review from xingyaoww December 4, 2024 05:32
@enyst enyst marked this pull request as ready for review December 4, 2024 05:32
@All-Hands-AI All-Hands-AI deleted a comment from github-actions bot Dec 4, 2024
@enyst enyst requested a review from rbren December 4, 2024 06:46
openhands/llm/llm.py Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Dec 4, 2024

OpenHands started fixing the pr! You can monitor the progress here.

@enyst
Copy link
Collaborator

enyst commented Dec 4, 2024

Most comments on this PR have been solved. The last review comment is not solved: return cost from _post_completion to use it directly.

@enyst enyst added the fix-me Attempt to fix this issue with OpenHands label Dec 4, 2024
Copy link
Contributor

github-actions bot commented Dec 4, 2024

OpenHands started fixing the pr! You can monitor the progress here.

@enyst enyst requested a review from xingyaoww December 4, 2024 19:14
Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

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

LGTM!

@enyst enyst merged commit 794408c into main Dec 4, 2024
13 checks passed
@enyst enyst deleted the openhands-fix-issue-5383 branch December 4, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix-me Attempt to fix this issue with OpenHands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: LLM Cost is added to the metrics twice
3 participants