LCORE-642: Llama Stack configuration regeneration #651
Conversation
WalkthroughAdds conditional BYOK RAG handling to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant LlamaCFG as llama_stack_configuration
participant LS as ls_config
Caller->>LlamaCFG: generate_configuration(LS, byok_rag)
alt byok_rag empty
LlamaCFG->>LlamaCFG: log "No BYOK RAG configured"
LlamaCFG-->>Caller: return LS (unchanged)
else byok_rag present
rect rgb(236,248,255)
note right of LlamaCFG: BYOK processing flow
LlamaCFG->>LlamaCFG: construct_vector_dbs_section(LS, byok_rag)
LlamaCFG->>LlamaCFG: construct_vector_io_providers_section(LS, byok_rag)
LlamaCFG->>LlamaCFG: update LS.vector_dbs and LS.providers.vector_io
LlamaCFG->>LlamaCFG: log "Added X vector_dbs, Y providers/vector_io"
end
LlamaCFG-->>Caller: return updated LS
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llama_stack_configuration.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/llama_stack_configuration.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/llama_stack_configuration.py
🧬 Code graph analysis (1)
src/llama_stack_configuration.py (1)
src/models/config.py (4)
config(139-145)Configuration(567-590)ByokRag(554-564)dump(587-590)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
| "kvstore": { | ||
| "db_path": ".llama/" + brag.vector_db_id + ".db", | ||
| "namespace": None, |
There was a problem hiding this comment.
Use configured BYOK db_path instead of hard-coded location.
Hard-coding the SQLite file path discards the db_path supplied in ByokRag, so every generated config points to .llama/<vector_db_id>.db regardless of the actual BYOK location. That breaks BYOK setups that rely on their declared file path.
- "db_path": ".llama/" + brag.vector_db_id + ".db",
+ "db_path": str(brag.db_path),📝 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.
| "kvstore": { | |
| "db_path": ".llama/" + brag.vector_db_id + ".db", | |
| "namespace": None, | |
| "kvstore": { | |
| "db_path": str(brag.db_path), | |
| "namespace": None, |
🤖 Prompt for AI Agents
In src/llama_stack_configuration.py around lines 97 to 99, the kvstore db_path
is hard-coded to ".llama/<vector_db_id>.db" which ignores the ByokRag-provided
db_path; update the config to use the configured BYOK path (e.g., use
brag.db_path or the ByokRag.db_path attribute) when present, falling back to the
existing ".llama/<vector_db_id>.db" only if no custom path is provided; ensure
you preserve any relative/path joining semantics and handle None/empty values
safely before assigning to the "kvstore.db_path" field.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/test_llama_stack_configuration.py (2)
215-225: Use constant instead of hardcoding provider_type.The test hardcodes
"inline::faiss"forprovider_type, but this should reference theDEFAULT_RAG_TYPEconstant to ensure tests remain valid if the default changes. The same issue exists in lines 228, 279, and 290.First, re-add the import if you removed it per the earlier comment:
from constants import ( + DEFAULT_RAG_TYPE, DEFAULT_EMBEDDING_MODEL, DEFAULT_EMBEDDING_DIMENSION, )Then apply this pattern to replace hardcoded values:
assert output[0] == { "provider_id": "byok_vector_db_id_1", - "provider_type": "inline::faiss", + "provider_type": DEFAULT_RAG_TYPE, "config": { "kvstore": {Repeat for lines 228, 279, and 290.
301-318: Consider using tmp_path for correct type annotation.The
tmpdirfixture returnspy.path.local, notpathlib.Path. Modern pytest (7.0+) providestmp_pathwhich returnspathlib.Pathdirectly, aligning better with the type annotation and coding guidelines.Apply this pattern (repeat for other test functions):
-def test_generate_configuration_no_input_file(tmpdir: Path) -> None: +def test_generate_configuration_no_input_file(tmp_path: Path) -> None: """Test the function to generate configuration when input file does not exist.""" cfg = Configuration( name="test_name", service=ServiceConfiguration(), llama_stack=LlamaStackConfiguration( use_as_library_client=True, library_client_config_path="tests/configuration/run.yaml", api_key="whatever", ), user_data_collection=UserDataCollection( feedback_enabled=False, feedback_storage=None ), ) - outfile = tmpdir / "run.xml" + outfile = tmp_path / "run.xml"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_llama_stack_configuration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/test_llama_stack_configuration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/test_llama_stack_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/test_llama_stack_configuration.py
🧬 Code graph analysis (1)
tests/unit/test_llama_stack_configuration.py (2)
src/models/config.py (6)
config(139-145)ByokRag(554-564)Configuration(567-590)ServiceConfiguration(148-165)LlamaStackConfiguration(176-219)UserDataCollection(222-255)src/llama_stack_configuration.py (3)
generate_configuration(24-49)construct_vector_dbs_section(52-77)construct_vector_io_providers_section(80-110)
🪛 GitHub Actions: Python linter
tests/unit/test_llama_stack_configuration.py
[warning] 162-162: C0301: Line too long (111/100). Line exceeds maximum line length.
[warning] 170-170: C0301: Line too long (108/100). Line exceeds maximum line length.
[warning] 199-199: C0301: Line too long (111/100). Line exceeds maximum line length.
[warning] 240-240: C0301: Line too long (108/100). Line exceeds maximum line length.
[warning] 16-16: W0611: Unused DEFAULT_RAG_TYPE imported from constants (unused-import).
🪛 GitHub Actions: Ruff
tests/unit/test_llama_stack_configuration.py
[error] 17-17: Ruff: F401 'constants.DEFAULT_RAG_TYPE' imported but unused. Remove unused import or run with '--fix'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (2)
tests/unit/test_llama_stack_configuration.py (2)
29-158: LGTM: Comprehensive test coverage for vector_dbs section.The four test functions provide thorough coverage of
construct_vector_dbs_sectionbehavior across different scenarios: empty initialization, preserving existing data, appending BYOK entries, and full merging. The assertions correctly validate both the structure and content of the returned data.
301-380: LGTM: Thorough testing of generate_configuration.The three test functions provide good coverage of
generate_configurationbehavior: error handling for missing input, successful generation without BYOK, and successful generation with BYOK entries. The assertions correctly validate the presence of required sections in the generated YAML.
| from constants import ( | ||
| DEFAULT_RAG_TYPE, | ||
| DEFAULT_EMBEDDING_MODEL, | ||
| DEFAULT_EMBEDDING_DIMENSION, | ||
| ) |
There was a problem hiding this comment.
Remove unused import and use the constant in tests.
The DEFAULT_RAG_TYPE constant is imported but never used, causing linter failures. However, the tests hardcode "inline::faiss" in assertions (lines 217, 228, 279, 290) when they should reference this constant for maintainability.
Apply this diff to fix the unused import warning:
-from constants import (
- DEFAULT_RAG_TYPE,
- DEFAULT_EMBEDDING_MODEL,
- DEFAULT_EMBEDDING_DIMENSION,
-)
+from constants import (
+ DEFAULT_EMBEDDING_MODEL,
+ DEFAULT_EMBEDDING_DIMENSION,
+)Additionally, consider using DEFAULT_RAG_TYPE in test assertions instead of hardcoding "inline::faiss". This would require re-adding the import but using it consistently. See additional comment on lines 215-225 for details.
📝 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 constants import ( | |
| DEFAULT_RAG_TYPE, | |
| DEFAULT_EMBEDDING_MODEL, | |
| DEFAULT_EMBEDDING_DIMENSION, | |
| ) | |
| from constants import ( | |
| DEFAULT_EMBEDDING_MODEL, | |
| DEFAULT_EMBEDDING_DIMENSION, | |
| ) |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 16-16: W0611: Unused DEFAULT_RAG_TYPE imported from constants (unused-import).
🪛 GitHub Actions: Ruff
[error] 17-17: Ruff: F401 'constants.DEFAULT_RAG_TYPE' imported but unused. Remove unused import or run with '--fix'.
🤖 Prompt for AI Agents
In tests/unit/test_llama_stack_configuration.py around lines 16 to 20 and in the
assertion locations (~215-295), the DEFAULT_RAG_TYPE import is either unused or
the tests hardcode "inline::faiss"; either remove the unused import or
(preferred) re-add and use DEFAULT_RAG_TYPE in all assertions instead of the
literal "inline::faiss" (replace occurrences at lines ~217, 228, 279, 290),
ensuring the constant is imported at the top and referenced in assertions so the
linter warning is resolved and tests use the shared constant for
maintainability.
|
|
||
|
|
||
| def test_construct_vector_io_providers_section_init() -> None: | ||
| """Test the function construct_vector_io_providers_section for no vector_io_providers configured before.""" |
There was a problem hiding this comment.
Break long docstring to comply with line length limit.
The docstring exceeds 100 characters.
Apply this diff:
- """Test the function construct_vector_io_providers_section for no vector_io_providers configured before."""
+ """Test construct_vector_io_providers_section with no prior config.
+
+ Validates behavior when no vector_io_providers are configured before.
+ """📝 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.
| """Test the function construct_vector_io_providers_section for no vector_io_providers configured before.""" | |
| """Test construct_vector_io_providers_section with no prior config. | |
| Validates behavior when no vector_io_providers are configured before. | |
| """ |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 162-162: C0301: Line too long (111/100). Line exceeds maximum line length.
🤖 Prompt for AI Agents
In tests/unit/test_llama_stack_configuration.py around line 162, the docstring
for the test exceeds the 100-character line length limit; break the long
docstring into multiple shorter lines (or rewrite it to be a single shorter
sentence) so each line is <=100 characters, keeping the same meaning (e.g.,
split after a clause or rephrase to "Test construct_vector_io_providers_section
when no vector_io_providers are configured.").
|
|
||
|
|
||
| def test_construct_vector_io_providers_section_init_with_existing_data() -> None: | ||
| """Test the function construct_vector_io_providers_section for vector_io_providers configured before.""" |
There was a problem hiding this comment.
Break long docstring to comply with line length limit.
The docstring exceeds 100 characters.
Apply this diff:
- """Test the function construct_vector_io_providers_section for vector_io_providers configured before."""
+ """Test construct_vector_io_providers_section with existing config.
+
+ Validates behavior when vector_io_providers are already configured.
+ """📝 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.
| """Test the function construct_vector_io_providers_section for vector_io_providers configured before.""" | |
| """Test construct_vector_io_providers_section with existing config. | |
| Validates behavior when vector_io_providers are already configured. | |
| """ |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 170-170: C0301: Line too long (108/100). Line exceeds maximum line length.
🤖 Prompt for AI Agents
In tests/unit/test_llama_stack_configuration.py around line 170, the
module-level/test docstring is longer than the 100-character line length limit;
shorten or wrap it into multiple shorter lines so no single line exceeds 100
characters (for example split the sentence into two lines inside the
triple-quoted docstring or rephrase to a shorter sentence), keeping the same
content and punctuation.
|
|
||
|
|
||
| def test_construct_vector_io_providers_section_append() -> None: | ||
| """Test the function construct_vector_io_providers_section for no vector_io_providers configured before.""" |
There was a problem hiding this comment.
Break long docstring to comply with line length limit.
The docstring exceeds 100 characters.
Apply this diff:
- """Test the function construct_vector_io_providers_section for no vector_io_providers configured before."""
+ """Test construct_vector_io_providers_section append behavior.
+
+ Validates appending BYOK entries when no vector_io_providers exist.
+ """📝 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.
| """Test the function construct_vector_io_providers_section for no vector_io_providers configured before.""" | |
| """Test construct_vector_io_providers_section append behavior. | |
| Validates appending BYOK entries when no vector_io_providers exist. | |
| """ |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 199-199: C0301: Line too long (111/100). Line exceeds maximum line length.
🤖 Prompt for AI Agents
In tests/unit/test_llama_stack_configuration.py around line 199, the
module-level or test docstring is longer than 100 characters; break the long
docstring into multiple shorter lines (wrap at <=100 chars) to comply with the
line-length limit and maintain the same wording and punctuation across the
wrapped lines (e.g., split into two or three string literal lines or use a
multiline triple-quoted string with explicit line breaks).
|
|
||
|
|
||
| def test_construct_vector_io_providers_section_full_merge() -> None: | ||
| """Test the function construct_vector_io_providers_section for vector_io_providers configured before.""" |
There was a problem hiding this comment.
Break long docstring to comply with line length limit.
The docstring exceeds 100 characters.
Apply this diff:
- """Test the function construct_vector_io_providers_section for vector_io_providers configured before."""
+ """Test construct_vector_io_providers_section merge behavior.
+
+ Validates merging existing vector_io_providers with BYOK entries.
+ """📝 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.
| """Test the function construct_vector_io_providers_section for vector_io_providers configured before.""" | |
| """Test construct_vector_io_providers_section merge behavior. | |
| Validates merging existing vector_io_providers with BYOK entries. | |
| """ |
🧰 Tools
🪛 GitHub Actions: Python linter
[warning] 240-240: C0301: Line too long (108/100). Line exceeds maximum line length.
🤖 Prompt for AI Agents
In tests/unit/test_llama_stack_configuration.py around line 240, the
module-level docstring for the test exceeds the 100-character line length limit;
split the long docstring into multiple shorter lines (wrap at sensible word
boundaries) so no line exceeds 100 characters, keeping the same content and
punctuation and preserving the triple-quoted string format.
tisnik
force-pushed
the
lcore-642-llama-stack-configuration-regeneration
branch
from
4a7f9c8 to
86e21de
Compare
1 participant
Description
LCORE-642: Llama Stack configuration regeneration
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Improvements
Tests