Refactor fast-agent and agno support using structured configuration classes #8
Conversation
Reviewer's GuideThis PR refactors both Agno and FastAgent framework support to use structured configuration classes and dedicated code generators, replacing manual string concatenation with type-safe builders for maintainable and testable code generation. Sequence diagram for Agno agent code generation using structured configurationsequenceDiagram
participant User
participant AgnoFramework
participant AgnoConfigBuilder
participant AgnoFrameworkConfig
participant AgnoCodeGenerator
User->>AgnoFramework: build_agent_content()
AgnoFramework->>AgnoConfigBuilder: build_model_config(), build_tools_for_servers()
AgnoFramework->>AgnoFrameworkConfig: create AgnoAgentConfig, AgnoTeamConfig
AgnoFramework->>AgnoCodeGenerator: pass AgnoFrameworkConfig
AgnoCodeGenerator->>AgnoCodeGenerator: generate_complete_code()
AgnoCodeGenerator-->>AgnoFramework: return generated code
AgnoFramework-->>User: return code
Sequence diagram for FastAgent code generation using structured configurationsequenceDiagram
participant User
participant FastAgentFramework
participant FastAgentConfig
participant FastAgentCodeGenerator
User->>FastAgentFramework: build_agent_content()
FastAgentFramework->>FastAgentConfig: create and populate config
FastAgentFramework->>FastAgentCodeGenerator: pass FastAgentConfig
FastAgentCodeGenerator->>FastAgentCodeGenerator: generate_complete_code()
FastAgentCodeGenerator-->>FastAgentFramework: return generated code
FastAgentFramework-->>User: return code
Class diagram for new Agno structured configuration and code generationclassDiagram
class AgnoFrameworkConfig {
+List~AgnoAgentConfig~ agents
+AgnoTeamConfig team
+bool has_prompt_file
+Set~str~ required_imports
+Set~str~ tool_imports
}
class AgnoAgentConfig {
+str name
+str variable_name
+str instruction
+str role
+AgnoModelConfig model_config
+List~AgnoToolConfig~ tools
+bool use_history
+bool human_input
+Dict~str,Any~ enhanced_properties
}
class AgnoTeamConfig {
+str name
+str variable_name
+str mode
+List~str~ agent_variables
+AgnoModelConfig model_config
+List~AgnoToolConfig~ tools
+List~str~ instructions
+Dict~str,Any~ enhanced_properties
}
class AgnoModelConfig {
+str model_type
+str model_id
+str provider
+str api_key_env
+str base_url_env
}
class AgnoToolConfig {
+str tool_class
+str import_path
+Dict~str,Any~ params
}
class AgnoCodeGenerator {
-AgnoFrameworkConfig config
+generate_imports()
+generate_agent_definitions()
+generate_team_definition()
+generate_main_function()
+generate_entry_point()
+generate_complete_code()
}
class AgnoConfigBuilder {
+build_model_config(model: str)
+build_tools_for_servers(servers: List~str~)
+get_tool_imports(tools: List~AgnoToolConfig~)
}
AgnoFrameworkConfig "1" o-- "*" AgnoAgentConfig
AgnoFrameworkConfig "1" o-- "0..1" AgnoTeamConfig
AgnoAgentConfig "1" o-- "0..1" AgnoModelConfig
AgnoAgentConfig "1" o-- "*" AgnoToolConfig
AgnoTeamConfig "1" o-- "0..1" AgnoModelConfig
AgnoTeamConfig "1" o-- "*" AgnoToolConfig
AgnoCodeGenerator --> AgnoFrameworkConfig
AgnoConfigBuilder --> AgnoToolConfig
AgnoConfigBuilder --> AgnoModelConfig
Class diagram for new FastAgent structured configuration and code generationclassDiagram
class FastAgentConfig {
+str name
+List~Dict~ agents
+List~Dict~ routers
+List~Dict~ chains
+List~Dict~ orchestrators
+bool has_prompt_file
+__post_init__()
}
class FastAgentCodeGenerator {
-FastAgentConfig config
+generate_imports()
+generate_app_creation()
+generate_agent_decorators()
+generate_router_decorators()
+generate_chain_decorators()
+generate_orchestrator_decorators()
+generate_main_function()
+generate_entry_point()
+generate_complete_code()
}
FastAgentCodeGenerator --> FastAgentConfig
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…lasses Co-authored-by: yeahdongcn <2831050+yeahdongcn@users.noreply.github.com>
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Copilot - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/agentman/frameworks/agno.py:231` </location>
<code_context>
def get_dockerfile_config_lines(self) -> List[str]:
"""Get Agno-specific Dockerfile configuration lines."""
return ["COPY .env ."]
+
+ def get_dockerfile_config_lines(self) -> List[str]:
</code_context>
<issue_to_address>
Duplicate get_dockerfile_config_lines method definition.
The first definition will be overwritten by the second. Please remove the duplicate to prevent confusion and maintenance issues.
</issue_to_address>
### Comment 2
<location> `src/agentman/frameworks/fast_agent_builder.py:19` </location>
<code_context>
+ orchestrators: List[Dict[str, Any]] = None
+ has_prompt_file: bool = False
+
+ def __post_init__(self):
+ if self.agents is None:
+ self.agents = []
+ if self.routers is None:
+ self.routers = []
+ if self.chains is None:
+ self.chains = []
+ if self.orchestrators is None:
+ self.orchestrators = []
+
</code_context>
<issue_to_address>
Mutable default arguments are handled in __post_init__, but the dataclass fields use None as default.
Consider using field(default_factory=list) for these fields to make the code more idiomatic and eliminate the need for __post_init__ to handle mutable defaults.
Suggested implementation:
```python
from dataclasses import dataclass, field
```
```python
agents: List[Dict[str, Any]] = field(default_factory=list)
routers: List[Dict[str, Any]] = field(default_factory=list)
chains: List[Dict[str, Any]] = field(default_factory=list)
orchestrators: List[Dict[str, Any]] = field(default_factory=list)
has_prompt_file: bool = False
```
```python
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def get_dockerfile_config_lines(self) -> List[str]: | ||
| """Get Agno-specific Dockerfile configuration lines.""" | ||
| return ["COPY .env ."] |
There was a problem hiding this comment.
issue (bug_risk): Duplicate get_dockerfile_config_lines method definition.
The first definition will be overwritten by the second. Please remove the duplicate to prevent confusion and maintenance issues.
There was a problem hiding this comment.
Fixed the duplicate method definition in commit eb45193. Removed the redundant get_dockerfile_config_lines method to eliminate the potential override issue.
| def __post_init__(self): | ||
| if self.agents is None: | ||
| self.agents = [] | ||
| if self.routers is None: | ||
| self.routers = [] | ||
| if self.chains is None: | ||
| self.chains = [] | ||
| if self.orchestrators is None: |
There was a problem hiding this comment.
suggestion: Mutable default arguments are handled in post_init, but the dataclass fields use None as default.
Consider using field(default_factory=list) for these fields to make the code more idiomatic and eliminate the need for post_init to handle mutable defaults.
Suggested implementation:
from dataclasses import dataclass, field agents: List[Dict[str, Any]] = field(default_factory=list)
routers: List[Dict[str, Any]] = field(default_factory=list)
chains: List[Dict[str, Any]] = field(default_factory=list)
orchestrators: List[Dict[str, Any]] = field(default_factory=list)
has_prompt_file: bool = FalseThere was a problem hiding this comment.
Implemented the suggested improvement in commit eb45193. Changed FastAgentConfig to use field(default_factory=list) for all list fields and removed the __post_init__ method, making the code more idiomatic and cleaner.
| for line in lines: | ||
| if line.startswith('import ') or line.startswith('from '): | ||
| import_section_found = True | ||
| elif '= Agent(' in line: | ||
| agent_section_found = True | ||
| elif '= Team(' in line: | ||
| team_section_found = True | ||
| elif 'def main(' in line: | ||
| main_section_found = True |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if line.startswith('import ') or line.startswith('from '): | ||
| import_section_found = True | ||
| elif '= Agent(' in line: | ||
| agent_section_found = True | ||
| elif '= Team(' in line: | ||
| team_section_found = True | ||
| elif 'def main(' in line: | ||
| main_section_found = True |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for instruction in self.config.team.instructions: | ||
| lines.append(f' "{instruction}",') |
There was a problem hiding this comment.
suggestion (code-quality): Replace a for append loop with list extend (for-append-to-extend)
| for instruction in self.config.team.instructions: | |
| lines.append(f' "{instruction}",') | |
| lines.extend( | |
| f' "{instruction}",' | |
| for instruction in self.config.team.instructions | |
| ) |
| config = parser.parse_content(content) | ||
|
|
||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| builder = AgentBuilder(config, temp_dir) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into method (
extract-method) - Simplify logical expression using De Morgan identities (
de-morgan)
| config = parser.parse_content(content) | ||
|
|
||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| builder = AgentBuilder(config, temp_dir) |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
| claude_config = builder.build_model_config("anthropic/claude-3-sonnet") | ||
| assert claude_config.model_type == "claude" | ||
| assert claude_config.model_id == "anthropic/claude-3-sonnet" | ||
|
|
||
| openai_config = builder.build_model_config("openai/gpt-4") | ||
| assert openai_config.model_type == "openai" | ||
| assert openai_config.model_id == "openai/gpt-4" | ||
|
|
||
| custom_config = builder.build_model_config("groq/mixtral-8x7b") | ||
| assert custom_config.model_type == "custom" | ||
| assert custom_config.model_id == "groq/mixtral-8x7b" |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
| config = parser.parse_content(content) | ||
|
|
||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| builder = AgentBuilder(config, temp_dir) |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
| builder = AgentBuilder(config, temp_dir) | ||
| code = builder.framework.build_agent_content() | ||
|
|
||
| # Should generate both agent and router | ||
| assert "@fast.agent(" in code | ||
| assert "@fast.router(" in code | ||
| assert "default=True" in code | ||
| assert 'model="openai/gpt-4"' in code | ||
| assert 'agents=["router_agent"]' in code |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
There was a problem hiding this comment.
Pull Request Overview
This PR refactors FastAgent and Agno framework support to use structured configuration classes and code generators, replacing manual string concatenation with type-safe builders and improving maintainability.
- Introduced
FastAgentConfigandFastAgentCodeGeneratorfor FastAgent - Introduced
AgnoFrameworkConfig,AgnoCodeGenerator, andAgnoConfigBuilderfor Agno - Updated
FastAgentFrameworkandAgnoFrameworkto delegate code generation to the new builders and added tests to cover them
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_refactored_framework_builders.py | Added tests covering structured builders for both frameworks |
| src/agentman/frameworks/fast_agent_builder.py | New classes for FastAgent structured config and code generation |
| src/agentman/frameworks/fast_agent.py | Refactored to use FastAgentCodeGenerator |
| src/agentman/frameworks/agno_builder.py | New classes for Agno structured config and code generation |
| src/agentman/frameworks/agno.py | Refactored to use AgnoCodeGenerator and AgnoConfigBuilder |
src/agentman/frameworks/agno.py
Outdated
| def get_dockerfile_config_lines(self) -> List[str]: | ||
| """Get Agno-specific Dockerfile configuration lines.""" | ||
| return ["COPY .env ."] |
There was a problem hiding this comment.
There are two identical definitions of get_dockerfile_config_lines in this class. Please remove the redundant method to avoid confusion and potential overrides.
| def get_dockerfile_config_lines(self) -> List[str]: | |
| """Get Agno-specific Dockerfile configuration lines.""" | |
| return ["COPY .env ."] |
There was a problem hiding this comment.
Fixed the duplicate method definition in commit eb45193. The redundant get_dockerfile_config_lines method has been removed to prevent method override issues.
| agents: List[Dict[str, Any]] = None | ||
| routers: List[Dict[str, Any]] = None | ||
| chains: List[Dict[str, Any]] = None | ||
| orchestrators: List[Dict[str, Any]] = None | ||
| has_prompt_file: bool = False | ||
|
|
||
| def __post_init__(self): | ||
| if self.agents is None: | ||
| self.agents = [] | ||
| if self.routers is None: | ||
| self.routers = [] | ||
| if self.chains is None: | ||
| self.chains = [] | ||
| if self.orchestrators is None: | ||
| self.orchestrators = [] | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Using None with manual __post_init__ initialization works, but dataclasses support field(default_factory=list) which is more idiomatic and eliminates the need for post-init checks.
| agents: List[Dict[str, Any]] = None | |
| routers: List[Dict[str, Any]] = None | |
| chains: List[Dict[str, Any]] = None | |
| orchestrators: List[Dict[str, Any]] = None | |
| has_prompt_file: bool = False | |
| def __post_init__(self): | |
| if self.agents is None: | |
| self.agents = [] | |
| if self.routers is None: | |
| self.routers = [] | |
| if self.chains is None: | |
| self.chains = [] | |
| if self.orchestrators is None: | |
| self.orchestrators = [] | |
| agents: List[Dict[str, Any]] = field(default_factory=list) | |
| routers: List[Dict[str, Any]] = field(default_factory=list) | |
| chains: List[Dict[str, Any]] = field(default_factory=list) | |
| orchestrators: List[Dict[str, Any]] = field(default_factory=list) | |
| has_prompt_file: bool = False |
There was a problem hiding this comment.
Implemented the suggested improvement in commit eb45193. Updated FastAgentConfig to use field(default_factory=list) which is more idiomatic and eliminates the need for manual __post_init__ checks.
|
|
||
| # Verify no string concatenation artifacts | ||
| assert not code.startswith('["') | ||
| assert not "\\n" in code.replace("\\n", "") |
There was a problem hiding this comment.
[nitpick] This assertion is confusing—replacing literal "\\n" in the code and then checking for it may not clearly express the intent. Consider simplifying or documenting what artifact you're guarding against.
| assert not "\\n" in code.replace("\\n", "") | |
| # Verify no escaped newline characters in the generated code | |
| assert "\\n" not in code |
| """Build tool configurations for given servers.""" | ||
| tools = [] | ||
| for server in servers: | ||
| if server in self.server_tool_mapping: | ||
| tools.append(self.server_tool_mapping[server]) | ||
| return tools |
There was a problem hiding this comment.
[nitpick] This method may return duplicate tool configurations if the same server appears multiple times. Consider deduplicating the resulting list to avoid redundant imports or tool instantiations.
| """Build tool configurations for given servers.""" | |
| tools = [] | |
| for server in servers: | |
| if server in self.server_tool_mapping: | |
| tools.append(self.server_tool_mapping[server]) | |
| return tools | |
| """Build tool configurations for given servers, ensuring no duplicates.""" | |
| tools_set = set() | |
| for server in servers: | |
| if server in self.server_tool_mapping: | |
| tools_set.add(self.server_tool_mapping[server]) | |
| return list(tools_set) |
|
@copilot Check all review comments and selectively address them and reply when needed. |
…defaults, optimize list comprehensions, add tool deduplication Co-authored-by: yeahdongcn <2831050+yeahdongcn@users.noreply.github.com>
|
@copilot If you've addressed all needed review comments, mark them as resolved. |
…mprove formatting Co-authored-by: yeahdongcn <2831050+yeahdongcn@users.noreply.github.com>
I've addressed all key review feedback issues and fixed code quality concerns: Fixed Issues:
Quality Improvements:
All review comments have been addressed. Commit: 1c58c2a |
|
@copilot also learn from #4 to see if anything is applicable to this PR. |
…l mapping, enhance code structure Co-authored-by: yeahdongcn <2831050+yeahdongcn@users.noreply.github.com>
I've analyzed PR #4 and applied several key learnings to improve the current implementation: Applied Improvements:
Key Benefits:
The implementation now reflects the best of both approaches. Commit: b3b22ef |
|
@copilot Perform the final round validation. Mark this PR as |
…orting Co-authored-by: yeahdongcn <2831050+yeahdongcn@users.noreply.github.com>
Final validation complete - this PR is Ready for review! Comprehensive Quality Verification: Key Achievements:
The refactor successfully delivers on all objectives with high code quality standards. Commit: 7e12ad9 |
This PR refactors the framework support for both FastAgent and Agno to use structured configuration objects instead of manual string concatenation, making the code more maintainable and robust.
Problem
The current implementation generates Python code through extensive string manipulation:
This approach has several issues:
Solution
Introduced structured configuration classes and code generators:
FastAgent Framework
FastAgentConfig: Type-safe configuration object for app settingsFastAgentCodeGenerator: Generates clean Python code from configurationAgno Framework
AgnoFrameworkConfig: Structured configuration for agents, teams, and toolsAgnoCodeGenerator: Handles import management and code structureAgnoConfigBuilder: Maps server names to tool configurations with proper typingKey Improvements
Example Output
FastAgent (before vs after - same clean output):
Agno (now with proper tool imports and structured configuration):
Testing
Benefits
Fixes #7.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
Summary by Sourcery
Refactor FastAgent and Agno framework support to leverage structured configuration classes and builder-based code generators in place of manual string concatenation.
New Features:
Enhancements:
Tests: