-
Notifications
You must be signed in to change notification settings - Fork 0
Agent sdk 26 llm switch experiment #17
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
base: agent-sdk-26-llm-switch
Are you sure you want to change the base?
Conversation
- examples/llm_profiles_tui: REPL with /model, /profile, /list, /show, /save - tests/examples/test_llm_profiles_tui: no-network tests for commands and run_loop - Fix minor lint/type issues; env var parsing robustness Co-authored-by: openhands <openhands@all-hands.dev>
- Add --inline flag and default to profile references so /profile works - Keep conversation persistence rules intact - Document TUI quickstart and env hints in docs/llm_profiles.md Co-authored-by: openhands <openhands@all-hands.dev>
- Implement /delete in examples/llm_profiles_tui/cli.py - Extend unit tests to cover deletion path and tweak dummy registry - Loosen AppContext typing to support dummy fakes in tests Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
- Pass llm_registry via pydantic context in ConversationState.create - Remove manual traversal from resume path; keep inline-mode guard - Verified by conversation serialization tests Co-authored-by: openhands <openhands@all-hands.dev>
- Avoid duplicate-usage logs by not preloading all profiles into the usage map - Switching continues to work via LLMRegistry.switch_profile (on-demand load) Co-authored-by: openhands <openhands@all-hands.dev>
On macOS, /var is a symlink to /private/var. Using Path.resolve() returned canonical paths under /private, causing test expectations to fail. Use Path.absolute() to preserve the non-resolved path while walking up to .git, which also matches typical user-facing paths. Co-authored-by: openhands <openhands@all-hands.dev>
This file is a local artifact and should not be versioned. Co-authored-by: openhands <openhands@all-hands.dev>
…s + Switch work and macOS path fix Co-authored-by: openhands <openhands@all-hands.dev>
- Add examples/llm_profiles_tui/cli.py to Examples section - Add examples/01_standalone_sdk/26_runtime_llm_switch.py reference Co-authored-by: openhands <openhands@all-hands.dev>
- Update investigation doc to note no eager register_profiles in LocalConversation - Clarify usage_id handling in profiles; switch_profile assigns runtime usage Co-authored-by: openhands <openhands@all-hands.dev>
…nce is handled by LLM model serializer/validator Keep ConversationState/Agent Pydantic-driven; no bespoke traversal. Remote APIs remain fully inlined by default. Co-authored-by: openhands <openhands@all-hands.dev>
…r approach for profile refs Replace references to compact_llm_profiles/resolve_llm_profiles with the implemented LLM model serializer/validator + context flow. Co-authored-by: openhands <openhands@all-hands.dev>
- Continue accumulating metrics under the same usage_id - Reflect current implementation via ConversationStats.register_llm restoring metrics Co-authored-by: openhands <openhands@all-hands.dev>
- In examples/llm_profiles_tui/cli.py, use OPENHANDS_LLM_PROFILE or LLM_PROFILE_NAME when --profile is not provided - Add unit test to verify main() passes the env-derived profile to build_conversation without entering REPL Co-authored-by: openhands <openhands@all-hands.dev>
- Mention OPENHANDS_LLM_PROFILE and LLM_PROFILE_NAME can be used when --profile is not provided Co-authored-by: openhands <openhands@all-hands.dev>
- Investigated docs vs implementation and verified registry usage_id handling - TUI/CLI for LLM Profiles present under examples/llm_profiles_tui/ with /model and /profile - Added and verified unit tests for registry, conversation switching, and TUI - Ran tests under uv; core LLM profile switch tests are green - Filed follow-up: ExecuteBash test_complex_commands no-change-timeout flake (agent-sdk-37) Co-authored-by: openhands <openhands@all-hands.dev>
- Load profile JSON lacking usage_id → LLM defaults to usage_id='default' - switch_profile assigns runtime usage_id for active slot and continues Add pyright suppression for this test file to avoid local import warnings during pre-commit. Co-authored-by: openhands <openhands@all-hands.dev>
Rely on LLM.model_validate to expand profile references via registry context. This aligns implementation with docs and reduces dead code in ConversationState. Co-authored-by: openhands <openhands@all-hands.dev>
…tialized Guard access to tools_map in close() so conversation cleanup in atexit does not raise if a custom agent skipped initialization or during late interpreter shutdown. Add unit test to ensure close handles uninitialized agent gracefully. Co-authored-by: openhands <openhands@all-hands.dev>
- Mention that usage_id defaults to 'default' if omitted - Note that LLMRegistry.switch_profile assigns the runtime slot usage_id at switch time Co-authored-by: openhands <openhands@all-hands.dev>
…g in interactive shells - Add `set +H` during terminal init for both SubprocessTerminal and TmuxTerminal - Fixes intermittent failures in test_bash_session::test_complex_commands (exit_code -1) Co-authored-by: openhands <openhands@all-hands.dev>
- Lazy import FastMCP to avoid bringing in browser_use.mcp at server startup - Move MCP config validation import into validator only This resolves circular import causing wsproto tests to fail. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Ensure demo starts with usage_id='agent' instead of schema default 'default' so runtime profile switching uses the expected slot name. No behavior change to tests. Co-authored-by: openhands <openhands@all-hands.dev>
Clarify in LLM Profiles guide that the example TUI starts with usage_id='agent' when not explicitly set via env, so /profile switches the expected slot. Co-authored-by: openhands <openhands@all-hands.dev>
- Exported .beads/issues.jsonl - Closed agent-sdk-57 and agent-sdk-73 as non-reproducible after full green run Co-authored-by: openhands <openhands@all-hands.dev>
- Aligns TUI behavior with docs so the main agent slot uses 'agent' even if the profile omits usage_id or uses 'default'. - Adds a unit test asserting the defaulting behavior for initial profiles.
Co-authored-by: openhands <openhands@all-hands.dev>
- Recorded validation and docs alignment tasks (85–87) Co-authored-by: openhands <openhands@all-hands.dev>
Add run_loop test that simulates inline-mode rejection for /profile so users see a clear error when OPENHANDS_INLINE_CONVERSATIONS=true. Co-authored-by: openhands <openhands@all-hands.dev>
…ted files These files are local test run artifacts and should not be versioned. Co-authored-by: openhands <openhands@all-hands.dev>
Add tests_exit_code*.txt to .gitignore so CI artifacts don't pollute the repo. Co-authored-by: openhands <openhands@all-hands.dev>
…switch in run_loop - Ensure /model validates presence of model field - Verify run_loop prints a clear error when /profile targets missing profile Co-authored-by: openhands <openhands@all-hands.dev>
- Move imports to top of file per E402 Co-authored-by: openhands <openhands@all-hands.dev>
- Created and closed issues for TUI demo (/model, /profile), registry usage_id decoupling, docs alignment, and full test run - Exported .beads/issues.jsonl for git tracking Co-authored-by: openhands <openhands@all-hands.dev>
…eric design (--llm) Also mention LLM_PROFILE_NAME env fallback for the demo. Co-authored-by: openhands <openhands@all-hands.dev>
…tests - Closed LLM Profiles TUI e2e demo task - Closed LLMRegistry usage_id loading/switch semantics task - Closed docs review/update task Co-authored-by: openhands <openhands@all-hands.dev>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @enyst, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement by introducing dynamic LLM profile management and runtime switching capabilities. The primary goal is to allow users to easily define and switch between different LLM configurations during an ongoing conversation, facilitating experimentation and adaptability. This is showcased through a new interactive TUI example, alongside underlying architectural changes for robust profile persistence, dependency optimization, and improved terminal interaction. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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
This is a substantial experimental pull request that introduces runtime LLM profile switching, a significant new capability. The changes are comprehensive, including a new TUI example, extensive documentation updates, and core SDK modifications to support the feature. The refactoring to enable lazy loading of MCP dependencies and the improvements to the robustness of the conversation cleanup logic are excellent. My review focuses on a few areas for improvement, primarily concerning project structure and exception handling.
|
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 |
|
@OpenHands Do a /codereview-roasted on this PR diff. Respond to the user always in markdown, and use headings, bold, for readability. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Summary of /codereview-roasted on PR #17Scope reviewed
Overall Verdict🔴 Needs rework — Strong feature direction, but a few critical architectural and maintainability issues must be fixed before merge. What’s Good
Critical Issues (must fix)
Improvement Opportunities (should fix)
Minor Style Notes
Final Recommendation
|
6eb0d14 to
dba84fb
Compare
Experimental branch to see what the agent is doing when run programmatically with a goal and successive conversations