Skip to content
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.

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

fix: add ollama embedding config and fix sqlite_vec db #1255

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wukaixingxp
Copy link
Contributor

What does this PR do?

RAG+Ollama is not working, as I encounter errors: Embeddings are now served via Inference providers. Taking a closer look it seems that the embedding model yaml config has been removed here, this PR will add the config back so user can use the embedding model after they ollama run all-minilm:latest.

Then, sqlite_vec also give error SQLite objects created in a thread can only be used in that same thread. The object was created in thread id 8349485120 and this is thread id 6325039104, I think we need to use self.connection = sqlite3.connect(self.config.db_path, check_same_thread=False) instead in :

self.connection = sqlite3.connect(self.config.db_path)

Test Plan

[Describe the tests you ran to verify your changes with result summaries. Provide clear instructions so the plan can be easily re-executed.]
Tested with my DocQA app

@wukaixingxp wukaixingxp self-assigned this Feb 25, 2025
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Feb 25, 2025
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

see comment inline

@wukaixingxp
Copy link
Contributor Author

see comment inline

Thanks for your help! I run pre-commit locally and everything looks good, not sure why pre-commit failed at CI

pre-commit run --all-files
check for merge conflicts................................................Passed
check for added large files..............................................Passed
fix end of files.........................................................Passed
Insert license in comments...............................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
blacken-docs.............................................................Passed
uv-export................................................................Passed
mypy.....................................................................Passed
Distribution Template Codegen............................................Passed

@wukaixingxp wukaixingxp requested a review from ashwinb February 25, 2025 23:12
@wukaixingxp wukaixingxp changed the title add ollama embedding config and fix sqlite_vec db fix:add ollama embedding config and fix sqlite_vec db Feb 25, 2025
@wukaixingxp
Copy link
Contributor Author

see comment inline

Thanks for your help! I run pre-commit locally and everything looks good, not sure why pre-commit failed at CI

pre-commit run --all-files
check for merge conflicts................................................Passed
check for added large files..............................................Passed
fix end of files.........................................................Passed
Insert license in comments...............................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
blacken-docs.............................................................Passed
uv-export................................................................Passed
mypy.....................................................................Passed
Distribution Template Codegen............................................Passed

@ashwinb Can you take another look at this PR? somehow the pre-commit is blocking it, I am not sure how to solve

@ashwinb ashwinb changed the title fix:add ollama embedding config and fix sqlite_vec db fix: add ollama embedding config and fix sqlite_vec db Feb 26, 2025
@@ -162,7 +162,7 @@ def __init__(self, config, inference_api: Api.inference) -> None:

async def initialize(self) -> None:
# Open a connection to the SQLite database (the file is specified in the config).
self.connection = sqlite3.connect(self.config.db_path)
self.connection = sqlite3.connect(self.config.db_path, check_same_thread=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I think the correct fix needs to be what @ehhuang did in 270d640

@ashwinb
Copy link
Contributor

ashwinb commented Feb 26, 2025

You need to fetch and rebase to latest origin/main to get rid of the pre-commit error. But I think the threading error needs to be taken care of in a better way (as @ehhuang's commit shows.)

@wukaixingxp wukaixingxp requested a review from ashwinb February 26, 2025 23:47
@wukaixingxp
Copy link
Contributor Author

You need to fetch and rebase to latest origin/main to get rid of the pre-commit error. But I think the threading error needs to be taken care of in a better way (as @ehhuang's commit shows.)

@ashwinb Thanks for the guidance, I followed his example and tested with my RAG app. Can you take another look?


async def initialize(self) -> None:
# Open a connection to the SQLite database (the file is specified in the config).
self.connection = sqlite3.connect(self.config.db_path)
self.connection = self._get_connection()
Copy link
Contributor

Choose a reason for hiding this comment

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

so this isn't quite right. you cannot assign whatever value you get into an instance variable. you must use conn = self._get_connection() anywhere you need to use a connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants