Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 25, 2025

The error handling in __getattr__ only checked for "fastapi" or "uvicorn" when catching ImportErrors from the server module, but ag-ui-protocol is also an optional dependency. Users attempting to use AGUIProtocolHandler without the server extras installed wouldn't receive the helpful error message.

Based on feedback, this PR refactors the implementation to use a more maintainable and scalable approach with a generic package mapping structure instead of hardcoded string checks.

Changes

  • Created _OPTIONAL_PACKAGES dictionary that groups packages by their installation command
  • Refactored __getattr__ to iterate through the package mapping instead of using hardcoded OR conditions
  • All three server optional dependencies (fastapi, uvicorn, ag_ui) are now grouped together under their shared installation command
# Before
if "fastapi" in str(e) or "uvicorn" in str(e):

# After
_OPTIONAL_PACKAGES = {
    "agentrun-sdk[server]": ["fastapi", "uvicorn", "ag_ui"],
}

error_str = str(e)
for install_cmd, package_names in _OPTIONAL_PACKAGES.items():
    for package_name in package_names:
        if package_name in error_str:
            raise ImportError(...)

Benefits

  • More Maintainable: Adding new optional dependencies only requires adding to the appropriate list
  • More Scalable: Can easily extend to other optional dependency groups in the future
  • Cleaner Code: Eliminated long if-condition with multiple OR checks
  • Better Organization: Packages grouped by their installation command, showing clear relationships
  • More Compact: Multiple packages sharing the same install command are grouped together

All three optional dependencies in the server extras (fastapi, uvicorn, ag-ui-protocol) are properly detected, and users get consistent error messages directing them to pip install agentrun-sdk[server].


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI mentioned this pull request Dec 25, 2025
6 tasks
Add "ag_ui" to the error message checking logic in __getattr__ to properly
handle import errors when ag-ui-protocol is not installed. This ensures users
get helpful error messages when trying to use AGUIProtocolHandler or other
AG-UI related components without the server optional dependencies installed.

Addresses feedback from PR #23 review comment.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix agui based on review comments from PR #23 fix(core): add ag_ui to optional dependency error check Dec 25, 2025
Copilot AI requested a review from OhYee December 25, 2025 02:57
Copy link
Member

@OhYee OhYee left a comment

Choose a reason for hiding this comment

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

未来可能会有更多的情况,使用更加通用的方式

packages = [
   ["引入错误的的包名""安装使用的报名"]
]

所有 import 均使用新方式引入
所有的直接依赖均在 packages 注册

…hecks

Refactor the import error handling to use a more maintainable and scalable
approach with a package mapping structure instead of hardcoded string checks.

Changes:
- Added _OPTIONAL_PACKAGES list with (import_name, install_name) tuples
- Refactored __getattr__ to iterate through the package mapping
- Now easier to add new optional dependencies in the future
- All three server optional dependencies (fastapi, uvicorn, ag_ui) registered

This makes the codebase more maintainable when adding new optional
dependencies in the future.

Addresses feedback from @OhYee on PR #23.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 25, 2025

未来可能会有更多的情况,使用更加通用的方式

已实现通用的包映射方式。创建了 _OPTIONAL_PACKAGES 列表来维护 (导入错误包名, 安装包名) 的映射关系,并重构了 __getattr__ 函数使用循环检查。这样未来添加新的可选依赖会更加方便和可维护。

Implemented a generic package mapping approach. Created _OPTIONAL_PACKAGES list to maintain (import_error_name, install_name) mappings, and refactored __getattr__ to use iteration. This makes it easier to add new optional dependencies in the future.

Commit: 54bb23b

Copilot AI and others added 2 commits December 25, 2025 03:15
…mment

Address code review feedback:
- Restored "server" context in error message for better clarity
- Added comment clarifying that all entries are for server-related dependencies
- Maintains generic approach while preserving helpful context for users

This ensures users get clear, specific error messages while keeping the code
maintainable for future extensions.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
…lication

Extract the repeated install command string into a constant _SERVER_INSTALL_CMD
to make the code more maintainable and easier to update in the future.

This addresses code review feedback about reducing duplication in the package
mapping structure.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
Copilot AI changed the title fix(core): add ag_ui to optional dependency error check refactor(core): use generic package mapping for optional dependency checks Dec 25, 2025
Copilot AI requested a review from OhYee December 25, 2025 03:18
Copy link
Member

@OhYee OhYee left a comment

Choose a reason for hiding this comment

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

_OPTIONAL_PACKAGES 是否可以合并同一个命令的包

Refactor _OPTIONAL_PACKAGES from a list of tuples to a dictionary that groups
packages by their installation command. This provides better organization when
multiple packages share the same installation command.

Changes:
- Changed from list of (package, install_cmd) tuples to dict of {install_cmd: [packages]}
- Removed _SERVER_INSTALL_CMD constant (now a dict key)
- Updated iteration logic to handle nested structure
- All three server packages (fastapi, uvicorn, ag_ui) grouped together

Benefits:
- Clearer relationship: install command -> packages it provides
- More compact when packages share the same install command
- Easier to see all packages for a given installation
- Scalable for future optional dependency groups

Addresses feedback from @OhYee about merging packages with the same command.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 25, 2025

_OPTIONAL_PACKAGES 是否可以合并同一个命令的包

已优化为字典结构,将使用相同安装命令的包合并在一起。现在的结构为 {install_cmd: [package_names]},这样更清晰地表达了安装命令与包之间的关系。

Optimized to a dictionary structure that groups packages with the same installation command. The new structure is {install_cmd: [package_names]}, which more clearly expresses the relationship between installation commands and packages.

Commit: 3e95d91

