Skip to content

chore: Updating documentation, adding exception handling for Vector Stores in RAG Tool, more tests on migration, and migrate off of inference_api for context_retriever for RAG #3367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Sep 11, 2025
Merged

chore: Updating documentation, adding exception handling for Vector Stores in RAG Tool, more tests on migration, and migrate off of inference_api for context_retriever for RAG #3367

merged 4 commits into from
Sep 11, 2025
+360 −50

Conversation

franciscojavierarceo
Copy link
Collaborator

@franciscojavierarceo franciscojavierarceo commented Sep 7, 2025 •
edited
Loading

What does this PR do?

  • Updating documentation on migration from RAG Tool to Vector Stores and Files APIs
  • Adding exception handling for Vector Stores in RAG Tool
  • Add more tests on migration from RAG Tool to Vector Stores
  • Migrate off of inference_api for context_retriever for RAG

Test Plan

Integration and unit tests added

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 7, 2025
@franciscojavierarceo franciscojavierarceo force-pushed the ragtool-migration branch 2 times, most recently from 8080713 to b169c7d Compare September 9, 2025 21:06
franciscojavierarceo
franciscojavierarceo commented Sep 9, 2025
View reviewed changes
tests/integration/tool_runtime/test_rag_tool.py
),
Document(
document_id="data-url-doc",
content="data:text/plain;base64,VGhpcyBpcyBhIGRhdGEgVVJMIGRvY3VtZW50IGFib3V0IGRlZXAgbGVhcm5pbmcu", # "This is a data URL document about deep learning."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as noted in the comment, this is a base64 encoded string that just says "This is a data URL document about deep learning."

@franciscojavierarceo franciscojavierarceo changed the title chore: Updating documentation, adding exception handling for Vector Stores in RAG Tool, and more tests on migration chore: Updating documentation, adding exception handling for Vector Stores in RAG Tool, more tests on migration, and migrate off of inference_api for context_retriever for RAG Sep 9, 2025
@franciscojavierarceo franciscojavierarceo marked this pull request as ready for review September 9, 2025 21:11
franciscojavierarceo
franciscojavierarceo commented Sep 10, 2025
View reviewed changes
llama_stack/providers/inline/tool_runtime/rag/memory.py
static=VectorStoreChunkingStrategyStaticConfig(
max_chunk_size_tokens=chunk_size_in_tokens,
chunk_overlap_tokens=chunk_size_in_tokens // 4,
try:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

some nested blocks here to handle failures more gracefully

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum given that each except block continues why do we need to have that nested structure? We always continue the loop anyway right? Or is the goal more granular logging of the actual failures?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah the goal was more granular logging just in case so that end users can debug potential data parsing issues more obviously.

Bobbins228
Bobbins228 reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Migration instructions are clear and provide ample warning for updating to the new OpenAI APIs.

Unit/Integration tests look good and are passing. /lgtm

@mattf mattf mentioned this pull request Sep 10, 2025
Open
r3v5
r3v5 approved these changes Sep 10, 2025
View reviewed changes
Copy link
Contributor

@r3v5 r3v5 left a comment

Choose a reason for hiding this comment

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

Thanks, @franciscojavierarceo! lgtm

@franciscojavierarceo
chore: Updating documentation and adding exception handling for Vecto…
ff0bd41 
…r Stores in RAG Tool and updating inference to use openai and updating memory implementation to use existing libraries

Signed-off-by: Francisco Javier Arceo <[email protected]>
franciscojavierarceo
franciscojavierarceo commented Sep 10, 2025
View reviewed changes
llama_stack/providers/inline/tool_runtime/rag/memory.py
@@ -60,6 +57,47 @@ def make_random_string(length: int = 8):
return "".join(secrets.choice(string.ascii_letters + string.digits) for _ in range(length))


async def raw_data_from_doc(doc: RAGDocument) -> tuple[bytes, str]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needed this after testing with the type script UI

leseb
leseb approved these changes Sep 11, 2025
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

One non-blocking question. Thanks!

llama_stack/providers/inline/tool_runtime/rag/memory.py
static=VectorStoreChunkingStrategyStaticConfig(
max_chunk_size_tokens=chunk_size_in_tokens,
chunk_overlap_tokens=chunk_size_in_tokens // 4,
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum given that each except block continues why do we need to have that nested structure? We always continue the loop anyway right? Or is the goal more granular logging of the actual failures?

@leseb leseb merged commit d15368a into llamastack:main Sep 11, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb approved these changes

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb is a code owner

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

@ehhuang ehhuang Awaiting requested review from ehhuang ehhuang is a code owner

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan is a code owner

@bbrowning bbrowning Awaiting requested review from bbrowning bbrowning is a code owner

@reluctantfuturist reluctantfuturist Awaiting requested review from reluctantfuturist reluctantfuturist is a code owner

@mattf mattf Awaiting requested review from mattf mattf is a code owner

@slekkala1 slekkala1 Awaiting requested review from slekkala1 slekkala1 is a code owner

+2 more reviewers

@Bobbins228 Bobbins228 Bobbins228 approved these changes

@r3v5 r3v5 r3v5 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@franciscojavierarceo @leseb @Bobbins228 @r3v5