Skip to content

Conversation

@xingyaoww
Copy link
Collaborator

@xingyaoww xingyaoww commented Nov 4, 2025

Summary

This PR improves the visualization API to make it more intuitive and flexible by allowing users to pass a ConversationVisualizer instance directly to the visualize parameter.

Motivation

Currently, if a user wants to use a custom visualizer, they have to:

# Current workaround (confusing):
conversation = Conversation(
    agent=agent,
    workspace=cwd,
    visualize=False,  # Confusing: we DO want visualization!
    callbacks=[custom_visualizer.on_event],
)

This is counter-intuitive because setting visualize=False sounds like you DON't want visualization, but you actually do - just with a custom visualizer.

Changes

Enhanced the visualize parameter to accept three types:

  • True: Use default visualizer (existing behavior, backward compatible)
  • False or None: No visualization (existing behavior, backward compatible)
  • ConversationVisualizer instance: Use custom visualizer (NEW)

Example Usage (New)

# Ideal (now supported):
conversation = Conversation(
    agent=agent,
    workspace=cwd,
    visualize=custom_visualizer,  # Direct and clear!
)

# Or to disable visualization:
conversation = Conversation(
    agent=agent,
    workspace=cwd,
    visualize=None  # or False
)

Files Modified

  • openhands-sdk/openhands/sdk/conversation/conversation.py: Updated factory class type signatures
  • openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py: Implemented new logic for LocalConversation
  • openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py: Implemented new logic for RemoteConversation
  • tests/sdk/conversation/local/test_conversation_visualize_param.py: Added comprehensive tests for the new functionality
  • examples/01_standalone_sdk/20_custom_visualizer.py: Added example demonstrating the new feature
  • .github/scripts/check_documented_examples.py: Improved error messages with step-by-step documentation instructions

Backward Compatibility

Fully backward compatible - all existing code will continue to work:

  • visualize=True (default) → creates default visualizer (unchanged)
  • visualize=False → no visualization (unchanged)
  • All existing tests continue to pass

Testing

  • Added 3 new test cases covering the new functionality
  • All 65 tests in local conversation tests pass
  • Pre-commit checks (ruff, pyright, pycodestyle) all pass

Documentation

📚 Related documentation PR: OpenHands/docs#77

The documentation includes:

  • Complete guide for using custom visualizers
  • Examples of highlight patterns and customization
  • Before/after comparison showing the API improvement

This addresses the feedback about making the visualization system more intuitive and user-friendly.


Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:a578ad3-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-a578ad3-python \
  ghcr.io/openhands/agent-server:a578ad3-python

All tags pushed for this build

ghcr.io/openhands/agent-server:a578ad3-golang-amd64
ghcr.io/openhands/agent-server:v1.0.0a5_golang_tag_1.21-bookworm_binary-amd64
ghcr.io/openhands/agent-server:a578ad3-golang-arm64
ghcr.io/openhands/agent-server:v1.0.0a5_golang_tag_1.21-bookworm_binary-arm64
ghcr.io/openhands/agent-server:a578ad3-java-amd64
ghcr.io/openhands/agent-server:v1.0.0a5_eclipse-temurin_tag_17-jdk_binary-amd64
ghcr.io/openhands/agent-server:a578ad3-java-arm64
ghcr.io/openhands/agent-server:v1.0.0a5_eclipse-temurin_tag_17-jdk_binary-arm64
ghcr.io/openhands/agent-server:a578ad3-python-amd64
ghcr.io/openhands/agent-server:v1.0.0a5_nikolaik_s_python-nodejs_tag_python3.12-nodejs22_binary-amd64
ghcr.io/openhands/agent-server:a578ad3-python-arm64
ghcr.io/openhands/agent-server:v1.0.0a5_nikolaik_s_python-nodejs_tag_python3.12-nodejs22_binary-arm64
ghcr.io/openhands/agent-server:a578ad3-golang
ghcr.io/openhands/agent-server:a578ad3-java
ghcr.io/openhands/agent-server:a578ad3-python

About Multi-Architecture Support

  • Each variant tag (e.g., a578ad3-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., a578ad3-python-amd64) are also available if needed

- Enhanced the 'visualize' parameter to accept three types:
  - True: Use default visualizer (existing behavior, backward compatible)
  - False or None: No visualization (existing behavior, backward compatible)
  - ConversationVisualizer instance: Use custom visualizer (NEW)

- This makes the API more intuitive:
  - Before: visualize=False + callbacks=[custom_viz.on_event] (confusing)
  - After: visualize=custom_visualizer (clear and direct)