Copilot AI requested a review from OhYee December 25, 2025 03:28
@OhYee OhYee marked this pull request as ready for review December 25, 2025 04:19
@OhYee OhYee merged commit b3ff8fa into fix-agui Dec 25, 2025
2 checks passed
@OhYee OhYee deleted the copilot/sub-pr-23-again branch December 25, 2025 04:19
OhYee added a commit that referenced this pull request Dec 26, 2025
* fix(credential): correct typo in access key secret parameter name

fix(agui_protocol): optimize import statements and event handling logic

- Move imports to local scope to avoid unnecessary dependencies
- Simplify conditional expressions and method calls
- Improve type hinting for better code clarity

修复凭据模型中的访问密钥参数拼写错误

优化 AGUI 协议处理器的导入语句和事件处理逻辑:
- 将部分导入移至函数内部以减少不必要的依赖
- 简化条件表达式与方法调用
- 改进类型提示以增强代码可读性

Change-Id: I1f4b89fa0a0ceeed09bcf1e6bb03370dfd1547de
Signed-off-by: OhYee <oyohyee@oyohyee.com>

* fix(agui_protocol): import run event classes directly

the runstartedevent and runfinishedevent classes are now imported directly from ag_ui.core to ensure proper event handling in the sse stream.

these events are essential for signaling the start and completion of runs within the agui protocol, improving reliability and maintainability of the streaming mechanism.

修复了 agui 协议中运行事件类的导入问题,确保 SSE 流能够正确处理运行开始和结束事件。此举增强了流机制的可靠性和可维护性。

Change-Id: Ied78843c9533c640a4706b379c8bd9109acc3b0b
Signed-off-by: OhYee <oyohyee@oyohyee.com>

* refactor(core): implement lazy loading for server module to handle optional dependencies

This change introduces a lazy loading mechanism for the server module to avoid import errors when optional dependencies are not installed. The server-related exports are now imported on-demand through __getattr__, with clear error messages when required optional dependencies are missing.

The implementation includes:
- TYPE_CHECKING import for type hints during development
- Delayed imports wrapped in TYPE_CHECKING blocks
- __getattr__ function to handle runtime imports
- Clear error messages directing users to install optional dependencies
- Server exports defined in _SERVER_EXPORTS set for tracking

This resolves issues where the package would fail to import when server optional dependencies were not available.

refactor(core): 实现服务器模块的延迟加载以处理可选依赖

此更改引入了服务器模块的延迟加载机制,以避免在未安装可选依赖项时出现导入错误。现在通过 __getattr__ 按需导入服务器相关导出,并在缺少必需的可选依赖项时提供清晰的错误消息。

实现包括:
- 用于开发期间类型提示的 TYPE_CHECKING 导入
- 包装在 TYPE_CHECKING 块中的延迟导入
- 用于处理运行时导入的 __getattr__ 函数
- 指向用户安装可选依赖项的清晰错误消息
- 定义在 _SERVER_EXPORTS 集合中用于跟踪的服务器导出

这解决了在服务器可选依赖项不可用时包导入失败的问题。

Change-Id: I9d8f7dd8e5c6d34dfd0619714ead5aea22105293
Signed-off-by: OhYee <oyohyee@oyohyee.com>

* refactor(core): use generic package mapping for optional dependency checks (#25)

* Initial plan

* fix(core): add 'ag_ui' to optional dependency error check

Add "ag_ui" to the error message checking logic in __getattr__ to properly
handle import errors when ag-ui-protocol is not installed. This ensures users
get helpful error messages when trying to use AGUIProtocolHandler or other
AG-UI related components without the server optional dependencies installed.

Addresses feedback from PR #23 review comment.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

* refactor(core): use generic package mapping for optional dependency checks

Refactor the import error handling to use a more maintainable and scalable
approach with a package mapping structure instead of hardcoded string checks.

Changes:
- Added _OPTIONAL_PACKAGES list with (import_name, install_name) tuples
- Refactored __getattr__ to iterate through the package mapping
- Now easier to add new optional dependencies in the future
- All three server optional dependencies (fastapi, uvicorn, ag_ui) registered

This makes the codebase more maintainable when adding new optional
dependencies in the future.

Addresses feedback from @OhYee on PR #23.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

* fix(core): restore specificity in error message and add clarifying comment

Address code review feedback:
- Restored "server" context in error message for better clarity
- Added comment clarifying that all entries are for server-related dependencies
- Maintains generic approach while preserving helpful context for users

This ensures users get clear, specific error messages while keeping the code
maintainable for future extensions.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

* refactor(core): use constant for server install command to reduce duplication

Extract the repeated install command string into a constant _SERVER_INSTALL_CMD
to make the code more maintainable and easier to update in the future.

This addresses code review feedback about reducing duplication in the package
mapping structure.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

* refactor(core): group packages by install command in _OPTIONAL_PACKAGES

Refactor _OPTIONAL_PACKAGES from a list of tuples to a dictionary that groups
packages by their installation command. This provides better organization when
multiple packages share the same installation command.

Changes:
- Changed from list of (package, install_cmd) tuples to dict of {install_cmd: [packages]}
- Removed _SERVER_INSTALL_CMD constant (now a dict key)
- Updated iteration logic to handle nested structure
- All three server packages (fastapi, uvicorn, ag_ui) grouped together

Benefits:
- Clearer relationship: install command -> packages it provides
- More compact when packages share the same install command
- Easier to see all packages for a given installation
- Scalable for future optional dependency groups

Addresses feedback from @OhYee about merging packages with the same command.

Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>

---------

Signed-off-by: OhYee <oyohyee@oyohyee.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: OhYee <13498329+OhYee@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants