-
Notifications
You must be signed in to change notification settings - Fork 56
Fix delegation visualization labels to show correct sender/receiver #1119
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
- Add optional 'sender' parameter to MessageEvent to track message origin - Update send_message() in BaseConversation and LocalConversation to accept sender - Modify DefaultConversationVisualizer to use sender info for accurate message titles - Update DelegateExecutor to pass formatted sender names when delegating tasks - Add helper function to format agent IDs (snake_case -> Title Case) - Add comprehensive tests for sender parameter behavior Fixes #1118 Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Fix typo: visualize -> visualizer parameter in DelegateExecutor - Remove string formatting logic from SDK visualizer (keep it in delegate layer) - Update tests to expect properly formatted agent names The delegate executor already formats agent names using _format_agent_name() which converts snake_case to Title Case (e.g., 'lodging_expert' -> 'Lodging Expert'). The SDK visualizer should not do any string transformation, just display the name as-is. Co-authored-by: openhands <openhands@all-hands.dev>
c1dbb4d to
792ff4c
Compare
- Added recipient field to MessageEvent to track message destination - Agent now derives recipient from last user message in event history - Updated visualizer to use recipient field for agent response titles - Removed hidden state tracking for cleaner, more maintainable design - Updated tests to use new recipient field instead of sender This supports multi-agent scenarios (like delegation) by making message routing explicit without adding complexity to the conversation state. Co-authored-by: openhands <openhands@all-hands.dev>
Architecture ImprovementI've updated the implementation to use a cleaner approach that eliminates hidden state: Previous Approach (first commit)
New Approach (current commit)
Benefits
Key Changes
This approach is much cleaner and more maintainable! 🎉 |
Move recipient derivation logic from agent.py to the visualizer for cleaner separation of concerns. The agent now only sets the sender field when delegating tasks, and the visualizer derives the recipient by examining the event history when displaying agent responses. Changes: - Remove recipient field from MessageEvent in message.py - Remove recipient derivation logic from agent.py - Add recipient derivation in default.py visualizer by looking back through event history to find the last user message sender - Update test to mock event history for recipient derivation - Keep sender field for delegation messages (set by delegation tool) This approach maintains backward compatibility while providing correct sender/receiver information for delegation scenarios. Co-authored-by: openhands <openhands@all-hands.dev>
- Implemented DelegationVisualizer in openhands-tools to display delegation-specific message labels
- Added _format_agent_name() function to convert snake_case/camelCase agent names to Title Case
- Updated DefaultConversationVisualizer in SDK to show simpler labels ('Message from User', 'Message from Agent')
- Updated 25_agent_delegation.py example to use DelegationVisualizer
- Added comprehensive test suite for DelegationVisualizer (5 tests)
- Updated SDK visualizer tests to match simplified labels
- All tests passing (21 tests for visualizers)
Fixes #1118
Co-authored-by: openhands <openhands@all-hands.dev>
The default visualizer now shows generic 'Message from Agent' labels without using the agent's specific name, keeping it simple and generic. Agent-specific names are only shown in the DelegationVisualizer which extends the default visualizer for delegation scenarios. This makes the separation of concerns clearer: - DefaultConversationVisualizer: Generic, no agent names needed - DelegationVisualizer: Shows specific agent names for delegation Updated tests to match the new generic labels. Co-authored-by: openhands <openhands@all-hands.dev>
The name parameter and _name attribute have been removed from: - ConversationVisualizerBase: No longer has name parameter or _name attribute - DefaultConversationVisualizer: No longer uses name, shows generic labels The DelegationVisualizer now independently adds: - name parameter in its __init__ - _name attribute for delegation-specific functionality - Properly calls parent __init__ without name parameter This maintains backward compatibility through proper inheritance while keeping the base visualizers generic and delegation-specific features in the appropriate subclass. All tests passing (16 SDK visualizer tests, 5 delegation visualizer tests). Co-authored-by: openhands <openhands@all-hands.dev>
This commit completes the delegation visualization improvements by ensuring agent names appear consistently in ALL event types, not just messages. Changes: - Override _create_event_panel() in DelegationVisualizer to add agent names to actions and observations - ActionEvent now shows '[Agent Name] Action' - ObservationEvent now shows '[Agent Name] Observation' - Added tests to verify action and observation events show agent names correctly This ensures the complete fix for issue #1118 where users expected to see: - 'Main Agent Action' instead of generic 'Agent Action' - 'Lodging Expert Observation' instead of generic 'Observation' Co-authored-by: openhands <openhands@all-hands.dev>
This reverts commit e049427.
- Use DelegationVisualizer explicitly in delegate impl for sub-agents - Check for DelegationVisualizer instance before accessing _name attribute - Update test mock to match BaseConversation.send_message signature - Remove name parameter from example custom visualizer Co-authored-by: openhands <openhands@all-hands.dev>
…isualizer This commit extends the DelegationVisualizer to show agent names for all event types: - Action events now show '[Agent Name] Action' instead of just 'Action' - Observation events now show '[Agent Name] Observation' instead of just 'Observation' - Message events continue to use the existing detailed sender/receiver format This provides consistent agent context across all event types in delegation scenarios, making it easier to track which agent is performing which action or receiving which observation during multi-agent delegation workflows. Implementation: - Override _create_event_panel() to handle ActionEvent and ObservationEvent - Extract agent name formatting and prepend to default titles - Delegate to parent class for other event types Tests: - Added test for action event with agent name - Added test for observation event with agent name Co-authored-by: openhands <openhands@all-hands.dev>
- Add 'Agent' suffix to delegated messages: 'Delegator Message to Lodging Agent' - Add 'to User' suffix to agent responses to user: 'Message from Delegator to User' - Update tests to match new labels - All 28 tests passing Co-authored-by: openhands <openhands@all-hands.dev>
… event titles This commit addresses all visualization issues in the delegation example: 1. System Prompt now shows: '[Agent Name] Agent System Prompt' 2. User messages now show: 'User Message to [Agent Name] Agent' 3. Agent-to-agent messages now show: '[Sender] Agent Message to [Receiver] Agent' 4. Agent-to-user messages now show: 'Message from [Agent Name] Agent to User' 5. Actions now show: '[Agent Name] Agent Action' 6. Observations now show: '[Agent Name] Agent Observation' Changes: - Added SystemPromptEvent handling in DelegationVisualizer - Updated all message, action, and observation labels to include 'Agent' suffix - Updated tests to match new label formats - Imported _SYSTEM_COLOR and SystemPromptEvent Fixes #1118 Co-authored-by: openhands <openhands@all-hands.dev>
Changed _format_agent_name from a module-level function to a static method of the DelegationVisualizer class for better encapsulation. Changes: - Moved _format_agent_name function inside DelegationVisualizer as @staticmethod - Updated all calls from _format_agent_name() to self._format_agent_name() - Updated docstring examples to reflect class method usage Co-authored-by: openhands <openhands@all-hands.dev>
Consolidated agent name formatting by removing the duplicate implementation from impl.py and using the centralized version in DelegationVisualizer. Changes: - Removed _format_agent_name function from impl.py - Updated all calls to use DelegationVisualizer._format_agent_name() - Maintains single source of truth for agent name formatting logic Co-authored-by: openhands <openhands@all-hands.dev>
…zer requirement All agent name formatting is now handled exclusively by the visualizer. The impl.py passes raw agent IDs and the DelegationVisualizer is responsible for formatting them for display. Changes: - Removed all calls to _format_agent_name from impl.py - Pass raw agent_id to DelegationVisualizer constructor - Pass raw parent_name to send_message (visualizer formats it) - Enforce that delegation tool requires DelegationVisualizer - Removed fallback code paths for other visualizer types 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 |
Delegation should not require DelegationVisualizer - it can work with no visualizer at all. Only create DelegationVisualizer for sub-agents if the parent conversation is already using DelegationVisualizer. Changes: - Removed ValueError that enforced DelegationVisualizer requirement - Sub-agents get DelegationVisualizer only if parent uses it - Sub-agents get no visualizer (None) if parent has no visualizer Co-authored-by: openhands <openhands@all-hands.dev>
…izer The skip_user_messages parameter doesn't make sense for delegation scenarios where inter-agent messages (which appear as user messages to sub-agents) are essential to understanding the delegation flow. Changes: - Removed skip_user_messages parameter from DelegationVisualizer.__init__ - Removed passing of skip_user_messages in impl.py - Simplified visualizer initialization Co-authored-by: openhands <openhands@all-hands.dev>
…onVisualizer" This reverts commit c00319c.
…andling The merge from main introduced CondensationRequest event handling that still referenced self._name which was removed in our refactoring. Updated to match the pattern used for other event types. Co-authored-by: openhands <openhands@all-hands.dev>
The base DefaultConversationVisualizer does not use the sender field at all. This test was added to verify sender functionality but is only relevant for the DelegationVisualizer subclass which overrides the behavior. The test has been removed from the SDK tests and only exists in the delegation-specific tests. Co-authored-by: openhands <openhands@all-hands.dev>
| extended_content: list[TextContent] = Field( | ||
| default_factory=list, description="List of content added by agent context" | ||
| ) | ||
| sender: str | None = Field( |
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.
Please let me try to see if I got this right, so right now, on the PR, we save a sender in the MessageEvent, although
- we don't tell the LLM the sender
- we don't use it in
visualize
🤔
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.
Agreed that's a bit sketchy!
We could use it in the default visualize so it makes more sense
enyst
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! Interesting which way it's going, maybe someday we'll tell the delegate who the sender really was 😇
I'd love it if @xingyaoww could take a look since it's changing the visualizer interface we just changed 😅
This PR reverts the visualizer to what is was before delegation "Message from Agent" "User Message" etc... @xingyaoww |
| Conversation will then calls `MyVisualizer()` followed by `initialize(state)` | ||
| """ | ||
|
|
||
| _name: str | None |
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.
is there anyway that we are not breaking this?
removing this is a breaking changes. But i do see doing so would simplify our code and hopefully have minimal impact to users
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.
Do we have any other breaking change for 1.0.1 ?
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.
I mean if you know, off-hand. Don't worry if you don't! I'll find out automatically
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.
this _name was introduce solely for delegation visualization. So I think it does make sense to remove it so the default viz remains as simple as possible... Cf other comment: no need for init anymore in minimal custom visualizer
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
Description
Fixes #1118
This PR fixes all visualization issues in the delegation example (
25_agent_delegation.py) by ensuring consistent "Agent" suffix usage across all event titles.Changes Made
1. System Prompt Visualization
SystemPromptEventhandling inDelegationVisualizer._create_event_panel()SystemPromptEventand_SYSTEM_COLORfrom SDK2. User Messages
3. Agent-to-Agent Messages
4. Agent-to-User Messages
5. Actions and Observations
Testing
tests/tools/delegate/test_visualizer.py)25_agent_delegation.pyImplementation Details
openhands-toolspackage (delegation-specific)openhands-sdk(maintains backward compatibility)Example Output
With these changes, delegation visualizations now show:
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:7e73ce6-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7e73ce6-python) is a multi-arch manifest supporting both amd64 and arm647e73ce6-python-amd64) are also available if needed