-
Notifications
You must be signed in to change notification settings - Fork 140
Add example for reconstructing OpenAI messages from events #1916
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
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ PR Artifacts Cleaned Up The |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
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.
Code Review Summary
This PR adds a useful example for reconstructing OpenAI messages from persisted events. However, there are critical issues that need to be addressed:
🔴 Critical Issues
1. Runtime artifacts committed to repository
The entire .pr/event-json-to-openai-messages/ directory should NOT be committed. It contains:
- Generated event logs and state files
- API usage metadata
- Lock files
These files:
- Will bloat the repository over time
- Contain runtime-specific data that's not portable
- Are regenerated automatically when the example runs
- May contain sensitive information
Action required: Delete the .pr/event-json-to-openai-messages/ directory and add both .pr/ and .conversations/ to .gitignore
🟠 Important Issues
2. Path inconsistency (line 12)
The code sets persistence_root = Path(".conversations") but the committed artifacts are in .pr/event-json-to-openai-messages/. This suggests the example was run with different settings than what's in the code.
Recommendation: Ensure the code matches how the example should actually be run.
🟡 Suggestions
3. Missing error handling (line 88)
The event loading code Event.model_validate_json(path.read_text()) will raise an unhandled exception if any JSON file is malformed. Consider adding try/except for robustness.
4. Add .gitignore entries
Add these patterns to .gitignore:
.pr/
.conversations/
🟢 Nits
5. Comment clarity (line 21)
Consider adding a comment explaining why imports come after environment setup:
# Import after setting env vars because these modules read them at import time
from openhands.sdk import ( # noqa: E402Overall: The Python code example itself is well-structured and demonstrates the concept clearly. Once the runtime artifacts are removed and the path inconsistency is resolved, this will be a valuable addition to the examples.
xingyaoww
left a comment
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.
LGTM! Shall we add it to our OpenHands/docs site as well?
Yes! |
|
Thanks @enyst! |
HUMAN: The bot reviewer doesn’t know that
.pr/are temporary and will be cleaned up automatically.This PR proposes an example of how to convert back events to LLM-ready messages, for RL or debugging or whatever purpose. Since the SDK is developer focused, maybe it makes sense? So we can also point easily to it if we get this question again.
Slack reference
—-
Summary
Testing
Docs PR: OpenHands/docs#303
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:15eda6e-pythonRun
All tags pushed for this build
About Multi-Architecture Support
15eda6e-python) is a multi-arch manifest supporting both amd64 and arm6415eda6e-python-amd64) are also available if needed