- Updated type signatures in:
  - Conversation factory class
  - LocalConversation
  - RemoteConversation

- Added comprehensive tests for the new functionality
- All existing tests continue to pass (backward compatible)

Co-authored-by: openhands <[email protected]>
- Shows how to pass a ConversationVisualizer directly to Conversation
- Demonstrates custom highlighting configuration
- Makes the API usage clear and intuitive

Co-authored-by: openhands <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-agent-server/openhands/agent_server
   event_service.py1988258%51–52, 71–73, 76–81, 93, 109, 116, 118, 125–126, 134–137, 144–146, 158–159, 162–163, 165–167, 169, 174, 177, 181–183, 185, 187, 191–195, 199, 203–204, 206, 223–224, 271, 278–279, 281, 284–286, 288, 292–295, 299–302, 321–322, 324–331, 333–334, 340, 346, 359–360
openhands-sdk/openhands/sdk/conversation
   base.py66592%100, 105–106, 141, 152
   conversation.py18194%99
openhands-sdk/openhands/sdk/conversation/impl
   local_conversation.py1804873%114, 116–117, 150–151, 176, 181, 193, 210, 218–220, 224–225, 277–278, 281, 288, 309–311, 313, 329, 331, 333, 337, 339–341, 343, 345, 351–352, 365–366, 368, 370, 374–377, 395, 403, 405–407, 425, 427
   remote_conversation.py3659972%54–60, 67–70, 99, 106, 114, 116–119, 129, 138, 142–143, 148–151, 186, 200, 217, 228, 237–238, 290, 310, 318, 330, 338–341, 344, 349–350, 355–359, 364–368, 373–376, 379, 390–391, 395, 399, 402, 473, 479, 481, 497, 499–500, 511, 528–529, 535, 550, 583, 585, 587–588, 592–593, 602, 610, 615–617, 619, 622, 624–625, 642, 649, 655–656, 664–665, 671–672, 677–678
openhands-sdk/openhands/sdk/conversation/visualizer
   base.py20290%69, 86
   default.py1404270%102–103, 106, 109–111, 113, 121, 125, 131, 144, 146, 160, 170–174, 187, 219–223, 232–236, 244–248, 259, 275, 279, 288, 290, 292, 295, 314
openhands-tools/openhands/tools/delegate
   impl.py1038616%30, 32–33, 42–43, 47, 53–54, 57–60, 62, 81–82, 88–89, 99–102, 104, 106, 108, 114–116, 119, 121, 123, 129–130, 132–134, 139–141, 158–159, 166–168, 177, 179–181, 183, 185–188, 191–194, 196–197, 201–204, 207–209, 214–215, 218–219, 222, 224–228, 230, 233–235, 237–238, 241, 243, 248–250
TOTAL11824548353% 

- Added detailed step-by-step instructions for documenting examples
- Included links to format documentation and example files
- Makes it much clearer for contributors how to add documentation

Co-authored-by: openhands <[email protected]>
xingyaoww pushed a commit to OpenHands/docs that referenced this pull request Nov 4, 2025
- Added comprehensive guide for using custom ConversationVisualizer
- Shows the new direct API: visualize=custom_visualizer
- Includes examples of highlight patterns and customization options
- Documents the improvement over the old callback-based approach

Related to: OpenHands/software-agent-sdk#1025

Co-authored-by: openhands <[email protected]>
- Rename 20_custom_visualizer.py to 26_custom_visualizer.py to avoid conflict with existing 20_stuck_detector.py
- Replace simple example with comprehensive MinimalProgressVisualizer implementation
- Demonstrate the new API improvement where ConversationVisualizer instances can be passed directly to visualize parameter
- Include detailed documentation and comparison between old and new approaches
- Add proper error handling and step tracking functionality

Co-authored-by: openhands <[email protected]>
Remove outdated API comparison comments and simplify example code
to focus on the current API without referencing old approaches.

Co-authored-by: openhands <[email protected]>
openhands-agent and others added 5 commits November 5, 2025 11:08
- Replace if/elif chain with @Handles decorator pattern
- Add EventHandlerMixin for clean event handler registration
- Improve code maintainability and type safety
- Self-documenting event handler methods
- Easy extensibility for new event types

Co-authored-by: openhands <[email protected]>
- Create separate _sentinel.py module for _DEFAULT_VISUALIZER
- Import sentinel from dedicated module in all conversation classes
- Resolves circular import between conversation.py and implementation files
- All visualizer API tests pass with proper default behavior

