-
Notifications
You must be signed in to change notification settings - Fork 57
LLM Client Factory #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LLM Client Factory #150
Conversation
- Introduces base client interface (LLMClient) with provider-specific implementations
- Adds provider factory pattern for creating LLM clients (create_client())
- Ensures model auto-loading before requests in LemonadeClient
- Sets default temperature (0.1) for deterministic responses
- Fixes ChatAgent JSON response parsing for markdown code blocks
New architecture:
- LemonadeProvider, OpenAIProvider, ClaudeProvider
- Factory function: create_client("lemonade") for easy client creation
- Model loading: Auto-download and load models when --model flag is used
- Temperature default: 0.1 for consistent, deterministic outputs
Test plan:
- Unit tests for model loading (tests/unit/test_lemonade_model_loading.py)
- Unit tests for LLM client factory (tests/unit/test_llm_client_factory.py)
- Replace 'from gaia.llm.llm_client import LLMClient' with 'from gaia.llm import LLMClient' - Update ChatSDK to use create_client() factory function - Update all components (routing, audio, apps, mcp, cli, blender) to use new imports - Update test imports - Update lint.py import validation All tests passing (246 passed, 13 skipped) All lint checks passed
There was a problem hiding this 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 refactors the LLM client architecture from a monolithic LLMClient class to a provider-specific pattern with a factory function for instantiation, improving maintainability and extensibility.
Key Changes
- Introduces an abstract base class
LLMClientwith provider-specific implementations (LemonadeProvider,OpenAIProvider,ClaudeProvider) - Adds a factory function
create_client()that auto-detects providers from flags or explicit provider names - Implements proactive model loading in
LemonadeClientto prevent 404 errors - Updates all imports across the codebase from
gaia.llm.llm_clienttogaia.llm
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| util/lint.py | Updates import check from gaia.llm.llm_client to gaia.llm |
| tests/unit/test_llm_client_factory.py | Adds comprehensive tests for factory pattern and provider capabilities |
| tests/unit/test_llm.py | Removes obsolete base URL normalization tests |
| tests/unit/test_lemonade_model_loading.py | Adds tests for proactive model loading functionality |
| tests/test_sdk.py | Updates LLMClient imports to new package structure |
| src/gaia/mcp/mcp_bridge.py | Updates to use create_client() factory |
| src/gaia/llm/vlm_client.py | Copyright year update only |
| src/gaia/llm/providers/openai_provider.py | Implements OpenAI provider with chat, generate, and embed support |
| src/gaia/llm/providers/lemonade.py | Implements Lemonade provider with full method support including vision |
| src/gaia/llm/providers/claude.py | Implements Claude provider with vision support |
| src/gaia/llm/providers/init.py | Exports provider classes |
| src/gaia/llm/llm_client.py | Removes monolithic LLMClient (723 lines deleted) |
| src/gaia/llm/lemonade_client.py | Adds base_url parameter, _ensure_model_loaded() method, and prompt parameter to load_model() |
| src/gaia/llm/factory.py | Implements factory function with auto-detection logic |
| src/gaia/llm/exceptions.py | Adds NotSupportedError exception |
| src/gaia/llm/base_client.py | Defines abstract base class with default NotSupportedError implementations |
| src/gaia/llm/init.py | Exports factory, base client, and exception |
| src/gaia/cli.py | Updates to use factory with explicit "lemonade" provider |
| src/gaia/chat/sdk.py | Updates to use factory with auto-detection flags |
| src/gaia/audio/audio_client.py | Updates to use factory |
| src/gaia/apps/llm/app.py | Updates to use factory |
| src/gaia/agents/routing/agent.py | Updates to use factory |
| src/gaia/agents/blender/tests/test_agent_simple.py | Updates imports to use new base_client module |
| src/gaia/agents/blender/tests/test_agent.py | Updates imports to use base_client |
| src/gaia/agents/blender/agent_simple.py | Updates to use factory with system_prompt support |
| setup.py | Adds gaia.llm.providers package |
Comments suppressed due to low confidence (1)
tests/test_sdk.py:430
- The test expects
chat_completions,get_available_models, andestimate_tokensmethods on LLMClient, but the new base_client.py ABC only definesgenerate,chat,embed,vision,get_performance_stats,load_model, andunload_model. This test will fail because these methods don't exist in the new interface. The test should be updated to match the new API or these methods should be added to the base client if they're still needed.
# Check methods exist
assert hasattr(LLMClient, "generate")
assert hasattr(LLMClient, "chat_completions")
assert hasattr(LLMClient, "get_available_models")
assert hasattr(LLMClient, "estimate_tokens")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses 8 issues identified in GitHub Copilot review:
1. Critical - Claude API system prompt bug
- Fix ClaudeProvider.chat() to use 'system' parameter instead of
prepending system messages to messages array
- Claude's API requires system prompt as separate parameter
2-5. Fix system_prompt passthrough in 4 files
- src/gaia/chat/sdk.py: Pass system_prompt to create_client()
- src/gaia/audio/audio_client.py: Pass system_prompt to create_client()
- src/gaia/apps/llm/app.py: Pass system_prompt to create_client()
- src/gaia/agents/blender/tests/test_agent_simple.py: Pass system_prompt
6. Fix test imports and assertions
- tests/test_sdk.py: Update method checks to match new ABC interface
- Check for 'generate', 'chat', 'embed', 'vision' instead of removed methods
7. Clean up dead code
- src/gaia/agents/blender/agent_simple.py: Remove unused use_local parameter
8. Fix docstring style
- src/gaia/llm/factory.py: Change 'Comments:' to 'Note:' for PEP-257 compliance
All files verified for Python syntax correctness.
- Update factory.py docstring to use standard 'Note:' convention - Update test_sdk.py to check for correct LLMClient interface methods - Verified other comments were already addressed in previous commits Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Update GitHub workflow to use new import path (gaia.llm.llm_client → gaia.llm) - Fix RAG test fixture to patch create_client factory instead of LLMClient class - All 5 RAG integration tests now passing
|
Make sure to test all agents and provide a summary table here once complete. Approved conditional on documentation + testing. |
TestingThis has been tested against the following GAIA User Guides:
|
Summary
Refactors LLM client architecture to use provider-specific implementations instead of a monolithic
LLMClientclass, improving maintainability and extensibility.Changes
Architecture
LLMClient(ABC) with provider-specific implementationsLemonadeProvider,OpenAIProvider,ClaudeProvidercreate_client("lemonade")for easy instantiationsrc/gaia/llm/llm_client.pyKey Features
LemonadeClientfrom gaia.llm import LLMClient, create_clientMigration
Existing code using
LLMClientshould update imports: