Repository navigation
fix: run sync document store methods in a worker thread - #13381
davidsbatista merged 5 commits into
Conversation
…tore FilterRetriever, AutoMergingRetriever and SentenceWindowRetriever call filter_documents_async unconditionally. The async methods are optional in the DocumentStore protocol, so a store that implements only the sync API failed with AttributeError: 'X' object has no attribute 'filter_documents_async' instead of the TypeError DocumentWriter and CacheChecker already raise for the same situation. Add the same hasattr guard at the top of each run_async and one test per retriever using document_store_class(), the helper the DocumentWriter and CacheChecker async tests already use for this case.
|
@Starchaser1024 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @Starchaser1024, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
|
Thanks for signing the CLA, @Starchaser1024! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
|
Thanks for the PR, @Starchaser1024! The guard is consistent with I'd rather fall back to the sync method in a worker thread when the async one is missing: if hasattr(self.document_store, "filter_documents_async"):
docs = await self.document_store.filter_documents_async(filters=filters) # type: ignore[attr-defined]
else:
docs = await asyncio.to_thread(self.document_store.filter_documents, filters=filters)This is what we already do elsewhere for sync-only code under async:
Could you:
Also, please keep the comment in filter_retriever.py that explains the # type: ignore[attr-defined]. |
FilterRetriever, AutoMergingRetriever, SentenceWindowRetriever, CacheChecker and DocumentWriter called the optional <method>_async APIs unconditionally, so a document store that implements only the synchronous protocol failed with AttributeError: 'X' object has no attribute 'filter_documents_async'. run_async now goes through a shared helper that awaits the async method when the store provides one and otherwise runs the synchronous method in a worker thread, the same fallback Pipeline.run_async applies to components without run_async. The attribute guards and their TypeError docstrings are gone; the tests use document_store_class() for a sync-only store and assert the calls return results.
|
All four points are in, and the two components that already had the guard now share the same path. The three retrievers no longer raise. They call a small private helper in On the The tests for all five components now build a sync-only store with |
|
Thanks, looks good! A few nits:
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Rename and reword the release note: the components no longer raise a specific error, they run the sync method in a worker thread. - Shorten the retriever comments and move the FilterRetriever one above the call it describes. - Drop the per-call debug log from _call_with_async_fallback; CacheChecker would emit it once per item.
|
All three are in, de63990:
|
|
Thanks for signing the CLA, @Starchaser1024! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Fixes #13380.
What changed
FilterRetriever,AutoMergingRetriever,SentenceWindowRetriever,CacheCheckerandDocumentWriterall called the optional<method>_asyncAPIs unconditionally. A document store that implements only the synchronous protocol therefore failed withAttributeError: 'X' object has no attribute 'filter_documents_async'— an error that names a missing attribute rather than the real situation, and that made these five components unusable fromrun_asyncon a store that works fine in a synchronous pipeline.run_asyncnow calls a small private helper added tohaystack/utils/async_utils.py:It awaits the async method when the store provides one and otherwise runs the synchronous method in a worker thread — the same fallback
Pipeline.run_asyncapplies to components that do not implementrun_async(haystack/utils/async_utils.py::_execute_component_async), and the shapehaystack/tools/tool.py:309,haystack/components/evaluators/llm_evaluator.py:344andhaystack/document_stores/in_memory/document_store.py:968already use. The three retrievers,CacheChecker(filter_documents) andDocumentWriter(write_documents) go through it, so the five components now behave the same way. The attribute guards and the:raises TypeError:entries in their docstrings are gone.filter_retriever.pykeeps the comment about the typing quirk: theDocumentStoreprotocol does not declare the async methods. The# type: ignore[attr-defined]itself is no longer needed, because the helper resolves the method withgetattrinstead of touching the attribute directly; the same note sits in the other two retrievers.Behaviour
A store that implements both APIs is unaffected: the async method is awaited exactly as before, and
InMemoryDocumentStorestill runs its own async path. A sync-only store now returns results instead of raising, with the synchronous call executed off the event loop.One test changed shape rather than intent:
test_sentence_window_retriever_async.py::test_run_async_queries_document_store_once_for_all_retrieved_documentsasserted the filter was passed positionally (call_args.args[0]). Calls now pass it by keyword, so the assertion readscall_args.kwargs["filters"]; what it checks — one store query for all retrieved documents, two conditions — is unchanged.Tests
The three retriever tests and the
CacheChecker/DocumentWriterones now build a sync-only store withdocument_store_class(...)and assert the calls return results. With the previous guard in place all five fail withTypeError: Document store MockedDocumentStore does not provide async support.; with this change:ruff checkandruff format --check(0.16.0, the rev pinned in.pre-commit-config.yaml) are clean on the touched files. The release note is reworded: the components now work with document stores that implement only the synchronous API, instead of documenting aTypeError.