Implement fallback LLM provider logic#248
Open
palakjaiswal16 wants to merge 1 commit into
Open
Conversation
|
@palakjaiswal16 is attempting to deploy a commit to the rishabhmishra0510-5147's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThis PR implements automatic fallback logic for multiple LLM providers. It adds a ChangesMulti-Provider Fallback Orchestration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/unit/ai/test_fallback_provider.py (1)
36-45: ⚡ Quick winAdd fallback stream-path tests to cover this new behavior.
DummyProvider.stream()is implemented, but this suite doesn’t currently verify fallback orchestration forFallbackProvider.stream(). Add at least one success-after-failure stream test (and ideally all-fail stream test) to lock in parity withcomplete()behavior.Proposed test additions
+@pytest.mark.asyncio +async def test_fallback_provider_stream_tries_next_provider_after_error(): + primary = DummyProvider("primary", should_fail=True) + fallback = DummyProvider("fallback") + provider = FallbackProvider([primary, fallback]) + + chunks = [chunk async for chunk in provider.stream("system", "user", DummyResponse)] + + assert chunks == ["ok from fallback"] + assert primary.calls == 1 + assert fallback.calls == 1 + assert provider.provider_name == "DummyProvider" + assert provider.model == "fallback-model" + + +@pytest.mark.asyncio +async def test_fallback_provider_stream_raises_after_all_providers_fail(): + provider = FallbackProvider([ + DummyProvider("primary", should_fail=True), + DummyProvider("fallback", should_fail=True), + ]) + + with pytest.raises(LLMProviderError): + [chunk async for chunk in provider.stream("system", "user", DummyResponse)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/unit/ai/test_fallback_provider.py` around lines 36 - 45, Add unit tests that exercise FallbackProvider.stream orchestration similar to the existing complete() tests: create two DummyProvider instances with .should_fail toggled so the first raises LLMProviderError from its stream() and the second yields "ok from ..." and assert the FallbackProvider.stream() AsyncIterator yields the expected string and that the failing provider's .calls incremented once and the succeeding provider was called; also add an all-fail stream test where every DummyProvider has should_fail=True and assert the FallbackProvider.stream() raises the propagated LLMProviderError and each provider's .calls was incremented. Use the same helper/setup used by existing tests and reference DummyProvider.stream, FallbackProvider.stream, and LLMProviderError when locating code to modify.backend/app/ai/providers/__init__.py (1)
207-213: 💤 Low valueConsider adding
openai_base_urlto Settings for configuration parity.The
openai_base_urlsetting referenced here is not defined inSettings(config.py), so thegetattrdefault will always be used. This works for standard OpenAI but prevents users from configuring custom endpoints (Azure OpenAI, proxies).Suggested addition to config.py
# In Settings class, add alongside other OpenAI settings: openai_base_url: str = "https://api.openai.com/v1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/ai/providers/__init__.py` around lines 207 - 213, The Settings class is missing an openai_base_url attribute while OpenAIProvider is instantiated using getattr(settings, "openai_base_url", ...), so add a new attribute openai_base_url: str = "https://api.openai.com/v1" to the Settings class (config) to allow configuration of custom endpoints (Azure/proxy); update any relevant type hints/tests that reference Settings and ensure OpenAIProvider construction (the code calling getattr(settings, "openai_base_url")) will use the configured value instead of always falling back to the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/ai/providers/__init__.py`:
- Around line 207-213: The Settings class is missing an openai_base_url
attribute while OpenAIProvider is instantiated using getattr(settings,
"openai_base_url", ...), so add a new attribute openai_base_url: str =
"https://api.openai.com/v1" to the Settings class (config) to allow
configuration of custom endpoints (Azure/proxy); update any relevant type
hints/tests that reference Settings and ensure OpenAIProvider construction (the
code calling getattr(settings, "openai_base_url")) will use the configured value
instead of always falling back to the default.
In `@backend/tests/unit/ai/test_fallback_provider.py`:
- Around line 36-45: Add unit tests that exercise FallbackProvider.stream
orchestration similar to the existing complete() tests: create two DummyProvider
instances with .should_fail toggled so the first raises LLMProviderError from
its stream() and the second yields "ok from ..." and assert the
FallbackProvider.stream() AsyncIterator yields the expected string and that the
failing provider's .calls incremented once and the succeeding provider was
called; also add an all-fail stream test where every DummyProvider has
should_fail=True and assert the FallbackProvider.stream() raises the propagated
LLMProviderError and each provider's .calls was incremented. Use the same
helper/setup used by existing tests and reference DummyProvider.stream,
FallbackProvider.stream, and LLMProviderError when locating code to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2b7716e-fdac-4734-a099-d89fb626075b
📒 Files selected for processing (4)
backend/app/ai/providers/__init__.pybackend/app/ai/service.pybackend/app/config.pybackend/tests/unit/ai/test_fallback_provider.py
rishabh0510rishabh
added
level:advanced
gssoc:approved
level:critical
and removed
level:advanced
labels
Owner
|
hey @palakjaiswal16 some tests are failing plese address them carefully |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implemented automatic fallback logic for LLM providers so the troubleshoot flow can try backup providers when the primary provider raises
LLMProviderError. This prevents the AI troubleshoot endpoint from failing immediately when a configured provider such as OpenRouter is unavailable.Related Issues
Fixes #194
Changes Made
FallbackProviderto try providers sequentially until one succeeds.ENVFORGE_LLM_PROVIDER_FALLBACKSsupport with default real-provider order:openrouter -> openai -> ollama.Verification
pytest tests/unit/aisuccessfullySafetyFilterDocumentation
docs/FEATURES.md(if adding a feature/profile)CHANGELOG.mdSummary by CodeRabbit
Release Notes