Co-authored-by: openhands <[email protected]>
- Fix parameter name from 'visualize' to 'visualizer' in example code
- Update comments to reflect the API change
- Align with the simplified API that uses visualizer=ConversationVisualizer/None/default

Co-authored-by: openhands <[email protected]>
jpshackelford pushed a commit to OpenHands/docs that referenced this pull request Nov 5, 2025
- Fix parameter name from 'visualize' to 'visualizer' throughout documentation
- Update visualization options to reflect simplified API:
  * Default behavior (no parameter): Creates default visualizer automatically
  * None: Disable visualization entirely
  * ConversationVisualizer instance: Use custom visualizer
- Improve error handling example with more practical code
- Align all examples with the API simplification changes

Related to: OpenHands/software-agent-sdk#1025

Co-authored-by: openhands <[email protected]>
Copy link
Contributor

jpshackelford commented Nov 5, 2025

Documentation Review and API Alignment

In response to @simonrosenberg's feedback:

Fixed Issues:

  • Updated parameter name from visualize to visualizer throughout example code
  • Aligned with the simplified API that uses visualizer=ConversationVisualizer/None/default
  • Enhanced error handling examples with more practical patterns

- Added import to test_conversation_factory.py for visualizer parameter tests
- Added import to test_remote_conversation.py for remote conversation tests
- Added import to integration base.py for integration test framework
- Fixed line length issue in integration base.py comment

All visualizer parameter updates now complete and tests passing.

Co-authored-by: openhands <[email protected]>
@jpshackelford
Copy link
Contributor

@OpenHands please fix the failing actions on PR #1025 at branch feature/improve-visualizer-api Be sure you understand the complete context of the PR and recent commits. Please also remember that we can omit the visualizer parameter to use the default visualizer. We shouldn't be adding an extra import to tests just to specify the default.

@openhands-ai
Copy link

openhands-ai bot commented Nov 5, 2025

I'm on it! jpshackelford can track my progress at all-hands.dev

- Update test files to use new visualizer parameter instead of visualize
- Change visualize=False to visualizer=None for no visualization
- Remove visualize=True from examples to use default visualizer
- Fix all type check failures from API signature mismatches
- All tests now pass with correct parameter usage

Co-authored-by: openhands <[email protected]>
@openhands-ai
Copy link

openhands-ai bot commented Nov 5, 2025

Summary

I successfully fixed all failing GitHub Actions on PR #1025 by resolving API signature mismatches throughout the codebase. The changes were minimal and focused:

✅ Request Completion Checklist:

  • Fixed failing actions: Resolved all pyright type check failures and test issues
  • Understood PR context: Applied fixes consistent with the new visualizer API that accepts ConversationVisualizer instances directly
  • Used default visualizer properly: Omitted the visualizer parameter entirely instead of adding extra imports (per your guidance)
  • No unnecessary imports: Followed your instruction to avoid adding imports just to specify defaults

🔧 Changes Made:

Test Files (4 files updated):

  • visualize=Falsevisualizer=None (for no visualization)
  • visualize=True → omit parameter (for default visualization)
  • Updated parameter names to match new API signature

Example Files (5 files updated):

  • Removed visualize=True parameters to use default visualizer
  • No extra imports added

✅ Verification:

  • Type checks: All pyright errors resolved
  • Tests: 30+ tests passing across all modified files
  • Pre-commit: All hooks pass (formatting, linting, type checking)
  • Git: Changes committed and pushed to feature/improve-visualizer-api

The failing GitHub Actions should now pass because all API signature mismatches have been resolved while maintaining the intended behavior of the new visualizer API.

View full conversation

- Update test to correctly verify that when a ConversationVisualizer instance is passed directly, it's used as-is
- Remove incorrect expectation that create_default_visualizer() should be called when passing a custom instance
- The test now properly verifies the custom visualizer instance is used and its callback is registered
- All 1077 SDK tests now pass

Co-authored-by: openhands <[email protected]>
self._visualizer = visualizer
composed_list = [self._visualizer.on_event] + composed_list
# visualizer should happen first for visibility
elif visualizer is _DEFAULT_VISUALIZER:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@OpenHands think about ways we can handle _DEFAULT_VISUALIZER in a more elegant way rather than making visualizer: ConversationVisualizer | None | Any

Copy link

Choose a reason for hiding this comment

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

I'm on it! xingyaoww can track my progress at all-hands.dev

Copy link

Choose a reason for hiding this comment

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

Summary

I've successfully improved the visualizer API by replacing the sentinel value pattern with a more elegant boolean-based approach. Here's what was changed:

