Implement Pluggable LLM Provider Architecture with Dynamic Model Selection#129
Implement Pluggable LLM Provider Architecture with Dynamic Model Selection#129Kaustubh1204 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces a pluggable LLM provider system: centralized Pydantic Settings, an LLMManager with provider registry and runtime fallback logic, provider implementations for OpenAI/Groq/Gemini, LangGraph nodes updated to use the manager, added LLM response model, tests + verification script, dependency additions, and a major README overhaul. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Manager as LLMManager
participant Registry as Provider Registry
participant ProviderA as Primary Provider
participant ProviderB as Fallback Provider
participant LLM as ChatModel Instance
App->>Manager: get_llm(provider_name?)
Manager->>Registry: resolve provider order (explicit/default)
Manager->>ProviderA: provider.get_llm(..., model/keys)
alt ProviderA instantiation succeeds
ProviderA->>LLM: create ChatModel (validate API key, pass params)
LLM->>Manager: return instance
Manager->>App: return LLM
else ProviderA instantiation fails
ProviderA->>Manager: raise/log error
Manager->>ProviderB: attempt next provider via _create_llm/_fallback
ProviderB->>LLM: create ChatModel
LLM->>Manager: return instance
Manager->>App: return LLM
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@backend/app/config.py`:
- Around line 1-2: Remove the unused import "os" from the top of the module: the
current import list includes "import os" but no code references the symbol;
update the import block to only import what is used (e.g., keep "from
pydantic_settings import BaseSettings, SettingsConfigDict") to eliminate the
unused import warning.
In `@backend/app/llm/manager.py`:
- Around line 33-55: Modify the fallback logic so the provider that just failed
is excluded from retry attempts: update the call site in the exception handler
that currently does return self._fallback(**kwargs) to pass the failed provider
name (e.g., return self._fallback(failed_provider=target_provider, **kwargs)),
then change the _fallback signature to def _fallback(self, failed_provider:
Optional[str] = None, **kwargs) -> BaseChatModel and ensure inside _fallback you
skip both self.primary_provider and failed_provider when iterating
self._providers (refer to _create_llm and _fallback to locate code).
In `@backend/app/llm/providers/base.py`:
- Around line 1-4: Remove the unused import LLMResponse from
backend/app/llm/providers/base.py: locate the top imports (including from
app.llm.utils import LLMResponse) and delete the LLMResponse import so only
necessary imports remain; after removing, run linters/tests to confirm there are
no remaining references to LLMResponse in this module and adjust any imports if
needed.
In `@backend/app/llm/providers/gemini_provider.py`:
- Around line 15-20: The ChatGoogleGenerativeAI instantiation in
gemini_provider.py passes temperature twice (once explicitly and once via
**kwargs), causing a TypeError; modify the factory that constructs the client
(the code that returns ChatGoogleGenerativeAI) to pop or remove "temperature"
from kwargs before expanding **kwargs (or only pass temperature via kwargs and
remove the explicit temperature argument) — apply the same change to the
analogous constructors in OpenAIProvider and GroqProvider to avoid duplicate
keyword arguments.
In `@backend/app/llm/providers/groq_provider.py`:
- Around line 15-20: The call to ChatGroq is passing temperature both explicitly
and via **kwargs, which can raise TypeError; before spreading kwargs into
ChatGroq (the call that constructs ChatGroq in this module), remove temperature
from kwargs (e.g., use kwargs.pop("temperature", default) or build a new dict
that excludes "temperature") so it is only passed once; update the code around
the ChatGroq(...) invocation to pass temperature explicitly and spread the
cleaned kwargs.
In `@backend/app/llm/providers/openai_provider.py`:
- Around line 15-19: The ChatOpenAI constructor call in openai_provider.py
passes temperature both explicitly and via **kwargs which can raise TypeError;
fix by removing temperature from kwargs before the call (use
kwargs.pop("temperature", kwargs.get("temperature", 0.7)) or simply temp =
kwargs.pop("temperature", 0.7) and then pass temperature=temp to ChatOpenAI in
the create method/function that instantiates ChatOpenAI), and apply the same
kwargs.pop pattern to the analogous instantiation sites in GroqProvider and
GeminiProvider to prevent duplicate keyword arguments.
In `@backend/app/llm/utils.py`:
- Around line 1-2: Remove the unused Optional import from the top of
backend/app/llm/utils.py: the current import line "from typing import Any, Dict,
Optional" should only import the types actually used (Any, Dict) so update the
import to remove Optional; check for any references to Optional in
functions/classes like BaseModel usages and only restore it if genuinely needed.
In `@README.md`:
- Around line 150-170: Update the Environment Configuration and Example .env
sections to document the new pluggable LLM layer by listing the new environment
variables LLM_PROVIDER and LLM_MODEL and adding entries for provider-specific
API keys (e.g., OPENAI_API_KEY, GEMINI_API_KEY) and any provider-specific model
names; ensure the Backend example .env shows these new keys alongside
GROQ_API_KEY/PINECONE_*/GOOGLE_* and add a short note explaining how to set
LLM_PROVIDER to choose between providers (e.g., "groq", "openai", "gemini") and
to set LLM_MODEL to the desired model identifier for the chosen provider.
🧹 Nitpick comments (3)
backend/app/llm/manager.py (1)
35-38: Avoid catching allExceptiontypes for fallback.Catching
Exceptioncan mask unexpected programming errors and make failures harder to detect. Consider narrowing to init-related errors (e.g.,ValueError/RuntimeError) and re-raising anything else.♻️ Possible refinement
- except Exception as e: + except (ValueError, RuntimeError) as e: logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}") return self._fallback(**kwargs) ... - except Exception as e: + except (ValueError, RuntimeError) as e: logger.debug(f"Fallback provider '{name}' failed: {e}") continueAlso applies to: 56-57
backend/app/modules/langgraph_nodes/judge.py (1)
26-31: Renamegroq_llmto reflect dynamic provider selection.With the manager in place, the instance may not be Groq. A neutral name avoids confusion.
💡 Suggested rename
-llm_manager = LLMManager() -groq_llm = llm_manager.get_llm(temperature=0.0, max_tokens=10) +llm_manager = LLMManager() +judge_llm = llm_manager.get_llm(temperature=0.0, max_tokens=10) ... - response = groq_llm.invoke([HumanMessage(content=prompt)]) + response = judge_llm.invoke([HumanMessage(content=prompt)])Also applies to: 49-49
backend/tests/test_llm_pluggability.py (1)
14-16: Guardlogging.basicConfigto avoid global test-side effects.Calling
basicConfigat import time can override the logging setup of other tests. Guard it or move it into a test runner setup.🧩 Safer setup
-logging.basicConfig(level=logging.INFO) +root_logger = logging.getLogger() +if not root_logger.handlers: + logging.basicConfig(level=logging.INFO)
| import os | ||
| from pydantic_settings import BaseSettings, SettingsConfigDict |
There was a problem hiding this comment.
Unused import: os is not used.
Proposed fix
-import os
from pydantic_settings import BaseSettings, SettingsConfigDict📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import os | |
| from pydantic_settings import BaseSettings, SettingsConfigDict | |
| from pydantic_settings import BaseSettings, SettingsConfigDict |
🤖 Prompt for AI Agents
In `@backend/app/config.py` around lines 1 - 2, Remove the unused import "os" from
the top of the module: the current import list includes "import os" but no code
references the symbol; update the import block to only import what is used
(e.g., keep "from pydantic_settings import BaseSettings, SettingsConfigDict") to
eliminate the unused import warning.
| target_provider = provider_name or self.primary_provider | ||
|
|
||
| try: | ||
| return self._create_llm(target_provider, **kwargs) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}") | ||
| return self._fallback(**kwargs) | ||
|
|
||
| def _create_llm(self, provider_name: str, **kwargs) -> BaseChatModel: | ||
| provider = self.get_provider(provider_name) | ||
| return provider.get_llm(**kwargs) | ||
|
|
||
| def _fallback(self, **kwargs) -> BaseChatModel: | ||
| """ | ||
| Simple fallback strategy: try all other available providers. | ||
| """ | ||
| for name in self._providers.keys(): | ||
| if name == self.primary_provider: | ||
| continue | ||
|
|
||
| try: | ||
| logger.info(f"Attempting fallback to provider: {name}") | ||
| return self._create_llm(name, **kwargs) |
There was a problem hiding this comment.
Exclude the failed provider from fallback attempts.
When provider_name differs from self.primary_provider, _fallback skips only the primary and may retry the failed provider while skipping a healthy primary. Pass the failed provider into _fallback and exclude it instead.
🛠️ Suggested adjustment
- try:
- return self._create_llm(target_provider, **kwargs)
- except Exception as e:
- logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}")
- return self._fallback(**kwargs)
+ try:
+ return self._create_llm(target_provider, **kwargs)
+ except Exception as e:
+ logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}")
+ return self._fallback(exclude={target_provider}, **kwargs)
- def _fallback(self, **kwargs) -> BaseChatModel:
+ def _fallback(self, exclude: set[str] | None = None, **kwargs) -> BaseChatModel:
"""
Simple fallback strategy: try all other available providers.
"""
+ exclude = exclude or set()
for name in self._providers.keys():
- if name == self.primary_provider:
+ if name in exclude:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_provider = provider_name or self.primary_provider | |
| try: | |
| return self._create_llm(target_provider, **kwargs) | |
| except Exception as e: | |
| logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}") | |
| return self._fallback(**kwargs) | |
| def _create_llm(self, provider_name: str, **kwargs) -> BaseChatModel: | |
| provider = self.get_provider(provider_name) | |
| return provider.get_llm(**kwargs) | |
| def _fallback(self, **kwargs) -> BaseChatModel: | |
| """ | |
| Simple fallback strategy: try all other available providers. | |
| """ | |
| for name in self._providers.keys(): | |
| if name == self.primary_provider: | |
| continue | |
| try: | |
| logger.info(f"Attempting fallback to provider: {name}") | |
| return self._create_llm(name, **kwargs) | |
| target_provider = provider_name or self.primary_provider | |
| try: | |
| return self._create_llm(target_provider, **kwargs) | |
| except Exception as e: | |
| logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}") | |
| return self._fallback(exclude={target_provider}, **kwargs) | |
| def _create_llm(self, provider_name: str, **kwargs) -> BaseChatModel: | |
| provider = self.get_provider(provider_name) | |
| return provider.get_llm(**kwargs) | |
| def _fallback(self, exclude: set[str] | None = None, **kwargs) -> BaseChatModel: | |
| """ | |
| Simple fallback strategy: try all other available providers. | |
| """ | |
| exclude = exclude or set() | |
| for name in self._providers.keys(): | |
| if name in exclude: | |
| continue | |
| try: | |
| logger.info(f"Attempting fallback to provider: {name}") | |
| return self._create_llm(name, **kwargs) |
🧰 Tools
🪛 Ruff (0.14.13)
37-37: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@backend/app/llm/manager.py` around lines 33 - 55, Modify the fallback logic
so the provider that just failed is excluded from retry attempts: update the
call site in the exception handler that currently does return
self._fallback(**kwargs) to pass the failed provider name (e.g., return
self._fallback(failed_provider=target_provider, **kwargs)), then change the
_fallback signature to def _fallback(self, failed_provider: Optional[str] =
None, **kwargs) -> BaseChatModel and ensure inside _fallback you skip both
self.primary_provider and failed_provider when iterating self._providers (refer
to _create_llm and _fallback to locate code).
| return ChatOpenAI( | ||
| model=model, | ||
| api_key=api_key, | ||
| temperature=kwargs.get("temperature", 0.7), | ||
| **kwargs |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/app/llm/providers/openai_provider.py | head -50Repository: AOSSIE-Org/Perspective
Length of output: 924
🌐 Web query:
langchain_openai ChatOpenAI temperature parameter signature
💡 Result:
In LangChain's ChatOpenAI (Python) the temperature parameter is a float (optional) passed to the constructor as temperature. Example:
- Signature / type: temperature: float | None
- Usage: ChatOpenAI(model="gpt-4o", temperature=0.2)
References: [1], [2].
Sources
- LangChain ChatOpenAI reference (shows temperature: float | None). [1]
- LangChain docs example of using ChatOpenAI with temperature parameter. [2]
🏁 Script executed:
python3 << 'EOF'
# Simulate the issue
def test_function(model, api_key, temperature, **kwargs):
print(f"model={model}, api_key={api_key}, temperature={temperature}, kwargs={kwargs}")
# Test case 1: Using get() - will cause duplicate keyword error
kwargs = {"temperature": 0.5, "other_param": "value"}
try:
result = test_function(
model="gpt-4o",
api_key="key",
temperature=kwargs.get("temperature", 0.7),
**kwargs
)
print("Test 1 (get): SUCCESS - no error")
except TypeError as e:
print(f"Test 1 (get): ERROR - {e}")
# Test case 2: Using pop() - should work fine
kwargs = {"temperature": 0.5, "other_param": "value"}
try:
temperature = kwargs.pop("temperature", 0.7)
result = test_function(
model="gpt-4o",
api_key="key",
temperature=temperature,
**kwargs
)
print("Test 2 (pop): SUCCESS - no error")
except TypeError as e:
print(f"Test 2 (pop): ERROR - {e}")
# Test case 3: Temperature not in kwargs with get()
kwargs = {"other_param": "value"}
try:
result = test_function(
model="gpt-4o",
api_key="key",
temperature=kwargs.get("temperature", 0.7),
**kwargs
)
print("Test 3 (get, no temp): SUCCESS - no error")
except TypeError as e:
print(f"Test 3 (get, no temp): ERROR - {e}")
EOFRepository: AOSSIE-Org/Perspective
Length of output: 152
🏁 Script executed:
find . -name "*test*" -type f | grep -i openaiRepository: AOSSIE-Org/Perspective
Length of output: 48
🏁 Script executed:
fd test -t f | head -20Repository: AOSSIE-Org/Perspective
Length of output: 104
🏁 Script executed:
cat -n backend/tests/test_llm_pluggability.pyRepository: AOSSIE-Org/Perspective
Length of output: 5213
🏁 Script executed:
rg "get_llm" -A 3 -B 1 --type py | head -60Repository: AOSSIE-Org/Perspective
Length of output: 4191
🏁 Script executed:
cat -n backend/app/llm/manager.py | head -40Repository: AOSSIE-Org/Perspective
Length of output: 1887
🏁 Script executed:
rg "get_llm\s*\(" -A 2 backend --type py | grep -A 2 "temperature"Repository: AOSSIE-Org/Perspective
Length of output: 555
🏁 Script executed:
cat -n backend/app/llm/providers/groq_provider.pyRepository: AOSSIE-Org/Perspective
Length of output: 929
🏁 Script executed:
cat -n backend/app/llm/providers/gemini_provider.pyRepository: AOSSIE-Org/Perspective
Length of output: 969
Use kwargs.pop() to prevent duplicate temperature keyword argument.
temperature is explicitly extracted but still forwarded via **kwargs. When a caller passes temperature, it will appear twice in the function call, raising TypeError. This occurs in active production code (e.g., judge.py, generate_perspective.py). Use pop() to remove the key from kwargs before spreading.
🐛 Proposed fix
- return ChatOpenAI(
- model=model,
- api_key=api_key,
- temperature=kwargs.get("temperature", 0.7),
- **kwargs
- )
+ temperature = kwargs.pop("temperature", 0.7)
+ return ChatOpenAI(
+ model=model,
+ api_key=api_key,
+ temperature=temperature,
+ **kwargs,
+ )Note: The same issue exists in GroqProvider and GeminiProvider and should be fixed there as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ChatOpenAI( | |
| model=model, | |
| api_key=api_key, | |
| temperature=kwargs.get("temperature", 0.7), | |
| **kwargs | |
| temperature = kwargs.pop("temperature", 0.7) | |
| return ChatOpenAI( | |
| model=model, | |
| api_key=api_key, | |
| temperature=temperature, | |
| **kwargs, | |
| ) |
🤖 Prompt for AI Agents
In `@backend/app/llm/providers/openai_provider.py` around lines 15 - 19, The
ChatOpenAI constructor call in openai_provider.py passes temperature both
explicitly and via **kwargs which can raise TypeError; fix by removing
temperature from kwargs before the call (use kwargs.pop("temperature",
kwargs.get("temperature", 0.7)) or simply temp = kwargs.pop("temperature", 0.7)
and then pass temperature=temp to ChatOpenAI in the create method/function that
instantiates ChatOpenAI), and apply the same kwargs.pop pattern to the analogous
instantiation sites in GroqProvider and GeminiProvider to prevent duplicate
keyword arguments.
| from pydantic import BaseModel, Field | ||
| from typing import Any, Dict, Optional |
There was a problem hiding this comment.
Unused import: Optional is not used.
Proposed fix
from pydantic import BaseModel, Field
-from typing import Any, Dict, Optional
+from typing import Any, Dict📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from pydantic import BaseModel, Field | |
| from typing import Any, Dict, Optional | |
| from pydantic import BaseModel, Field | |
| from typing import Any, Dict |
🤖 Prompt for AI Agents
In `@backend/app/llm/utils.py` around lines 1 - 2, Remove the unused Optional
import from the top of backend/app/llm/utils.py: the current import line "from
typing import Any, Dict, Optional" should only import the types actually used
(Any, Dict) so update the import to remove Optional; check for any references to
Optional in functions/classes like BaseModel usages and only restore it if
genuinely needed.
| ### 1. Environment Configuration | ||
| Both the frontend and backend require environment files. You will find `.env.example` templates in each directory. | ||
| - **Frontend**: Create `frontend/.env` and set `NEXT_PUBLIC_API_URL`. | ||
| - **Backend**: Create `backend/.env` and provide your `GROQ_API_KEY`, `PINECONE_API_KEY`, `PINECONE_INDEX_NAME`, `GOOGLE_SEARCH_API_KEY`, and `GOOGLE_SEARCH_ENGINE_ID`. | ||
|
|
||
| #### How to Obtain API Keys | ||
| - **Groq API Key**: Sign up at [Groq Console](https://console.groq.com) and create an API key. | ||
| - **Pinecone**: Create an index at [Pinecone Console](https://app.pinecone.io) to get your API Key and Index Name. | ||
| - **Google Custom Search**: | ||
| 1. **API Key**: Go to [Google Cloud Console](https://console.cloud.google.com), create a project, enable the "Custom Search API", and create credentials (API Key). | ||
| 2. **Search Engine ID**: Go to [Programmable Search Engine](https://programmablesearchengine.google.com), create a search engine (select "Search the entire web"), and copy the "Search engine ID" (cx). | ||
|
|
||
| ### Example .env Files | ||
| **Backend** (`backend/.env`) | ||
| ```env | ||
| GROQ_API_KEY=your_groq_api_key | ||
| PINECONE_API_KEY=your_pinecone_api_key | ||
| PINECONE_INDEX_NAME=your_index_name | ||
| GOOGLE_SEARCH_API_KEY=your_google_api_key | ||
| GOOGLE_SEARCH_ENGINE_ID=your_search_engine_id | ||
| ``` |
There was a problem hiding this comment.
Document new provider selector and API keys.
The new pluggable LLM layer adds LLM_PROVIDER/LLM_MODEL and additional keys (OpenAI/Gemini). The setup section and example .env should mention these so users can enable non‑Groq providers.
📚 Suggested doc update
-- **Backend**: Create `backend/.env` and provide your `GROQ_API_KEY`, `PINECONE_API_KEY`, `PINECONE_INDEX_NAME`, `GOOGLE_SEARCH_API_KEY`, and `GOOGLE_SEARCH_ENGINE_ID`.
+- **Backend**: Create `backend/.env` and provide `PINECONE_API_KEY`, `PINECONE_INDEX_NAME`, `GOOGLE_SEARCH_API_KEY`, and `GOOGLE_SEARCH_ENGINE_ID`, plus at least one LLM key (`GROQ_API_KEY` / `OPENAI_API_KEY` / `GOOGLE_API_KEY`). Optionally set `LLM_PROVIDER` and `LLM_MODEL`.
@@
-GROQ_API_KEY=your_groq_api_key
+LLM_PROVIDER=groq
+LLM_MODEL=llama-3.3-70b-versatile
+GROQ_API_KEY=your_groq_api_key
+OPENAI_API_KEY=your_openai_api_key
+GOOGLE_API_KEY=your_gemini_api_key
PINECONE_API_KEY=your_pinecone_api_key
PINECONE_INDEX_NAME=your_index_name
GOOGLE_SEARCH_API_KEY=your_google_api_key
GOOGLE_SEARCH_ENGINE_ID=your_search_engine_id🤖 Prompt for AI Agents
In `@README.md` around lines 150 - 170, Update the Environment Configuration and
Example .env sections to document the new pluggable LLM layer by listing the new
environment variables LLM_PROVIDER and LLM_MODEL and adding entries for
provider-specific API keys (e.g., OPENAI_API_KEY, GEMINI_API_KEY) and any
provider-specific model names; ensure the Backend example .env shows these new
keys alongside GROQ_API_KEY/PINECONE_*/GOOGLE_* and add a short note explaining
how to set LLM_PROVIDER to choose between providers (e.g., "groq", "openai",
"gemini") and to set LLM_MODEL to the desired model identifier for the chosen
provider.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/llm/manager.py`:
- Around line 46-48: The fallback logic currently only skips
self.primary_provider, so when a non-primary provider (variable target_provider
or provider_name) fails we should pass that failed provider into the fallback
and ensure _fallback excludes it; update the except blocks (the one catching
Exception around target_provider and the other at lines ~75-88) to call
self._fallback(exclude_providers={target_provider}, **kwargs) (or pass a
failed_provider param) and modify the _fallback method to accept an
exclude_providers set and filter out any provider in that set in addition to
self.primary_provider before attempting retries.
In `@backend/verify_llm_fixes.py`:
- Around line 10-11: The sys.path insertion is incorrect: instead of navigating
up twice then back into 'backend', replace sys.path.insert(0,
os.path.join(os.path.dirname(__file__), '..', '..', 'backend')) with a direct
insertion of the script's directory (e.g., sys.path.insert(0,
os.path.abspath(os.path.dirname(__file__)))) so imports resolve relative to this
file; update the call sites using sys.path.insert, os.path.join and __file__
accordingly.
- Around line 33-58: In test_cross_provider_model_isolation, capture original
os.environ values for 'LLM_MODEL' and 'LLM_PROVIDER' (via os.environ.get) before
mutating them, then perform the test as written (setting env vars,
importlib.reload(config), instantiating LLMManager and calling
manager.get_llm("groq")), and finally restore the environment in a finally
block: if an original value was None delete the key, otherwise reset it to the
original string; ensure restoration happens regardless of return/exception so
later tests (e.g., test_runtime_fallback_exists) are not polluted.
♻️ Duplicate comments (1)
backend/app/config.py (1)
1-2: Unused import:osis not used.The
osmodule is imported but not referenced anywhere in this file.Proposed fix
-import os from pydantic_settings import BaseSettings, SettingsConfigDict
🧹 Nitpick comments (5)
backend/app/llm/providers/base.py (1)
1-3: Unused import:Anyis not referenced in this file.The
Anytype is imported but never used. Consider removing it to keep imports clean.Proposed fix
from abc import ABC, abstractmethod -from typing import Any, Optional +from typing import Optional from langchain_core.language_models.chat_models import BaseChatModelbackend/app/llm/manager.py (2)
13-17: Annotate class-level mutable attribute withClassVar.The
_providersdictionary is a class-level mutable attribute shared across all instances. UsingClassVarmakes this intent explicit and satisfies type checkers.Proposed fix
+from typing import ClassVar, Dict, Type, Optional -from typing import Dict, Type, Optional ... class LLMManager: - _providers: Dict[str, Type[BaseLLMProvider]] = { + _providers: ClassVar[Dict[str, Type[BaseLLMProvider]]] = { "openai": OpenAIProvider, "groq": GroqProvider, "gemini": GeminiProvider, }
54-73:_build_fallbacksshould also exclude the primary provider consistently.Currently
_build_fallbackscorrectly excludesprimary_provider, but_fallbackexcludesself.primary_provider. These should use consistent logic. Also, consider whether_build_fallbacksshould strip additional provider-specific kwargs beyond justmodel_name(e.g., provider-specific parameters that might not apply across providers).backend/verify_llm_fixes.py (2)
21-21: Unused variablellm.The variable is assigned but never used. Since the test only verifies no
TypeErroris raised, you can discard the result with_:- llm = manager.get_llm("openai", temperature=0.5) + _ = manager.get_llm("openai", temperature=0.5)
72-78: Source-code inspection is fragile.Checking
'with_fallbacks' in sourcecould produce false positives (e.g., comments) or break on refactors. For a verification script this is acceptable, but consider callingget_llm()and verifying the returned object has fallbacks configured (if the API exposes this) for a more robust behavioral test.
| except Exception as e: | ||
| logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}") | ||
| return self._fallback(**kwargs) |
There was a problem hiding this comment.
Exclude the actually-failed provider from fallback attempts, not just primary_provider.
When provider_name is explicitly passed and differs from self.primary_provider, _fallback skips only the primary provider but may retry the provider that just failed. This can cause redundant failures and delay successful fallback.
Proposed fix
except Exception as e:
logger.warning(f"Failed to initialize primary provider '{target_provider}': {e}")
- return self._fallback(**kwargs)
+ return self._fallback(exclude={target_provider}, **kwargs)
- def _fallback(self, **kwargs) -> BaseChatModel:
+ def _fallback(self, exclude: set[str] | None = None, **kwargs) -> BaseChatModel:
"""
Simple fallback strategy: try all other available providers.
"""
+ exclude = exclude or set()
for name in self._providers.keys():
- if name == self.primary_provider:
+ if name in exclude:
continueAlso applies to: 75-88
🧰 Tools
🪛 Ruff (0.14.13)
46-46: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@backend/app/llm/manager.py` around lines 46 - 48, The fallback logic
currently only skips self.primary_provider, so when a non-primary provider
(variable target_provider or provider_name) fails we should pass that failed
provider into the fallback and ensure _fallback excludes it; update the except
blocks (the one catching Exception around target_provider and the other at lines
~75-88) to call self._fallback(exclude_providers={target_provider}, **kwargs)
(or pass a failed_provider param) and modify the _fallback method to accept an
exclude_providers set and filter out any provider in that set in addition to
self.primary_provider before attempting retries.
| # Add backend to path | ||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..', 'backend')) |
There was a problem hiding this comment.
Incorrect sys.path manipulation for the script's location.
For a file at backend/verify_llm_fixes.py, os.path.dirname(__file__) yields backend/, so '..', '..', 'backend' navigates outside the repository. Since app is inside backend/, you likely just need:
# Add backend to path
-sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..', 'backend'))
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add backend to path | |
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..', 'backend')) | |
| # Add backend to path | |
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) |
🤖 Prompt for AI Agents
In `@backend/verify_llm_fixes.py` around lines 10 - 11, The sys.path insertion is
incorrect: instead of navigating up twice then back into 'backend', replace
sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..',
'backend')) with a direct insertion of the script's directory (e.g.,
sys.path.insert(0, os.path.abspath(os.path.dirname(__file__)))) so imports
resolve relative to this file; update the call sites using sys.path.insert,
os.path.join and __file__ accordingly.
| def test_cross_provider_model_isolation(): | ||
| """Test that global LLM_MODEL doesn't break other providers.""" | ||
| print("\nTEST 2: Cross-Provider Model Isolation") | ||
| try: | ||
| # Set global LLM_MODEL to an OpenAI model | ||
| os.environ['LLM_MODEL'] = 'gpt-4o' | ||
| os.environ['LLM_PROVIDER'] = 'groq' | ||
|
|
||
| # Force reload of settings | ||
| import importlib | ||
| from app import config | ||
| importlib.reload(config) | ||
|
|
||
| from app.llm.manager import LLMManager | ||
| manager = LLMManager() | ||
|
|
||
| # This should use Groq's default model, not crash | ||
| llm = manager.get_llm("groq") | ||
| print("✅ PASS: Groq provider works despite global LLM_MODEL=gpt-4o") | ||
| return True | ||
| except Exception as e: | ||
| if "gpt-4o" in str(e).lower(): | ||
| print(f"❌ FAIL: Groq tried to use global LLM_MODEL: {e}") | ||
| return False | ||
| print(f"⚠️ SKIP: Could not test (likely missing API key): {e}") | ||
| return True |
There was a problem hiding this comment.
Environment variables modified but never restored—risks polluting subsequent tests.
LLM_MODEL and LLM_PROVIDER are set but never cleaned up. This breaks test isolation and could affect test_runtime_fallback_exists() which runs afterward. Store original values and restore them in a finally block:
♻️ Suggested fix
def test_cross_provider_model_isolation():
"""Test that global LLM_MODEL doesn't break other providers."""
print("\nTEST 2: Cross-Provider Model Isolation")
+ original_model = os.environ.get('LLM_MODEL')
+ original_provider = os.environ.get('LLM_PROVIDER')
try:
# Set global LLM_MODEL to an OpenAI model
os.environ['LLM_MODEL'] = 'gpt-4o'
os.environ['LLM_PROVIDER'] = 'groq'
# Force reload of settings
import importlib
from app import config
importlib.reload(config)
from app.llm.manager import LLMManager
manager = LLMManager()
# This should use Groq's default model, not crash
- llm = manager.get_llm("groq")
+ _ = manager.get_llm("groq")
print("✅ PASS: Groq provider works despite global LLM_MODEL=gpt-4o")
return True
except Exception as e:
if "gpt-4o" in str(e).lower():
print(f"❌ FAIL: Groq tried to use global LLM_MODEL: {e}")
return False
print(f"⚠️ SKIP: Could not test (likely missing API key): {e}")
return True
+ finally:
+ # Restore original environment
+ if original_model is None:
+ os.environ.pop('LLM_MODEL', None)
+ else:
+ os.environ['LLM_MODEL'] = original_model
+ if original_provider is None:
+ os.environ.pop('LLM_PROVIDER', None)
+ else:
+ os.environ['LLM_PROVIDER'] = original_provider📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_cross_provider_model_isolation(): | |
| """Test that global LLM_MODEL doesn't break other providers.""" | |
| print("\nTEST 2: Cross-Provider Model Isolation") | |
| try: | |
| # Set global LLM_MODEL to an OpenAI model | |
| os.environ['LLM_MODEL'] = 'gpt-4o' | |
| os.environ['LLM_PROVIDER'] = 'groq' | |
| # Force reload of settings | |
| import importlib | |
| from app import config | |
| importlib.reload(config) | |
| from app.llm.manager import LLMManager | |
| manager = LLMManager() | |
| # This should use Groq's default model, not crash | |
| llm = manager.get_llm("groq") | |
| print("✅ PASS: Groq provider works despite global LLM_MODEL=gpt-4o") | |
| return True | |
| except Exception as e: | |
| if "gpt-4o" in str(e).lower(): | |
| print(f"❌ FAIL: Groq tried to use global LLM_MODEL: {e}") | |
| return False | |
| print(f"⚠️ SKIP: Could not test (likely missing API key): {e}") | |
| return True | |
| def test_cross_provider_model_isolation(): | |
| """Test that global LLM_MODEL doesn't break other providers.""" | |
| print("\nTEST 2: Cross-Provider Model Isolation") | |
| original_model = os.environ.get('LLM_MODEL') | |
| original_provider = os.environ.get('LLM_PROVIDER') | |
| try: | |
| # Set global LLM_MODEL to an OpenAI model | |
| os.environ['LLM_MODEL'] = 'gpt-4o' | |
| os.environ['LLM_PROVIDER'] = 'groq' | |
| # Force reload of settings | |
| import importlib | |
| from app import config | |
| importlib.reload(config) | |
| from app.llm.manager import LLMManager | |
| manager = LLMManager() | |
| # This should use Groq's default model, not crash | |
| _ = manager.get_llm("groq") | |
| print("✅ PASS: Groq provider works despite global LLM_MODEL=gpt-4o") | |
| return True | |
| except Exception as e: | |
| if "gpt-4o" in str(e).lower(): | |
| print(f"❌ FAIL: Groq tried to use global LLM_MODEL: {e}") | |
| return False | |
| print(f"⚠️ SKIP: Could not test (likely missing API key): {e}") | |
| return True | |
| finally: | |
| # Restore original environment | |
| if original_model is None: | |
| os.environ.pop('LLM_MODEL', None) | |
| else: | |
| os.environ['LLM_MODEL'] = original_model | |
| if original_provider is None: | |
| os.environ.pop('LLM_PROVIDER', None) | |
| else: | |
| os.environ['LLM_PROVIDER'] = original_provider |
🧰 Tools
🪛 Ruff (0.14.13)
50-50: Local variable llm is assigned to but never used
Remove assignment to unused variable llm
(F841)
52-52: Consider moving this statement to an else block
(TRY300)
53-53: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@backend/verify_llm_fixes.py` around lines 33 - 58, In
test_cross_provider_model_isolation, capture original os.environ values for
'LLM_MODEL' and 'LLM_PROVIDER' (via os.environ.get) before mutating them, then
perform the test as written (setting env vars, importlib.reload(config),
instantiating LLMManager and calling manager.get_llm("groq")), and finally
restore the environment in a finally block: if an original value was None delete
the key, otherwise reset it to the original string; ensure restoration happens
regardless of return/exception so later tests (e.g.,
test_runtime_fallback_exists) are not polluted.
|
Post-Merge Verification Summary – Pluggable LLM Provider Architecture
|
| import sys | ||
|
|
||
| # Add backend to path | ||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..', 'backend')) |
There was a problem hiding this comment.
Avoid modifying sys.path with a hard-coded relative path
|
|
||
| from app.llm.manager import LLMManager | ||
| from app.llm.providers.base import BaseLLMProvider | ||
| from langchain_core.messages import AIMessage |
There was a problem hiding this comment.
this import path is uncommon; LangChain typically exposes AIMessage from langchain.schema or langchain_core.schema. If this import fails, the tests will error at import time.
| # Should log warning and try secondary | ||
| llm = manager.get_llm() | ||
| # It should be the secondary provider's LLM | ||
| self.assertEqual(llm.provider_name, "secondary") |
There was a problem hiding this comment.
it is my assertion that manager.get_llm() returns the model (MockChatModel) instance — not a provider wrapper. test assumes that returned object has provider_name. Confirm actual LLMManager returns the LLM instance (provider.get_llm()).
|
@Kaustubh1204 I've not review the all code commited by you as it is very large commit, my recommendation is that add the feature in small chunks, so the reviewer and maintainer can easily check it |
|
Also add FEATURE REQUEST, BUG FIX - these type of labels before the PR title to know what this PR actually does in the code base. |
2 participants
This PR implements a pluggable LLM provider architecture for the backend, allowing dynamic switching between OpenAI, Groq, and Gemini providers. It ensures system flexibility, redundancy, and cost-efficiency while maintaining compatibility with existing workflows.
Key Features:
LLMManager + Provider Classes:
Central manager routes requests to the selected provider.
Provider classes for OpenAI, Groq, and Gemini implement a unified interface.
Dynamic Configuration:
API keys and default providers managed via config.py and Pydantic settings.
Switch providers at runtime using the LLM_PROVIDER environment variable.
Fallback Logic:
If the primary provider fails, requests automatically route to secondary providers.
Integration:
Updated generate_perspective.py and judge.py to use the manager instead of hardcoded providers.
Maintains LangChain/LangGraph workflow compatibility.
Mock Providers for Testing:
Added backend/tests/test_llm_pluggability.py.
MockProvider and MockChatModel simulate OpenAI, Groq, and Gemini responses.
Tests verify routing, fallback, and explicit provider selection without real API keys.
4 tests passed successfully (0.005s).
How to Test Locally:
cd backend
python -m unittest tests/test_llm_pluggability.py
This runs all tests safely without needing .env or real API keys.
Outcome:
Fully flexible backend for multiple LLM providers.
Reduced vendor lock-in and improved system resilience.
Safe, repeatable testing infrastructure for contributors and CI/CD.
Ready for future extension to additional providers or models.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.