Skip to content

fix: restore LLM model values in reapply_all_settings fallback (#1587)#2008

Open
hunterxtang wants to merge 2 commits into
mainfrom
fix/1587-llm-fallback-update-main
Open

fix: restore LLM model values in reapply_all_settings fallback (#1587)#2008
hunterxtang wants to merge 2 commits into
mainfrom
fix/1587-llm-fallback-update-main

Conversation

@hunterxtang

@hunterxtang hunterxtang commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1587 on main. Ports the reviewed version of this fix from #1932 (approved and merged into release-0.5.1, including @Wallgau's review feedback from that PR) back to main's package layout: the provider-name constants and _configured_provider_names helper land in src/api/settings/helpers.py, and the no-arg fallback's LLM loop lands in src/api/settings/langflow_sync.py. Supersedes the closed #1926, which targeted main but predates the review feedback. The test file is #1932's reviewed test file with only the import path adapted; the 3 LLM-specific tests fail on current main and pass with the fix.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed settings reapplication so saved AI provider selections are restored more reliably.
    • Improved fallback behavior to update all configured language model providers and keep the active provider’s model while leaving others unselected.
    • Embedding settings are now reapplied correctly for supported providers only, without affecting unrelated providers.

@github-actions github-actions Bot added backend 🔷 Issues related to backend services (OpenSearch, Langflow, APIs) tests bug 🔴 Something isn't working. labels Jul 2, 2026
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Refactors provider-priority selection in settings helpers into module-level constants, reworks the no-argument fallback path in _update_langflow_model_values to reapply model values across all configured LLM and embedding providers (not just embeddings), and adds regression tests for issue #1587.

Changes

Provider fallback fix

Layer / File(s) Summary
Provider-name constants and helper functions
src/api/settings/helpers.py
Introduces _LLM_PROVIDER_NAMES and _EMBEDDING_PROVIDER_NAMES module-level tuples and updates _first_configured_llm_provider/_first_configured_embedding_provider to iterate over them instead of inline lists.
Fallback reapplication logic in langflow_sync
src/api/settings/langflow_sync.py
Imports new provider-name constants and _configured_provider_names; reworks the no-argument path of _update_langflow_model_values to update all configured LLM providers (setting current provider's model, None for others) then all configured embedding providers similarly.
Regression tests for issue #1587
tests/unit/api/test_langflow_sync_bug_1587.py
Adds fixtures and tests verifying fallback updates all configured LLM providers with force_llm_update=True, preserves the active provider's model while nulling others, continues correct embedding reapplication, skips unconfigured providers, and is bypassed when explicit arguments are supplied.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant UpdateLangflowModelValues as _update_langflow_model_values
  participant Helpers as _configured_provider_names
  participant Langflow as change_langflow_model_value

  Caller->>UpdateLangflowModelValues: call with no arguments
  UpdateLangflowModelValues->>Helpers: get configured LLM provider names
  Helpers-->>UpdateLangflowModelValues: LLM provider list
  loop each configured LLM provider
    UpdateLangflowModelValues->>Langflow: change_langflow_model_value(llm_model, force_llm_update=True)
  end
  UpdateLangflowModelValues->>Helpers: get configured embedding provider names
  Helpers-->>UpdateLangflowModelValues: embedding provider list
  loop each configured embedding provider
    UpdateLangflowModelValues->>Langflow: change_langflow_model_value(embedding_model)
  end
Loading

Possibly related issues

Possibly related PRs

  • langflow-ai/openrag#1935: Both PRs modify src/api/settings/helpers.py's provider-selection logic affecting model-default fallback behavior.

Suggested labels: refactor

Suggested reviewers: mpawlow

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: restoring LLM model values in the reapply_all_settings fallback.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/1587-llm-fallback-update-main

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jul 2, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/api/settings/helpers.py (1)

32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: dedupe via _configured_provider_names.

Both helpers could delegate to the newly added _configured_provider_names to avoid duplicating the "configured" filtering logic.

♻️ Optional dedup
 def _first_configured_llm_provider(config, excluding: str) -> str:
     """Return the first configured LLM provider that isn't `excluding`."""
-    for p in _LLM_PROVIDER_NAMES:
-        if p != excluding and getattr(config.providers, p).configured:
-            return p
-    return "openai"
+    for p in _configured_provider_names(config, _LLM_PROVIDER_NAMES):
+        if p != excluding:
+            return p
+    return "openai"


 def _first_configured_embedding_provider(config, excluding: str) -> str:
     """Return the first configured embedding provider (openai/watsonx/ollama) that isn't `excluding`, or "" if none."""
-    for p in _EMBEDDING_PROVIDER_NAMES:
-        if p != excluding and getattr(config.providers, p).configured:
-            return p
-    return ""
+    for p in _configured_provider_names(config, _EMBEDDING_PROVIDER_NAMES):
+        if p != excluding:
+            return p
+    return ""
🤖 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 `@src/api/settings/helpers.py` around lines 32 - 42, The provider-selection
helpers duplicate the same “configured and not excluded” filtering logic; update
`_first_configured_llm_provider` and `_first_configured_embedding_provider` to
delegate to `_configured_provider_names` instead of iterating and checking
`configured` themselves. Keep the existing fallback behavior in each helper
(`openai` for LLMs and empty string for embeddings) while using the shared
helper to pick the first matching provider.
🤖 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 `@src/api/settings/helpers.py`:
- Around line 32-42: The provider-selection helpers duplicate the same
“configured and not excluded” filtering logic; update
`_first_configured_llm_provider` and `_first_configured_embedding_provider` to
delegate to `_configured_provider_names` instead of iterating and checking
`configured` themselves. Keep the existing fallback behavior in each helper
(`openai` for LLMs and empty string for embeddings) while using the shared
helper to pick the first matching provider.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b4953853-00dd-4980-91f3-b5d175f41046

📥 Commits

Reviewing files that changed from the base of the PR and between 4f104ec and 4c05267.

📒 Files selected for processing (3)
  • src/api/settings/helpers.py
  • src/api/settings/langflow_sync.py
  • tests/unit/api/test_langflow_sync_bug_1587.py

@github-actions github-actions Bot added the lgtm label Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend 🔷 Issues related to backend services (OpenSearch, Langflow, APIs) bug 🔴 Something isn't working. lgtm tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: reapply_all_settings() skips LLM sync in fallback branch

2 participants