Changes Made:

  1. Type Signature Improvement

    • Changed from: visualizer: ConversationVisualizer | None | Any = _DEFAULT_VISUALIZER
    • Changed to: visualizer: bool | ConversationVisualizer | None = True
  2. Implementation Updates

    • Replaced visualizer is _DEFAULT_VISUALIZER checks with visualizer is True
    • Updated logic comments to reflect "False or None" instead of just "None"
    • Removed the _sentinel.py file entirely
  3. Files Modified

    • conversation.py - Updated factory class type signatures (3 overloads + implementation)
    • local_conversation.py - Updated implementation and docstring
    • remote_conversation.py - Updated implementation and docstring
    • Deleted _sentinel.py - No longer needed

Benefits:

More Elegant: Uses intuitive boolean values (True/False) instead of custom sentinel objects
Better Type Safety: Eliminates the need for Any type, enabling proper type checking
Cleaner API: Intuitive behavior - True = default visualizer, False/None = no visualization, ConversationVisualizer = custom
Simpler Code: No need for sentinel pattern boilerplate
Fully Backward Compatible: All existing tests pass without modification

Verification:

The changes have been committed and pushed to the feature/improve-visualizer-api branch, updating PR #1025.

View full conversation

Replace the sentinel value pattern (_DEFAULT_VISUALIZER) with a more
elegant boolean-based approach for the visualizer parameter.

Changes:
- Changed type signature from 'ConversationVisualizer | None | Any' to
  'bool | ConversationVisualizer | None'
- Updated default value from _DEFAULT_VISUALIZER to True
- Replaced sentinel check 'visualizer is _DEFAULT_VISUALIZER' with
  'visualizer is True'
- Removed the _sentinel.py file as it's no longer needed
- Updated documentation to reflect the new type signature

Benefits:
- Cleaner, more intuitive API (True = default, False/None = disabled)
- Proper type checking without needing 'Any'
- No need for custom sentinel values or types
- Fully backward compatible with existing usage

All tests pass successfully.

Co-authored-by: openhands <[email protected]>
@jpshackelford
Copy link
Contributor

@OpenHands -- your latest commit seems to be taking us in the wrong direction. Please see review comments from @simonrosenberg and @jpshackelford

@openhands-ai
Copy link

openhands-ai bot commented Nov 5, 2025

I'm on it! jpshackelford can track my progress at all-hands.dev

Based on review feedback from @simonrosenberg and @jpshackelford, this commit
improves the visualizer API by replacing the confusing True sentinel value
with an explicit _DEFAULT_VISUALIZER object.

Changes:
- Replace 'visualizer: bool | ConversationVisualizer | None = True'
  with 'visualizer: ConversationVisualizer | None | Any = _DEFAULT_VISUALIZER'
- Use explicit sentinel object _DEFAULT_VISUALIZER instead of True
- Update logic from 'visualizer is True' to 'visualizer is _DEFAULT_VISUALIZER'
- Update docstrings to reflect cleaner API (omit parameter for default)
- Maintain full backward compatibility

This makes the API much cleaner:
- Default: Conversation(agent=agent) # Uses default visualizer
- None: Conversation(agent=agent, visualizer=None) # No visualization
- Custom: Conversation(agent=agent, visualizer=my_visualizer) # Custom

The True sentinel was confusing because it didn't clearly indicate 'default'.
The new explicit sentinel object makes the intent much clearer.

Co-authored-by: openhands <[email protected]>
@openhands-ai
Copy link

openhands-ai bot commented Nov 5, 2025

Summary

I have successfully addressed the review comments from @simonrosenberg and @jpshackelford regarding the visualizer API changes in PR #1025.

✅ Request Completion Checklist:

  • Understood reviewers' concerns: The True sentinel value was confusing and unclear
  • Designed cleaner API: Replaced True with explicit _DEFAULT_VISUALIZER object
  • Implemented focused changes: Updated only the 3 core files that needed modification
  • Maintained backward compatibility: All existing code continues to work
  • Validated thoroughly: All tests pass, type checking passes, pre-commit hooks pass

🔧 Key Changes Made:

  1. Replaced confusing sentinel: True_DEFAULT_VISUALIZER = object()
  2. Updated type signatures: bool | ConversationVisualizer | None = TrueConversationVisualizer | None | Any = _DEFAULT_VISUALIZER
  3. Updated logic: visualizer is Truevisualizer is _DEFAULT_VISUALIZER
  4. Clarified docstrings: Now clearly indicate "Default: Use default visualizer (when omitted)"

🎯 Result - Much Cleaner API:

