adding the ability to work with llama_stack release-0.2.22 #44
Conversation
WalkthroughRelaxed exact dependency pins and added mypy options in pyproject.toml. Strengthened node-type checks and JSON/error handling in scripts/query_rag.py. Added provider_vector_db_id propagation and a local filesystem provider to LlamaStack YAML generation and switched to the core llama-stack client. Tests updated to validate YAML and provider_vector_db_id flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Processor as _LlamaStackDB.save
participant Config as write_yaml_config
participant Client as LlamaStackAsLibraryClient
participant VectorDB as vector_io.insert
User->>Processor: save(documents, index_id)
Processor->>Config: write_yaml_config(index_id, filename, db_file, provider_vector_db_id)
Config-->>Processor: YAML with provider_vector_db_id persisted
Processor->>Client: start client (llama_stack.core.library_client)
Client-->>Processor: client instance
Processor->>Client: vector_dbs.list()/select -> provider_vector_db_id
Processor->>VectorDB: insert(..., vector_db_id=provider_vector_db_id)
VectorDB-->>Processor: success
Processor-->>User: save complete
sequenceDiagram
participant User
participant QueryScript as scripts/query_rag.py
participant Index as llama-index search
participant Output as stdout/JSON
User->>QueryScript: run query
QueryScript->>Index: search
Index-->>QueryScript: result (single-node or multiple)
alt single-node
QueryScript->>QueryScript: require TextNode else warn/error
QueryScript->>Output: TextNode content or JSON error (exit 1)
else multi-node
QueryScript->>QueryScript: filter NodeWithScore entries only
QueryScript->>Output: list of {id, score, text, metadata}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
andrej1991
force-pushed
the
update_llama
branch
5 times, most recently
from
1b6456a to
a744ec6
Compare
andrej1991
force-pushed
the
update_llama
branch
2 times, most recently
from
da9ef91 to
5b19e46
Compare
![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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml(2 hunks)scripts/query_rag.py(4 hunks)src/lightspeed_rag_content/document_processor.py(6 hunks)tests/test_document_processor_llama_stack.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_document_processor_llama_stack.py (1)
src/lightspeed_rag_content/document_processor.py (5)
write_yaml_config(301-327)_LlamaStackDB(187-404)save(145-150)save(372-404)save(515-518)
⏰ 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-and-push-dev
- GitHub Check: Pylinter
andrej1991
force-pushed
the
update_llama
branch
3 times, most recently
from
376381d to
00885ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lightspeed_rag_content/document_processor.py (1)
1-1: Fix Black formatting to pass CI checks.The Black formatter reports this file needs reformatting. Please run
uv tool run black --write src/lightspeed_rag_content/document_processor.pyto fix the formatting issues.
🧹 Nitpick comments (1)
scripts/query_rag.py (1)
39-66: Consider usingsys.exit(1)for consistency.The enhanced single-node type checking and error handling is well-implemented. However, line 66 uses the built-in
exit(1)whilesysis already imported at line 8. For consistency and explicitness, consider usingsys.exit(1).Apply this diff:
) if args.json: result = { "query": args.query, "type": "single_node", "node_id": args.node, "error": f"Node is not a TextNode (type: {type(node).__name__})", } print(json.dumps(result, indent=2)) - exit(1) + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml(2 hunks)scripts/query_rag.py(4 hunks)src/lightspeed_rag_content/document_processor.py(6 hunks)tests/test_document_processor_llama_stack.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_document_processor_llama_stack.py
🧰 Additional context used
🪛 GitHub Actions: Black
src/lightspeed_rag_content/document_processor.py
[error] 1-1: Black formatting check failed. This file would be reformatted by running 'uv tool run black --write'.
⏰ 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-and-push-dev
- GitHub Check: Pylinter
🔇 Additional comments (10)
pyproject.toml (2)
23-25: LGTM: Mypy configuration enhances type checking.The added mypy configuration options properly configure type checking for the project structure with the src layout.
40-52: Verify the broader dependency relaxation aligns with project requirements.The git diff confirms this PR relaxes version constraints for most dependencies (PyYAML, llama-index packages, faiss-cpu, torch, aiosqlite, sqlite-vec) from exact pins (
==) to minimum versions (>=), while keeping llama-stack and llama-stack-client pinned to 0.2.22. The commit message states "adding the ability to work with llama_stack release-0.2.22," but doesn't explain the rationale for the broader relaxation.This systematic change represents a significant shift in dependency management policy. Please clarify:
- Is this broader relaxation intentional, or should it be limited to llama-stack packages only?
- Have these relaxed dependencies been tested for compatibility with their latest versions?
- Should upper bound constraints be considered (e.g.,
>=X.Y.Z,<X+1.0.0) to prevent major version jumps?scripts/query_rag.py (3)
10-10: LGTM: Type safety imports improve code robustness.The additions of
cast,NodeWithScore, andTextNodeimports support enhanced type checking and safer node handling throughout the script.Also applies to: 15-15
106-121: LGTM: Robust multi-node filtering with proper type handling.The implementation correctly filters nodes to only process
NodeWithScoreinstances, with defensive text extraction that falls back tobase_node.node.get_content()when needed. Debug logging for skipped nodes aids troubleshooting.
161-161: LGTM: Client import path updated for llama-stack 0.2.22.The client import path correctly reflects the library reorganization from
distribution.library_clienttocore.library_clientintroduced in llama-stack 0.2.22.src/lightspeed_rag_content/document_processor.py (5)
189-238: LGTM: YAML template updated for llama-stack 0.2.22 configuration structure.The template additions correctly incorporate the new
filesAPI, localfs provider, andprovider_vector_db_idplaceholder to align with llama-stack's updated configuration schema.
301-307: LGTM: Backward-compatible signature enhancement.The addition of the optional
provider_vector_db_idparameter maintains backward compatibility while supporting the new llama-stack configuration requirements.
295-295: LGTM: Client class path updated for llama-stack 0.2.22.The client class path correctly reflects the reorganization from
distribution.library_clienttocore.library_clientin llama-stack 0.2.22.
325-325: Verify YAML handling when provider_vector_db_id is None.When
provider_vector_db_idisNone, the template substitution on line 237 will insert an empty string, resulting in a blank line in the generated YAML. Please verify:
- Is a blank line acceptable in the llama-stack YAML configuration?
- Should the template conditionally omit the
provider_vector_db_idline entirely when it'sNone?If blank lines cause YAML parsing issues, consider modifying the template to handle this case explicitly (e.g., using conditional formatting or post-processing to remove empty lines).
384-390: LGTM: Updated save flow correctly propagates provider_vector_db_id.The save method properly constructs the
provider_vector_db_idYAML line and passes it towrite_yaml_config, and thevector_io.insertcall correctly uses thevector_db_idparameter with manual chunks.
andrej1991
force-pushed
the
update_llama
branch
from
00885ce to
deb39c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyproject.toml (1)
40-52: Verify compatibility with relaxed version constraints.The dependencies have been changed from exact pins (==) to minimum versions (>=), which improves flexibility but may introduce compatibility issues with newer releases. While llama-stack packages remain pinned to 0.2.22, other dependencies like PyYAML, torch, and llama-index components can now float to newer versions.
Consider testing with the upper bounds of these dependencies to ensure no breaking changes occur, or document the tested version range if specific upper limits are known to cause issues.
src/lightspeed_rag_content/document_processor.py (1)
237-237: Consider YAML formatting when provider_vector_db_id is None.When
provider_vector_db_idis None or not provided, line 325 injects an empty string into the template, which results in a blank line in the YAML output. While this may be acceptable, consider explicitly handling this case for cleaner output:- provider_vector_db_id=provider_vector_db_id or "", + provider_vector_db_id=f"provider_vector_db_id: {index}" if provider_vector_db_id else "# provider_vector_db_id not configured",Alternatively, verify that the empty line doesn't cause issues with YAML parsing.
Also applies to: 325-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml(2 hunks)scripts/query_rag.py(4 hunks)src/lightspeed_rag_content/document_processor.py(6 hunks)tests/test_document_processor_llama_stack.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_document_processor_llama_stack.py
⏰ 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). (1)
- GitHub Check: build-and-push-dev
🔇 Additional comments (9)
pyproject.toml (1)
23-25: LGTM! Mypy configuration is appropriate.The added mypy options are standard best practices for src-layout projects and will improve type checking accuracy.
scripts/query_rag.py (4)
10-10: LGTM! Import additions support improved type safety.The new imports enable stronger type checking for node handling throughout the script.
Also applies to: 15-15
161-161: LGTM! Client path updated for llama_stack 0.2.22 compatibility.The migration from
distribution.library_clienttocore.library_clientaligns with the llama_stack release-0.2.22 restructuring mentioned in the PR objectives.
39-66: LGTM! Robust type checking and error handling for single-node queries.The implementation correctly validates that retrieved nodes are TextNode instances and provides clear error messages in both JSON and non-JSON modes.
105-121: LGTM! NodeWithScore filtering adds defensive type checking.The implementation correctly filters results to only include NodeWithScore instances, with appropriate debug logging for any skipped entries. The text extraction fallback using
get_content()provides resilience against missing attributes.src/lightspeed_rag_content/document_processor.py (4)
210-217: Verify the hard-coded/tmp/llama-stack-filespath.The files provider configuration uses a hard-coded temporary directory path. Consider whether this should be:
- Configurable via parameter or environment variable
- Created within the same temporary directory as the rest of the llama-stack config
- Documented for users who may need to manage this directory
Can you confirm if this path needs to be configurable or if the hard-coded
/tmplocation is intentional for all deployment scenarios?
301-307: LGTM! Backward-compatible signature extension.The addition of the optional
provider_vector_db_idparameter maintains backward compatibility while enabling the new configuration flow required for llama_stack 0.2.22.
295-295: LGTM! Client path updated for llama_stack 0.2.22 compatibility.The migration to
core.library_clientis consistent with the changes inscripts/query_rag.pyand aligns with the llama_stack restructuring in release-0.2.22. The docstring has been appropriately updated as well.Also applies to: 333-333
384-386: LGTM! provider_vector_db_id correctly propagated to YAML generation.The save method now passes the provider_vector_db_id configuration through to write_yaml_config, enabling the new VectorDB provider integration mentioned in the PR objectives.
2 participants
Description
This change is needed, due to the llama_stack and llama_stack_client config was moved to the release 0.2.22 in the lightspeed-core v0.3.0.
Llama-stack release-0.2.21 introduced this change: https://github.com/llamastack/llama-stack/pull/3253/files
The change was introduced because llama-stack in the future will support VectorStores against VectorDBs.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests