Skip to content

Conversation

@vcfgv
Copy link
Collaborator

@vcfgv vcfgv commented Sep 11, 2025

No description provided.

@vcfgv vcfgv requested a review from Copilot September 11, 2025 08:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the agent system by removing the name parameter from the @serve decorator and refactoring the AgentRegistry from a class-based to an instance-based approach with module-level convenience functions.

  • Removed name parameter from @serve decorator, now using class name directly
  • Refactored AgentRegistry from static class methods to instance methods with a global default instance
  • Added comprehensive unit tests for the new registry system

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
python/valuecell/examples/core_e2e_demo.py Removed name parameters from @Serve decorators and reduced sleep time
python/valuecell/core/agent/tests/test_registry.py Added comprehensive test suite for the new AgentRegistry system
python/valuecell/core/agent/registry.py Refactored from class methods to instance methods with convenience functions
python/valuecell/core/agent/decorator.py Updated to use register_agent function and removed name parameter handling
python/valuecell/core/agent/connect.py Updated imports to use new registry convenience functions
python/valuecell/core/agent/README.md Updated documentation to remove name parameter examples
.github/workflows/python.yml Added unit test job to CI workflow

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

assert len(unregistered) == 2
assert "Test_1" in unregistered
assert "Test_2" in unregistered
# Note: Since TestAgent1 was unregistered by "Test_1", "Other" is also removed
Copy link

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

This comment and the changed expectation suggest a potential misunderstanding of the unregister behavior. The comment should clearly explain why 'Other' is removed when unregistering by pattern 'Test', as this might not be intuitive to readers.

Suggested change
# Note: Since TestAgent1 was unregistered by "Test_1", "Other" is also removed
# Note: In this registry implementation, unregistering any key for a class removes all keys for that class.
# Therefore, since "Test_1" (registered to TestAgent1) is unregistered, all keys for TestAgent1, including "Other", are also removed.

Copilot uses AI. Check for mistakes.
@vcfgv vcfgv merged commit c1d04b6 into main Sep 11, 2025
2 checks passed
@vcfgv vcfgv deleted the refactor/remove-serve-decorator-name-field-for-simplicity branch September 11, 2025 10:16
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.

1 participant