LCORE-1473: Tool RAG is always enabled#1326
Merged
Merged
Conversation
Contributor
WalkthroughThis PR modifies the RagConfiguration schema to change the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `pylint` configuration to improve the quality of Python code reviews.Add a pylint configuration file to your project to customize how CodeRabbit runs |
are-ces
force-pushed
the
fix-default-rag-tool
branch
from
2303f7a to
6c8c606
Compare
are-ces
force-pushed
the
fix-default-rag-tool
branch
from
6c8c606 to
c7704b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/models/config.py (1)
1720-1726: Field description is incomplete regarding fallback behavior.The description states "When omitted, all registered BYOK vector stores are used (backward compatibility)." However, per the docstring above (lines 1709-1710) and the implementation in
responses.py, this fallback only occurs when bothinlineandtoolare empty. Ifinlineis configured buttoolis omitted, tool RAG is disabled (not fallback to all stores).Consider updating the description for clarity:
📝 Suggested description update
tool: list[str] = Field( default_factory=list, title="Tool RAG IDs", description="RAG IDs made available to the LLM as a file_search tool. " f"Use '{constants.OKP_RAG_ID}' to include the OKP vector store. " - "When omitted, all registered BYOK vector stores are used (backward compatibility).", + "Empty by default (no tool RAG). When both inline and tool are empty, " + "all registered BYOK vector stores are used as fallback.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/config.py` around lines 1720 - 1726, The description for the tool Field in src/models/config.py is misleading about fallback behavior; update the Field description for tool to state that the fallback to "all registered BYOK vector stores" only occurs when both inline and tool are empty (i.e., if inline is set but tool is omitted, tool RAG is disabled), and make the wording consistent with the docstring and responses.py logic so readers understand the three-state behavior (explicit tool list, inline-only, or fallback when both are empty).docs/byok_guide.md (1)
310-310: Consider making empty-list semantics explicit here.Given
rag.toolnow defaults to[], this line would be even clearer as: iftoolis empty andinlineis empty, fallback uses all registered stores.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/byok_guide.md` at line 310, Update the sentence explaining fallback semantics to explicitly state empty-list behavior: clarify that since rag.tool defaults to [] an empty `tool` and an empty `inline` means the system falls back to using all registered stores (i.e., replace "If omitted" wording with an explicit condition like "If `tool` is empty (defaults to []) and `inline` is empty, all registered stores are used as fallback"). Reference `rag.tool` and `inline` so readers understand the empty-list default and the exact fallback condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi.json`:
- Around line 11035-11047: Update the RagConfiguration description to resolve
the contradiction about the "tool" field: clarify that "tool" defaults to [] (no
tool RAG) and does not by itself enable all BYOK stores, and then explicitly
state that the special-case fallback of registering all stores occurs only when
both "inline" and "tool" are empty; keep the note about the special ID "okp" and
references to byok_rag and constants "okp" intact so readers can find the unique
IDs and providers.
In `@src/models/config.py`:
- Line 10: Replace the import of Self from typing with Self from
typing_extensions: update the import line that currently reads "from typing
import Any, Literal, Optional, Self" to import Self from typing_extensions
instead, leaving other typing imports unchanged; then run a quick search to
ensure all `@model_validator` methods and their return annotations (instances
using Self) still type-check and update any import references if necessary.
---
Nitpick comments:
In `@docs/byok_guide.md`:
- Line 310: Update the sentence explaining fallback semantics to explicitly
state empty-list behavior: clarify that since rag.tool defaults to [] an empty
`tool` and an empty `inline` means the system falls back to using all registered
stores (i.e., replace "If omitted" wording with an explicit condition like "If
`tool` is empty (defaults to []) and `inline` is empty, all registered stores
are used as fallback"). Reference `rag.tool` and `inline` so readers understand
the empty-list default and the exact fallback condition.
In `@src/models/config.py`:
- Around line 1720-1726: The description for the tool Field in
src/models/config.py is misleading about fallback behavior; update the Field
description for tool to state that the fallback to "all registered BYOK vector
stores" only occurs when both inline and tool are empty (i.e., if inline is set
but tool is omitted, tool RAG is disabled), and make the wording consistent with
the docstring and responses.py logic so readers understand the three-state
behavior (explicit tool list, inline-only, or fallback when both are empty).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cc744c1-7c53-4d35-9f4d-259a72f0a91a
📒 Files selected for processing (7)
docs/byok_guide.mddocs/openapi.jsonsrc/models/config.pysrc/utils/responses.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_rag_configuration.pytests/unit/utils/test_responses.py
Comment on lines
11035
to
+11047
| "tool": { | ||
| "anyOf": [ | ||
| { | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "type": "array" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "type": "array", | ||
| "title": "Tool RAG IDs", | ||
| "description": "RAG IDs made available to the LLM as a file_search tool. Use 'okp' to include the OKP vector store. When omitted, all registered BYOK vector stores are used (backward compatibility)." | ||
| } | ||
| }, | ||
| "additionalProperties": false, | ||
| "type": "object", | ||
| "title": "RagConfiguration", | ||
| "description": "RAG strategy configuration.\n\nControls which RAG sources are used for inline and tool-based retrieval.\n\nEach strategy lists RAG IDs to include. The special ID ``\"okp\"`` defined in constants,\nactivates the OKP provider; all other IDs refer to entries in ``byok_rag``.\n\nBackward compatibility:\n - ``inline`` defaults to ``[]`` (no inline RAG).\n - ``tool`` defaults to ``None`` which means all registered vector stores\n are used (identical to the previous ``tool.byok.enabled = True`` default)." | ||
| "description": "RAG strategy configuration.\n\nControls which RAG sources are used for inline and tool-based retrieval.\n\nEach strategy lists RAG IDs to include. The special ID ``\"okp\"`` defined in constants,\nactivates the OKP provider; all other IDs refer to entries in ``byok_rag``.\n\nBackward compatibility:\n - ``inline`` defaults to ``[]`` (no inline RAG).\n - ``tool`` defaults to ``[]`` (no tool RAG).\n\nIf no RAG strategy is defined (inline and tool are empty),\nthe RAG tool will register all stores available to llama-stack." |
Contributor
There was a problem hiding this comment.
Resolve conflicting rag.tool behavior in the OpenAPI schema docs.
Line 11041 states that omitting tool enables all BYOK stores, but Line 11047 states tool defaults to [] and “all stores” is only a fallback when both inline and tool are empty. These semantics conflict and will mislead API/config consumers.
Suggested doc fix
"tool": {
"items": {
"type": "string"
},
"type": "array",
+ "default": [],
"title": "Tool RAG IDs",
- "description": "RAG IDs made available to the LLM as a file_search tool. Use 'okp' to include the OKP vector store. When omitted, all registered BYOK vector stores are used (backward compatibility)."
+ "description": "RAG IDs made available to the LLM as a file_search tool. Use 'okp' to include the OKP vector store. Defaults to an empty list (tool RAG disabled). If both rag.inline and rag.tool are empty, all stores available to llama-stack are registered as fallback."
}📝 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.
Suggested change
| "tool": { | |
| "anyOf": [ | |
| { | |
| "items": { | |
| "type": "string" | |
| }, | |
| "type": "array" | |
| }, | |
| { | |
| "type": "null" | |
| } | |
| ], | |
| "items": { | |
| "type": "string" | |
| }, | |
| "type": "array", | |
| "title": "Tool RAG IDs", | |
| "description": "RAG IDs made available to the LLM as a file_search tool. Use 'okp' to include the OKP vector store. When omitted, all registered BYOK vector stores are used (backward compatibility)." | |
| } | |
| }, | |
| "additionalProperties": false, | |
| "type": "object", | |
| "title": "RagConfiguration", | |
| "description": "RAG strategy configuration.\n\nControls which RAG sources are used for inline and tool-based retrieval.\n\nEach strategy lists RAG IDs to include. The special ID ``\"okp\"`` defined in constants,\nactivates the OKP provider; all other IDs refer to entries in ``byok_rag``.\n\nBackward compatibility:\n - ``inline`` defaults to ``[]`` (no inline RAG).\n - ``tool`` defaults to ``None`` which means all registered vector stores\n are used (identical to the previous ``tool.byok.enabled = True`` default)." | |
| "description": "RAG strategy configuration.\n\nControls which RAG sources are used for inline and tool-based retrieval.\n\nEach strategy lists RAG IDs to include. The special ID ``\"okp\"`` defined in constants,\nactivates the OKP provider; all other IDs refer to entries in ``byok_rag``.\n\nBackward compatibility:\n - ``inline`` defaults to ``[]`` (no inline RAG).\n - ``tool`` defaults to ``[]`` (no tool RAG).\n\nIf no RAG strategy is defined (inline and tool are empty),\nthe RAG tool will register all stores available to llama-stack." | |
| "tool": { | |
| "items": { | |
| "type": "string" | |
| }, | |
| "type": "array", | |
| "default": [], | |
| "title": "Tool RAG IDs", | |
| "description": "RAG IDs made available to the LLM as a file_search tool. Use 'okp' to include the OKP vector store. Defaults to an empty list (tool RAG disabled). If both rag.inline and rag.tool are empty, all stores available to llama-stack are registered as fallback." | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/openapi.json` around lines 11035 - 11047, Update the RagConfiguration
description to resolve the contradiction about the "tool" field: clarify that
"tool" defaults to [] (no tool RAG) and does not by itself enable all BYOK
stores, and then explicitly state that the special-case fallback of registering
all stores occurs only when both "inline" and "tool" are empty; keep the note
about the special ID "okp" and references to byok_rag and constants "okp" intact
so readers can find the unique IDs and providers.
| from pathlib import Path | ||
| from typing import Any, Optional, Literal, Self | ||
| from re import Pattern | ||
| from typing import Any, Literal, Optional, Self |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check minimum Python version in project configuration
echo "=== Checking pyproject.toml for Python version ==="
cat pyproject.toml 2>/dev/null | grep -A5 "python" | head -20
echo ""
echo "=== Checking setup.py/setup.cfg for Python version ==="
cat setup.py 2>/dev/null | grep -i python | head -10
cat setup.cfg 2>/dev/null | grep -i python | head -10
echo ""
echo "=== Checking if typing_extensions.Self was previously used ==="
git log -1 --format="%H" -- src/models/config.py 2>/dev/null
git show HEAD:src/models/config.py 2>/dev/null | head -15Repository: lightspeed-core/lightspeed-stack
Length of output: 865
🏁 Script executed:
# Check if Self import was added in this PR
echo "=== Check git diff for src/models/config.py ==="
git diff HEAD -- src/models/config.py 2>/dev/null | head -50
echo ""
echo "=== Check actual usage of Self in the file ==="
rg "Self" src/models/config.py -B2 -A2Repository: lightspeed-core/lightspeed-stack
Length of output: 4681
Change Self import to use typing_extensions.Self for consistency with coding guidelines.
The file imports Self from typing (line 10), but the coding guidelines specify using typing_extensions.Self for model validators in type annotations. This file has 18+ @model_validator methods that all use Self as return type, so update the import to comply with the guideline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/config.py` at line 10, Replace the import of Self from typing with
Self from typing_extensions: update the import line that currently reads "from
typing import Any, Literal, Optional, Self" to import Self from
typing_extensions instead, leaving other typing imports unchanged; then run a
quick search to ensure all `@model_validator` methods and their return annotations
(instances using Self) still type-check and update any import references if
necessary.
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
When
rag.inlineis configured in the lightspeed config but notrag.tool,the RAG Tool should be disabled. However, this is not the case as Tool RAG is always called.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Test by configuring inline rag and not tool rag, make a query and see that the tool calls will be empty.
Summary by CodeRabbit
Documentation
Tests