diff --git a/haystack/components/retrievers/sentence_window_retriever.py b/haystack/components/retrievers/sentence_window_retriever.py index e24b2666d6..0eecfb1297 100644 --- a/haystack/components/retrievers/sentence_window_retriever.py +++ b/haystack/components/retrievers/sentence_window_retriever.py @@ -104,8 +104,7 @@ def __init__( metadata fields. If False, it will skip retrieving the context for documents that are missing the required metadata fields, but will still include the original document in the results. """ - if window_size < 1: - raise ValueError("The window_size parameter must be greater than 0.") + self._validate_window_size(window_size) self.window_size = window_size self.document_store = document_store @@ -187,7 +186,8 @@ def run(self, retrieved_documents: list[Document], window_size: int | None = Non :param retrieved_documents: List of retrieved documents from the previous retriever. :param window_size: The number of documents to retrieve before and after the relevant one. This will overwrite - the `window_size` parameter set in the constructor. + the `window_size` parameter set in the constructor. It must be greater than 0; values of + 0 and negative values are rejected. :returns: A dictionary with the following keys: - `context_windows`: A list of strings, where each string represents the concatenated text from the @@ -197,8 +197,8 @@ def run(self, retrieved_documents: list[Document], window_size: int | None = Non meta field. """ - window_size = window_size or self.window_size - SentenceWindowRetriever._raise_if_windows_size_is_negative(window_size) + window_size = self.window_size if window_size is None else window_size + self._validate_window_size(window_size) self._raise_if_documents_do_not_have_expected_metadata(retrieved_documents) context_text = [] @@ -220,7 +220,8 @@ async def run_async(self, retrieved_documents: list[Document], window_size: int :param retrieved_documents: List of retrieved documents from the previous retriever. :param window_size: The number of documents to retrieve before and after the relevant one. This will overwrite - the `window_size` parameter set in the constructor. + the `window_size` parameter set in the constructor. It must be greater than 0; values of + 0 and negative values are rejected. :returns: A dictionary with the following keys: - `context_windows`: A list of strings, where each string represents the concatenated text from the @@ -230,8 +231,8 @@ async def run_async(self, retrieved_documents: list[Document], window_size: int meta field. """ - window_size = window_size or self.window_size - SentenceWindowRetriever._raise_if_windows_size_is_negative(window_size) + window_size = self.window_size if window_size is None else window_size + self._validate_window_size(window_size) self._raise_if_documents_do_not_have_expected_metadata(retrieved_documents) context_text = [] @@ -244,7 +245,7 @@ async def run_async(self, retrieved_documents: list[Document], window_size: int return {"context_windows": context_text, "context_documents": context_documents} @staticmethod - def _raise_if_windows_size_is_negative(window_size: int) -> None: + def _validate_window_size(window_size: int) -> None: if window_size < 1: raise ValueError("The window_size parameter must be greater than 0.") diff --git a/releasenotes/notes/fix-sentence-window-runtime-window-size-zero-774d768cefcde325.yaml b/releasenotes/notes/fix-sentence-window-runtime-window-size-zero-774d768cefcde325.yaml new file mode 100644 index 0000000000..efd6eb8a27 --- /dev/null +++ b/releasenotes/notes/fix-sentence-window-runtime-window-size-zero-774d768cefcde325.yaml @@ -0,0 +1,22 @@ +--- +upgrade: + - | + Passing ``window_size=0`` to ``SentenceWindowRetriever.run`` or ``SentenceWindowRetriever.run_async`` + now raises a ``ValueError`` instead of silently using the ``window_size`` set in the constructor. You are + affected if you pass ``window_size=0`` at runtime, either directly or from an upstream component in a + pipeline. If you were relying on ``0`` to mean "use the value from the constructor", omit the argument + (or pass ``None``) instead: + + .. code:: python + + retriever = SentenceWindowRetriever(document_store=document_store, window_size=3) + + # Before: silently used window_size=3 + retriever.run(retrieved_documents=docs, window_size=0) + + # After: omit the argument to use the constructor value + retriever.run(retrieved_documents=docs) +fixes: + - | + ``SentenceWindowRetriever.run`` and ``SentenceWindowRetriever.run_async`` now validate an explicitly + provided ``window_size=0`` instead of treating it as unset and falling back to the constructor value. diff --git a/test/components/retrievers/test_sentence_window_retriever.py b/test/components/retrievers/test_sentence_window_retriever.py index d7cb222729..7d8f45ef90 100644 --- a/test/components/retrievers/test_sentence_window_retriever.py +++ b/test/components/retrievers/test_sentence_window_retriever.py @@ -174,11 +174,35 @@ def test_document_without_all_source_ids(self, in_memory_doc_store): ) retriever.run(retrieved_documents=docs) - def test_run_invalid_window_size(self, in_memory_doc_store): - docs = [Document(content="This is a text with some words. There is a ", meta={"id": "doc_0", "split_id": 0})] + def test_init_rejects_zero_window_size(self, in_memory_doc_store): with pytest.raises(ValueError): - retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=0) - retriever.run(retrieved_documents=docs) + SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=0) + + def test_run_rejects_invalid_runtime_window_size(self, in_memory_doc_store): + retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=3) + + with pytest.raises(ValueError, match="window_size parameter must be greater than 0"): + retriever.run(retrieved_documents=[], window_size=0) + + with pytest.raises(ValueError, match="window_size parameter must be greater than 0"): + retriever.run(retrieved_documents=[], window_size=-1) + + def test_run_without_runtime_window_size_uses_constructor_value(self, in_memory_doc_store): + docs = [ + Document(content=f"Sentence {sent}.", meta={"id": f"doc_{sent}", "source_id": "source1", "split_id": sent}) + for sent in range(10) + ] + in_memory_doc_store.write_documents(docs) + retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=2) + retrieved_documents = [doc for doc in docs if doc.content == "Sentence 4."] + + # window_size omitted: the constructor value is used, so 2 documents on each side + result = retriever.run(retrieved_documents=retrieved_documents) + assert len(result["context_documents"]) == 5 + + # window_size passed explicitly: it overrides the constructor value + result = retriever.run(retrieved_documents=retrieved_documents, window_size=1) + assert len(result["context_documents"]) == 3 def test_constructor_parameter_does_not_change(self, in_memory_doc_store): retriever = SentenceWindowRetriever(in_memory_doc_store, window_size=5) diff --git a/test/components/retrievers/test_sentence_window_retriever_async.py b/test/components/retrievers/test_sentence_window_retriever_async.py index 3c776f5b9f..de85b08e60 100644 --- a/test/components/retrievers/test_sentence_window_retriever_async.py +++ b/test/components/retrievers/test_sentence_window_retriever_async.py @@ -15,6 +15,7 @@ class TestSentenceWindowRetrieverAsync: + @pytest.mark.asyncio async def test_document_without_split_id(self, in_memory_doc_store): docs = [ Document(content="This is a text with some words. There is a ", meta={"id": "doc_0"}), @@ -62,11 +63,37 @@ async def test_document_without_all_source_ids(self, in_memory_doc_store): await retriever.run_async(retrieved_documents=docs) @pytest.mark.asyncio - async def test_run_async_invalid_window_size(self, in_memory_doc_store): - docs = [Document(content="This is a text with some words. There is a ", meta={"id": "doc_0", "split_id": 0})] + async def test_init_rejects_zero_window_size(self, in_memory_doc_store): with pytest.raises(ValueError): - retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=0) - await retriever.run_async(retrieved_documents=docs) + SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=0) + + @pytest.mark.asyncio + async def test_run_async_rejects_invalid_runtime_window_size(self, in_memory_doc_store): + retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=3) + + with pytest.raises(ValueError, match="window_size parameter must be greater than 0"): + await retriever.run_async(retrieved_documents=[], window_size=0) + + with pytest.raises(ValueError, match="window_size parameter must be greater than 0"): + await retriever.run_async(retrieved_documents=[], window_size=-1) + + @pytest.mark.asyncio + async def test_run_async_without_runtime_window_size_uses_constructor_value(self, in_memory_doc_store): + docs = [ + Document(content=f"Sentence {sent}.", meta={"id": f"doc_{sent}", "source_id": "source1", "split_id": sent}) + for sent in range(10) + ] + in_memory_doc_store.write_documents(docs) + retriever = SentenceWindowRetriever(document_store=in_memory_doc_store, window_size=2) + retrieved_documents = [doc for doc in docs if doc.content == "Sentence 4."] + + # window_size omitted: the constructor value is used, so 2 documents on each side + result = await retriever.run_async(retrieved_documents=retrieved_documents) + assert len(result["context_documents"]) == 5 + + # window_size passed explicitly: it overrides the constructor value + result = await retriever.run_async(retrieved_documents=retrieved_documents, window_size=1) + assert len(result["context_documents"]) == 3 @pytest.mark.asyncio async def test_constructor_parameter_does_not_change(self, in_memory_doc_store):