# Default visualizer (when omitted)
Conversation(agent=agent)

# No visualization  
Conversation(agent=agent, visualizer=None)

# Custom visualizer
Conversation(agent=agent, visualizer=my_visualizer)

The API is now much more intuitive and follows better software engineering practices by using an explicit sentinel object rather than overloading the meaning of True. The changes have been committed and pushed to the feature/improve-visualizer-api branch, updating PR #1025 with the improved implementation.

View full conversation

visualize: bool = True,
name_for_visualization: str | None = None,
visualizer: (
type[ConversationVisualizerBase] | ConversationVisualizerBase | None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm a bit dense, is there a reason why this is a type too, or is it the agent overreacting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, basically in the default cases, it defaults to a class so that the conversation can initialize it -- i'm not sure if we can put DefaultConversationVisualizer() (an instance) as a default arg though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think because the user can pass either an instance or class for the visualizer

e.g Conversation(visualizer=MyCustomVisualizer)

or

custom_visualizer = MyCustomVisualizer()
Conversation(visualizer=custom_visualizer)

I feel we could simplify this by asking the user to only pass in a class instead of an instance. We can check in the __init__ method whether the user sent an instance instead and raise an exception. It would also get rid of some duplicate code initializing the visualizer state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's what GPT-5 suggests

Generally no, it’s not a good idea to make the default argument of a function a class instance (or any mutable object like a list, dict, or set).


Here’s why — and when it can be acceptable.


⚠️ The core issue: Mutable default arguments


In Python, default argument values are evaluated once — at the time the function is defined, not each time it’s called.


That means if you do something like this:

class Config:
    def __init__(self):
        self.count = 0

def do_something(cfg=Config()):
    cfg.count += 1
    return cfg.count

You might expect each call to create a new Config object — but instead, every call shares the same instance:

do_something()  # 1
do_something()  # 2
do_something()  # 3

This happens because the single Config() instance created at function definition time is reused.


✅ The correct pattern


If you want a new instance each time, make the default None and create it inside the function:

def do_something(cfg=None):
    if cfg is None:
        cfg = Config()
    cfg.count += 1
    return cfg.count

Now each call behaves as expected.


🧠 When a class instance 

can

 be a default


There are limited cases where this pattern is acceptable:

  • The object is immutable (e.g. a frozen Pydantic model, a tuple, or a dataclass with frozen=True).

  • The object is stateless or pure, meaning reusing it has no side effects.

  • It’s a sentinel or constant used only for comparison, not mutation.


Example:

EMPTY_CFG = Config()
def process(cfg=EMPTY_CFG):
    ...

As long as you treat EMPTY_CFG as read-only, this is fine.


✅ Best practice summary

Case Good Idea? Notes
Mutable object (list, dict, class instance) Causes shared state bugs
Immutable or stateless object Safe but rare
Use None as default and create new instance ✅✅ Idiomatic and safest


Would you like me to show the same pattern using a Pydantic model as the default argument example (since you prefer Pydantic)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the type[ConversationVisualizerBase] serve the purpose of a default_factory

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to take Rohit's suggestion here

I feel we could simplify this by asking the user to only pass in a class

but idk if right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i'd vote for go with the current version since it doesn't limit flexibility - but i can be convinced otherwise!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@OpenHands Read this particular review thread. Now, there are some Ruff rules that check for mutable defaults in python: find them, you can search the internet docs if you need to. I think it's more than one.

Then checkout main branch, and verify if our Ruff in pre-commit and in CI contains those rules. If it doesn't, open an issue documenting the problem, and open a PR to add them.

Copy link

Choose a reason for hiding this comment

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

I'm on it! enyst can track my progress at all-hands.dev

Copy link

Choose a reason for hiding this comment

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

Summary of work

What I found (Ruff rules for mutable defaults)

  • B006: mutable-argument-default
  • B008: function-call-in-default-argument
  • B039: mutable-contextvar-default
  • RUF012: mutable-class-default

What I verified on main

  • Our Ruff config (pyproject.toml) only selected E, F, I, UP, ARG. These mutable-default rules were not enabled.
  • pre-commit and CI use that same config, so they weren’t enforcing these rules.

What I changed

  • Created branch chore/lint-enable-mutable-defaults from main.
  • Edited pyproject.toml to enable the rules via:
    • [tool.ruff.lint] extend-select = ["B006", "B008", "B039", "RUF012"]
  • Kept the change minimal: only pyproject.toml was modified.

What I observed after enabling the rules (for awareness)

  • Quick local scan showed:
    • 20× B008
    • 8× RUF012
    • 2× B006
    • 0× B039

Tracking and PRs

Checklist

  • Identified all relevant Ruff rules (B006, B008, B039, RUF012)
  • Verified main branch, pre-commit, and CI didn’t include them
  • Opened an issue documenting the problem and plan
  • Opened a draft PR adding the rules with a minimal change
  • Committed only the relevant file (pyproject.toml)

Next steps

  • Decide whether to land the rules with fixes in follow-up PR(s) or add allowlists for intentional patterns; then mark the PR ready for review.

View full conversation

@openhands-ai
Copy link

openhands-ai bot commented Nov 6, 2025

Summary of my work (/codereview-roasted on PR #1025)

What I did

  • Reviewed the visualizer API changes end-to-end with emphasis on ease-of-use for client developers, clean design, and backward compatibility.
  • Assessed defaults, migration path, and internal usage (server, delegation, tests/examples).
  • Provided concrete, actionable feedback with alternatives for each issue.

Key findings (short)

  • Direction is good: visualizer accepts None | instance | class. This improves ergonomics.
  • Backward compatibility and defaults have gaps that will surprise/break users.

Critical issues (must fix)

  • Server still passes visualize=False to LocalConversation; now silently ignored → default visualizer is enabled unexpectedly.

    • Fix options:
      • Back-compat shim: accept visualize in constructors/factory, translate to visualizer (True→Default, False→None), DeprecationWarning.
      • Centralize translation in Conversation factory and pop visualize/name_for_visualization.
      • If shims are rejected: error loudly on unknown visualize with clear migration text.
  • Delegation tool breaks visualization inheritance:

    • Reads parent_conversation.visualize (attr doesn’t exist) and passes visualize=sub_visualizer (ignored now).
    • Fix options:
      • Add BaseConversation.visualizer property (read-only) and update DelegateExecutor to pass visualizer=... (instance/class).
      • Or retain a translate path for visualize temporarily with warnings.
  • Default highlighting regression:

    • DEFAULT_HIGHLIGHT_REGEX isn’t applied by default → degraded out-of-the-box UX.
    • Fix: use DEFAULT_HIGHLIGHT_REGEX when highlight_regex is None (allow user overrides/merge).
  • Callback composition risks state loss:

    • Visualizer runs before persistence; any visualizer error can prevent events from being persisted.
    • Fix: persist first or wrap callbacks in try/except to guarantee persistence.
  • Silent swallowing of unknown kwargs (**_):

    • Masks migration mistakes (e.g., visualize), causing confusing behavior.
    • Fix: accept deprecated params and warn, or validate/raise on unknown kwargs during transition.

Improvement opportunities (should fix)

  • Public accessor: expose conversation.visualizer for tools/extensions (read-only).
  • Clarify/document the “class vs instance vs None” decision with examples.
  • Allow injecting Console into DefaultConversationVisualizer for testability/quiet mode.
  • Consider registry/singledispatch for event handling to avoid if/elif chains.
  • Ensure Remote/Local parity is intentional (server path must pass visualizer=None explicitly).
  • Clean up examples/tests to consistently use visualizer=None or a visualizer instance; avoid importing defaults just to select default behavior.

Verdict

  • Needs targeted rework. The API shape is solid, but back-compat gaps, a default UX regression, and a few robustness issues should be addressed before merging.

Recommended next steps

  • Add a back-compat shim for visualize/name_for_visualization and emit DeprecationWarning.
  • Update internal callers:
    • Server: use visualizer=None.
    • Delegation: use .visualizer accessor and pass visualizer= sub-visualizers.
  • Restore default highlighting.
  • Harden compose_callbacks to guarantee persistence.
  • Update docs with a short migration guide and examples for all three user intents (default, none, custom).

View full conversation

@xingyaoww xingyaoww added test-examples Run all applicable "examples/" files. Expensive operation. integration-test Runs the integration tests and comments the results labels Nov 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

🔄 Running Examples with openhands/claude-haiku-4-5-20251001

Last updated: 2025-11-06 16:49:18 UTC

Example Status Duration Cost
01_standalone_sdk/02_custom_tools.py ✅ PASS 95s $0.11
01_standalone_sdk/03_activate_skill.py ✅ PASS 10s $0.01
01_standalone_sdk/05_use_llm_registry.py ✅ PASS 10s $0.01
01_standalone_sdk/07_mcp_integration.py ✅ PASS 48s $0.02
01_standalone_sdk/09_pause_example.py ✅ PASS 87s $0.01
01_standalone_sdk/10_persistence.py ✅ PASS 35s $0.02
01_standalone_sdk/11_async.py ✅ PASS 137s $0.03
01_standalone_sdk/12_custom_secrets.py ✅ PASS 18s $0.01
01_standalone_sdk/13_get_llm_metrics.py ✅ PASS 55s $0.02
01_standalone_sdk/14_context_condenser.py ✅ PASS 162s $0.28
01_standalone_sdk/17_image_input.py ✅ PASS 17s $0.02
01_standalone_sdk/18_send_message_while_processing.py ✅ PASS 36s $0.01
01_standalone_sdk/19_llm_routing.py ✅ PASS 22s $0.02
01_standalone_sdk/20_stuck_detector.py ✅ PASS 18s $0.01
01_standalone_sdk/21_generate_extraneous_conversation_costs.py ✅ PASS 12s $0.01
01_standalone_sdk/22_anthropic_thinking.py ✅ PASS 15s $0.01
01_standalone_sdk/23_responses_reasoning.py ✅ PASS 48s $0.01
01_standalone_sdk/24_planning_agent_workflow.py ✅ PASS 682s $0.64
01_standalone_sdk/25_agent_delegation.py ❌ FAIL (exit: 1) 59s $0.00
01_standalone_sdk/26_custom_visualizer.py ✅ PASS 24s $0.00N/A
02_remote_agent_server/01_convo_with_local_agent_server.py ✅ PASS 96s $0.05
02_remote_agent_server/02_convo_with_docker_sandboxed_server.py ✅ PASS 160s $0.04
02_remote_agent_server/03_browser_use_with_docker_sandboxed_server.py ✅ PASS 64s $0.04

❌ Some tests failed

Total: 23 | Passed: 22 | Failed: 1

View full workflow run

@openhands-ai
Copy link

openhands-ai bot commented Nov 6, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Check Documented Examples

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #1025 at branch `feature/improve-visualizer-api`

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

🧪 Integration Tests Results

Overall Success Rate: 100.0%
Total Cost: $0.38
Models Tested: 3
Timestamp: 2025-11-06 16:20:50 UTC

📁 Detailed Logs & Artifacts

Click the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.

📊 Summary

Model Success Rate Tests Passed Total Tests Cost
litellm_proxy_deepseek_deepseek_chat 100.0% 7/7 7 $0.04
litellm_proxy_gpt_5_mini_2025_08_07 100.0% 7/7 7 $0.04
litellm_proxy_claude_sonnet_4_5_20250929 100.0% 7/7 7 $0.30

📋 Detailed Results

litellm_proxy_deepseek_deepseek_chat

  • Success Rate: 100.0% (7/7)
  • Total Cost: $0.04
  • Run Suffix: litellm_proxy_deepseek_deepseek_chat_1c1d1c6_deepseek_run_N7_20251106_161723

litellm_proxy_gpt_5_mini_2025_08_07

  • Success Rate: 100.0% (7/7)
  • Total Cost: $0.04
  • Run Suffix: litellm_proxy_gpt_5_mini_2025_08_07_1c1d1c6_gpt5_mini_run_N7_20251106_161730

litellm_proxy_claude_sonnet_4_5_20250929

  • Success Rate: 100.0% (7/7)
  • Total Cost: $0.30
  • Run Suffix: litellm_proxy_claude_sonnet_4_5_20250929_1c1d1c6_sonnet_run_N7_20251106_161732

Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

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

Generally LGTM, thank you for this. I think it's more developer-friendly this way!

@xingyaoww xingyaoww enabled auto-merge (squash) November 6, 2025 16:40
@xingyaoww xingyaoww merged commit 4c276bd into main Nov 6, 2025
37 of 39 checks passed
@xingyaoww xingyaoww deleted the feature/improve-visualizer-api branch November 6, 2025 16:41
xingyaoww added a commit to OpenHands/docs that referenced this pull request Nov 6, 2025
* Document custom visualizer feature

- Added comprehensive guide for using custom ConversationVisualizer
- Shows the new direct API: visualize=custom_visualizer
- Includes examples of highlight patterns and customization options
- Documents the improvement over the old callback-based approach

Related to: OpenHands/software-agent-sdk#1025

Co-authored-by: openhands <[email protected]>

* Update custom visualizer documentation for improved example

- Fix file reference from 20_custom_visualizer.py to 26_custom_visualizer.py
- Update example code to match the comprehensive MinimalProgressVisualizer implementation
- Add documentation for both simple configuration and custom subclass approaches
- Clarify the difference between basic highlighting and full custom visualization
- Update running instructions to use correct file name

This aligns with the improved example in software-agent-sdk PR #1025.

Co-authored-by: openhands <[email protected]>

* Comprehensive rewrite of custom visualizer documentation

Transform the documentation from simple API usage examples to comprehensive
educational content that teaches developers how to create effective custom visualizers:

Key improvements:
- Detailed explanation of the event system (ActionEvent, ObservationEvent, MessageEvent, AgentErrorEvent)
- Two clear approaches: simple configuration vs. full subclassing
- Comprehensive best practices section covering:
  * State management and progress tracking
  * Error handling and fallback strategies
  * Performance considerations for high-frequency events
  * Flexible output formats for different environments
  * Integration with external systems (webhooks, logging, monitoring)
- Real-world code examples for each concept
- Built-in visualizer reference with default patterns and configuration options
- Updated to reference the improved 26_custom_visualizer.py example

This transforms the guide from basic API documentation into a complete tutorial
for building production-ready custom visualizers.

Co-authored-by: openhands <[email protected]>

* Address review feedback: remove 'production-ready', focus on custom visualizers, improve next steps

- Remove 'production-ready' language as requested
- Remove mention of API improvement to focus on custom visualizers
- Update Next Steps section with more relevant related topics:
  * Async conversations and event callbacks
  * Conversation metrics and performance tracking
  * Interactive conversations with real-time updates
  * Pause/resume with custom logic

Co-authored-by: openhands <[email protected]>

* Add custom visualizer guide to navigation

- Add convo-custom-visualizer to Agent Features section in docs.json
- Position it after agent-custom as requested
- This should resolve the 404 error and make the guide appear in sidebar

Co-authored-by: openhands <[email protected]>

* Add link to default visualizer source code

- Add reference link to ConversationVisualizer implementation
- Helps developers understand the code structure and implementation details
- Positioned in Built-in Visualizer Reference section for easy access

Co-authored-by: openhands <[email protected]>

* Update custom visualizer documentation to match actual code patterns

- Update example code to match the actual MinimalProgressVisualizer implementation
- Remove overly speculative best practices that aren't evident in the code
- Focus best practices on patterns actually demonstrated in the examples
- Simplify performance considerations to match what's actually shown
- Update state management examples to use the actual implementation patterns

Co-authored-by: openhands <[email protected]>

* Add comprehensive event types table to custom visualizer documentation

Add a table overview of all event types handled by the default visualizer
before diving into detailed descriptions. This provides a quick reference
for developers to understand the complete event system.

Co-authored-by: openhands <[email protected]>

* Improve custom visualizer documentation with event properties and rendering details

- Replace 'when fired' code snippets with detailed property information
- Add comprehensive event property descriptions and default rendering info
- Add Approach 3: Custom Object with on_event Method (no subclassing required)
- Improve subclassing documentation with built-in features and patterns
- Update code examples to show actual event property usage
- Better explain ConversationVisualizer inheritance benefits

Co-authored-by: openhands <[email protected]>

* Document decorator-based event handler pattern for custom visualizers

- Add section on EventHandlerMixin and @Handles decorator
- Show how to avoid long if/elif chains in on_event methods
- Explain benefits: self-documenting, type-safe, extensible
- Provide complete working example with best practices

Co-authored-by: openhands <[email protected]>

* Update custom visualizer documentation for new API

- Fix parameter name from 'visualize' to 'visualizer' throughout documentation
- Update visualization options to reflect simplified API:
  * Default behavior (no parameter): Creates default visualizer automatically
  * None: Disable visualization entirely
  * ConversationVisualizer instance: Use custom visualizer
- Improve error handling example with more practical code
- Align all examples with the API simplification changes

Related to: OpenHands/software-agent-sdk#1025

Co-authored-by: openhands <[email protected]>

* Update documentation to reflect new visualizer API from PR #1025

- Changed base class from ConversationVisualizer to ConversationVisualizerBase
- Updated to new DefaultConversationVisualizer for built-in visualization
- Changed parameter from 'visualize' to 'visualizer' throughout
- Updated all code examples to use new API
- Simplified visualizer configuration options
- Updated method signatures and initialization patterns
- Synchronized all code blocks with latest examples from agent-sdk PR #1025

Co-authored-by: openhands <[email protected]>

* Update convo-custom-visualizer.mdx

---------

Co-authored-by: openhands <[email protected]>
Co-authored-by: John-Mason P. Shackelford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-test Runs the integration tests and comments the results test-examples Run all applicable "examples/" files. Expensive operation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants