omniparser fixing - #277
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to handle controls detected by OmniParser grounding service by introducing a new MCP tool to add controls to the control dictionary and modifying the control collection strategy to automatically send newly detected grounding controls to the MCP server.
Key changes:
- New
add_control_listMCP tool that converts TargetInfo dictionaries to UIAWrapper objects and adds them to the control dictionary - Modified
_collect_merged_control_listto be async, detect newly added grounding controls, assign IDs, and send them to the MCP server - Configuration change to use
omniparser.IOU_THRESHOLDinstead of the system-leveliou_threshold_for_merge
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ufo/client/mcp/local_servers/ui_mcp_server.py |
Added add_control_list tool to accept grounding control data and wrap it as UIAWrapper objects in the control dictionary |
ufo/agents/processors/strategies/app_agent_processing_strategy.py |
Modified control merging to async operation, added logic to identify newly added controls, assign IDs, and dispatch commands to add them to the MCP server |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Step 3: Merging control list | ||
| merged_control_list = self._collect_merged_control_list( | ||
| merged_control_list = await self._collect_merged_control_list( | ||
| api_control_list, grounding_control_list |
There was a problem hiding this comment.
The method signature of _collect_merged_control_list was changed to add a required command_dispatcher parameter, but this call site was not updated to pass it. This will cause a TypeError at runtime. The command_dispatcher variable is available in scope at line 416, so it should be passed as the third argument.
| api_control_list, grounding_control_list | |
| api_control_list, grounding_control_list, command_dispatcher |
| # Create a set of API control IDs for fast lookup | ||
| api_control_ids = {control.id for control in api_control_list} | ||
|
|
||
| # Find controls in merged list that are not in API list | ||
| added_controls = [ | ||
| control | ||
| for control in merged_control_list | ||
| if control.id not in api_control_ids | ||
| ] |
There was a problem hiding this comment.
The logic for finding added controls may fail when controls have None IDs. If any control in api_control_list has id=None, then None will be included in api_control_ids. Subsequently, any grounding controls with id=None will not be identified as added controls because None not in api_control_ids will evaluate to False. Consider filtering out None values from the set or using a different comparison approach, such as comparing by object identity or other unique attributes.
| return | ||
|
|
||
| # Convert TargetInfo list to dict format for command parameters | ||
| control_list_data = [asdict(target) for target in added_controls] |
There was a problem hiding this comment.
The asdict function from the dataclasses module cannot be used with Pydantic BaseModel instances. TargetInfo is a Pydantic BaseModel (as defined in target.py), not a dataclass. This will raise a TypeError at runtime. Use target.model_dump() or target.dict() instead (depending on Pydantic version - v2 uses model_dump(), v1 uses dict()).
| control_list_data = [asdict(target) for target in added_controls] | |
| control_list_data = [ | |
| target.model_dump() if hasattr(target, "model_dump") else target.dict() | |
| for target in added_controls | |
| ] |
| for idx, control in enumerate(added_controls, start=1): | ||
| if not control.id: | ||
| control.id = str(max_id + idx) | ||
|
|
||
| self.logger.info( | ||
| f"Found {len(added_controls)} new controls added after merging. Assigned IDs {max_id + 1} to {max_id + len(added_controls)}. Sending add command." |
There was a problem hiding this comment.
The log message incorrectly assumes all added controls received new IDs. If some controls in added_controls already have IDs (line 611 checks if not control.id), they will be skipped during ID assignment, but the log message still reports "Assigned IDs {max_id + 1} to {max_id + len(added_controls)}". This could be misleading. Consider logging the actual number of controls that received IDs or the specific IDs that were assigned.
| for idx, control in enumerate(added_controls, start=1): | |
| if not control.id: | |
| control.id = str(max_id + idx) | |
| self.logger.info( | |
| f"Found {len(added_controls)} new controls added after merging. Assigned IDs {max_id + 1} to {max_id + len(added_controls)}. Sending add command." | |
| assigned_ids: List[str] = [] | |
| for idx, control in enumerate(added_controls, start=1): | |
| if not control.id: | |
| control.id = str(max_id + idx) | |
| assigned_ids.append(control.id) | |
| self.logger.info( | |
| "Found %d new controls added after merging. Assigned IDs to %d " | |
| "controls: %s. Sending add command.", | |
| len(added_controls), | |
| len(assigned_ids), | |
| ", ".join(assigned_ids) if assigned_ids else "none", |
No description provided.