Repository: deepset-ai/haystack
Pull request: feat: preprocessor raises warning when doc length exceeds threshold
### Related Issues
- fixes https://github.com/deepset-ai/haystack/issues/3285

### Proposed Changes:
- Adds a `max_chars` parameter to `PreProcessor.__init__` 
- This parameter will make PreProcessor output a warning when documents longer than `max_chars` are about to be returned.
- See  #3557 about the reason why the creation of such documents cannot be prevented without breaking other constraints.
- Adds a relevant test

### How did you test it?
- unit test

### Notes for the reviewer
n/a

### Checklist
- [x] I have read the [contributors guidelines](https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md) and the [code of conduct](https://github.com/deepset-ai/haystack/blob/main/code_of_conduct.txt)
- [x] I have updated the related issue with new insights and changes
- [x] I added tests that demonstrate the correct behavior of the change
- [x] I've used one of the [conventional commit types](https://www.conventionalcommits.org/en/v1.0.0/) for my PR title: `fix:`, `feat:`, `build:`, `chore:`, `ci:`, `docs:`, `style:`, `refactor:`, `perf:`, `test:`.
- [x] I documented my code
- [x] I ran [pre-commit hooks](https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md#installation) and fixed any issue


Problem statement:
Preprocessing: Warning when max doc length significantly exceeds preprocessor split length
**Is your feature request related to a problem? Please describe.**
When running a retriever-reader pipeline, we noticed that certain inference times were much longer than others, and were even running into timeout problems. We thought this was odd, since we had a preprocessor splitting files into 250-word docs in order to avoid this. However, since `split_respect_sentence_boundary` was set to `True` by default, and since there were large passages in our files without a period (hence were parsed as a single sentence and not split by the preprocessor), this meant that some of the documents were much, much longer than 250 words (e.g., 62,000 words), hence the slowdown. It took us quite some time to figure this out, which admittedly falls partly on our shoulders, but it would have saved us considerable time and effort if we had received some sort of warning indicating that the max doc length in our document store was many multiples greater than the preprocessor's split length.

**Describe the solution you'd like**
It would be helpful to receive some kind of warning whenever the max document length after preprocessing is significantly greater than the preprocessor's split length. (I don't have an exact definition of "significantly greater" pinned down; apologies for the imprecision.)

Hints from issue discussion:

Commit message: improve test

Patch to review:
diff --git a/haystack/nodes/preprocessor/preprocessor.py b/haystack/nodes/preprocessor/preprocessor.py
index 0d24e93ae1..3cee9375d2 100644
--- a/haystack/nodes/preprocessor/preprocessor.py
+++ b/haystack/nodes/preprocessor/preprocessor.py
@@ -64,6 +64,7 @@ def __init__(
         id_hash_keys: Optional[List[str]] = None,
         progress_bar: bool = True,
         add_page_number: bool = False,
+        max_chars: int = 10_000,
     ):
         """
         :param clean_header_footer: Use heuristic to remove footers and headers across different pages by searching
@@ -97,6 +98,7 @@ def __init__(
                                 field `"page"`. Page boundaries are determined by `"\f"' character which is added
                                 in between pages by `PDFToTextConverter`, `TikaConverter`, `ParsrConverter` and
                                 `AzureConverter`.
+        :param max_chars: the maximum lenght a document is expected to have. Each document that is longer than max_chars after pre-processing will raise a warning.
         """
         super().__init__()
 
@@ -122,6 +124,7 @@ def __init__(
         self.id_hash_keys = id_hash_keys
         self.progress_bar = progress_bar
         self.add_page_number = add_page_number
+        self.max_chars = max_chars
 
     def process(
         self,
@@ -169,8 +172,26 @@ def process(
         else:
             raise Exception("documents provided to PreProcessor.prepreprocess() is not of type list nor Document")
 
+        self._long_documents(ret, max_chars=self.max_chars)
+
         return ret
 
+    def _long_documents(self, documents: List[Document], max_chars=10_000):
+        """
+        Function that tries to detect unusually long documents.
+
+        NOTE: this function is a heuristic that is in place only because a proper fix that prevents such documents from forming
+        would imply a complete revamp of this class, including better definitions of what the various units (word, sentence, passage) mean exactly.
+        """
+        for document in documents:
+            if len(document.content) > max_chars:
+                logger.warning(
+                    "Document %s is %s characters long after preprocessing, where the maximum lenght should be %s.",
+                    document.id,
+                    len(document.content),
+                    max_chars,
+                )
+
     def _process_single(
         self,
         document: Union[dict, Document],
diff --git a/test/nodes/test_preprocessor.py b/test/nodes/test_preprocessor.py
index bf0218f383..5972fc4c33 100644
--- a/test/nodes/test_preprocessor.py
+++ b/test/nodes/test_preprocessor.py
@@ -508,3 +508,16 @@ def test_file_exists_error_during_download(monkeypatch: MonkeyPatch, module_tmp_
 
     # This shouldn't raise an exception as the `FileExistsError` is ignored
     PreProcessor(split_length=2, split_respect_sentence_boundary=False)
+
+
+def test_preprocessor_very_long_document(caplog):
+    preproc = PreProcessor(
+        clean_empty_lines=False, clean_header_footer=False, clean_whitespace=False, split_by=None, max_chars=10
+    )
+    documents = [
+        Document(content=f"this is a test document with more than max_char characters: {'1'*i}") for i in range(9)
+    ]
+    results = preproc.process(documents)
+    assert results == documents
+    for i in range(5):
+        assert f"is 6{i} characters long after preprocessing, where the maximum lenght should be 10." in caplog.text


Code review comments:
diff_hunk: @@ -97,6 +98,7 @@ def __init__(
                                 field `"page"`. Page boundaries are determined by `"\f"' character which is added
                                 in between pages by `PDFToTextConverter`, `TikaConverter`, `ParsrConverter` and
                                 `AzureConverter`.
+        :param max_chars: the maximum lenght a document is expected to have. Each document that is longer than max_chars after pre-processing will raise a warning.
path: haystack/nodes/preprocessor/preprocessor.py
text: @user1:
1. "lenght" -> "length"
2. I know it's probably unnecessary but I'd still specify that it's the maximum length _in characters_.

diff_hunk: @@ -508,3 +508,16 @@ def test_file_exists_error_during_download(monkeypatch: MonkeyPatch, module_tmp_
 
     # This shouldn't raise an exception as the `FileExistsError` is ignored
     PreProcessor(split_length=2, split_respect_sentence_boundary=False)
+
+
+def test_preprocessor_very_long_document(caplog):
+    preproc = PreProcessor(
+        clean_empty_lines=False, clean_header_footer=False, clean_whitespace=False, split_by=None, max_chars=10
+    )
+    documents = [
+        Document(content=f"this is a test document with more than max_char characters: {'1'*i}") for i in range(9)
+    ]
+    results = preproc.process(documents)
+    assert results == documents
+    for i in range(5):
+        assert f"is 6{i} characters long after preprocessing, where the maximum lenght should be 10." in caplog.text
path: test/nodes/test_preprocessor.py
text: @user1:
"lenght" -> "length"

diff_hunk: @@ -169,8 +172,26 @@ def process(
         else:
             raise Exception("documents provided to PreProcessor.prepreprocess() is not of type list nor Document")
 
+        self._long_documents(ret, max_chars=self.max_chars)
+
         return ret
 
+    def _long_documents(self, documents: List[Document], max_chars=10_000):
+        """
+        Function that tries to detect unusually long documents.
+
+        NOTE: this function is a heuristic that is in place only because a proper fix that prevents such documents from forming
+        would imply a complete revamp of this class, including better definitions of what the various units (word, sentence, passage) mean exactly.
+        """
+        for document in documents:
+            if len(document.content) > max_chars:
+                logger.warning(
+                    "Document %s is %s characters long after preprocessing, where the maximum lenght should be %s.",
path: haystack/nodes/preprocessor/preprocessor.py
text: @user1:
1. "lenght" -> "length"
2. Do you think it's clear enough that nothing's actually been _done_ about the max length being exceeded? Or do you think the fact that it's just a warning conveys this adequately?

@user2:
Maybe we should consider renaming the variable to `max_chars_check`, so it's more clear that this variable is only used for checking the max number of chars?

@author:
@user2 I like the idea! I'll rename the variable :blush: 

@author:
I've improved a bit the error message as well