diff --git a/haystack/components/converters/image/image_utils.py b/haystack/components/converters/image/image_utils.py index 35e7612640..0242b0202d 100644 --- a/haystack/components/converters/image/image_utils.py +++ b/haystack/components/converters/image/image_utils.py @@ -162,54 +162,55 @@ def _convert_pdf_to_images( resolved_page_range = page_range or range(1, num_pages + 1) - for page_number in resolved_page_range: - if page_number < 1 or page_number > num_pages: - logger.warning( - "Page {page_number} is out of range for the PDF file {file_path}. Skipping it.", - page_number=page_number, - file_path=bytestream.meta.get("file_path"), - ) - continue + try: + for page_number in resolved_page_range: + if page_number < 1 or page_number > num_pages: + logger.warning( + "Page {page_number} is out of range for the PDF file {file_path}. Skipping it.", + page_number=page_number, + file_path=bytestream.meta.get("file_path"), + ) + continue - # Get dimensions of the page - page = pdf[max(page_number - 1, 0)] # Adjust for 0-based indexing - _, _, width, height = page.get_mediabox() + # Get dimensions of the page + page = pdf[max(page_number - 1, 0)] # Adjust for 0-based indexing + _, _, width, height = page.get_mediabox() - target_resolution_dpi = 300.0 + target_resolution_dpi = 300.0 - # From pypdfium2 docs: scale (float) – A factor scaling the number of pixels per PDF canvas unit. This defines - # the resolution of the image. To convert a DPI value to a scale factor, multiply it by the size of 1 canvas - # unit in inches (usually 1/72in). - # https://pypdfium2.readthedocs.io/en/stable/python_api.html#pypdfium2._helpers.page.PdfPage.render - target_scale = target_resolution_dpi / 72.0 + # From pypdfium2 docs: scale (float) – A factor scaling the number of pixels per PDF canvas unit. This + # defines the resolution of the image. To convert a DPI value to a scale factor, multiply it by the size of + # 1 canvas unit in inches (usually 1/72in). + # https://pypdfium2.readthedocs.io/en/stable/python_api.html#pypdfium2._helpers.page.PdfPage.render + target_scale = target_resolution_dpi / 72.0 - # Calculate potential pixels for target_dpi - pixels_for_target_scale = width * height * target_scale**2 + # Calculate potential pixels for target_dpi + pixels_for_target_scale = width * height * target_scale**2 - pil_max_pixels = PILImage.MAX_IMAGE_PIXELS or int(1024 * 1024 * 1024 // 4 // 3) - # 90% of PIL's default limit to prevent borderline cases - pixel_limit = pil_max_pixels * 0.9 + pil_max_pixels = PILImage.MAX_IMAGE_PIXELS or int(1024 * 1024 * 1024 // 4 // 3) + # 90% of PIL's default limit to prevent borderline cases + pixel_limit = pil_max_pixels * 0.9 - scale = target_scale - if pixels_for_target_scale > pixel_limit: - logger.info( - "Large PDF detected ({pixels:.2f} pixels). Resizing the image to fit the pixel limit.", - pixels=pixels_for_target_scale, - ) - scale = (pixel_limit / (width * height)) ** 0.5 + scale = target_scale + if pixels_for_target_scale > pixel_limit: + logger.info( + "Large PDF detected ({pixels:.2f} pixels). Resizing the image to fit the pixel limit.", + pixels=pixels_for_target_scale, + ) + scale = (pixel_limit / (width * height)) ** 0.5 - pdf_bitmap = page.render(scale=scale) + pdf_bitmap = page.render(scale=scale) - image: "Image" = pdf_bitmap.to_pil() - pdf_bitmap.close() - if size is not None: - # Set reducing_gap=None to disable multi-step shrink; better quality. - # https://pillow.readthedocs.io/en/latest/reference/Image.html#PIL.Image.Image.thumbnail - image.thumbnail(size=size, reducing_gap=None) + image: "Image" = pdf_bitmap.to_pil() + pdf_bitmap.close() + if size is not None: + # Set reducing_gap=None to disable multi-step shrink; better quality. + # https://pillow.readthedocs.io/en/latest/reference/Image.html#PIL.Image.Image.thumbnail + image.thumbnail(size=size, reducing_gap=None) - all_pdf_images.append((page_number, image)) - - pdf.close() + all_pdf_images.append((page_number, image)) + finally: + pdf.close() if return_base64: return [ diff --git a/haystack/components/preprocessors/sentence_tokenizer.py b/haystack/components/preprocessors/sentence_tokenizer.py index d42f1b4df7..07d2cf50fe 100644 --- a/haystack/components/preprocessors/sentence_tokenizer.py +++ b/haystack/components/preprocessors/sentence_tokenizer.py @@ -103,7 +103,7 @@ if nltk_imports.is_successful(): """ try: return self._re_period_context # type: ignore - except: # noqa: E722 + except AttributeError: self._re_period_context = re.compile( self._period_context_fmt % { diff --git a/releasenotes/notes/close-pdf-on-image-conversion-error-df5231752de05e51.yaml b/releasenotes/notes/close-pdf-on-image-conversion-error-df5231752de05e51.yaml new file mode 100644 index 0000000000..e330714154 --- /dev/null +++ b/releasenotes/notes/close-pdf-on-image-conversion-error-df5231752de05e51.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Converting PDF pages to images, as ``PDFToImageContent`` and ``DocumentToImageContent`` do, now closes the + pypdfium2 document even when rendering a page raises an error. Before, the document was only closed after all + pages rendered successfully, so a failing page left it open until garbage collection. diff --git a/test/components/converters/image/test_image_utils.py b/test/components/converters/image/test_image_utils.py index d509a926a3..23bf067ced 100644 --- a/test/components/converters/image/test_image_utils.py +++ b/test/components/converters/image/test_image_utils.py @@ -96,6 +96,22 @@ class TestConvertPdfToImages: assert "Large PDF detected" in caplog.text + def test_convert_pdf_to_images_closes_pdf_when_rendering_fails(self) -> None: + bytestream = get_bytestream_from_source(Path("test/test_files/pdf/sample_pdf_1.pdf")) + + mock_pdf_document = MagicMock() + mock_pdf_document.__len__.return_value = 1 + mock_page = MagicMock() + mock_page.get_mediabox.return_value = (0, 0, 100, 100) + mock_page.render.side_effect = RuntimeError("render failed") + mock_pdf_document.__getitem__.return_value = mock_page + + with patch("haystack.components.converters.image.image_utils.PdfDocument", return_value=mock_pdf_document): + with pytest.raises(RuntimeError, match="render failed"): + _convert_pdf_to_images(bytestream=bytestream, page_range=[1]) + + mock_pdf_document.close.assert_called_once() + class TestEncodeImageToBase64: def test_encode_image_to_base64(self) -> None: diff --git a/test/components/preprocessors/test_sentence_tokenizer.py b/test/components/preprocessors/test_sentence_tokenizer.py index ad7a3f988b..07f5c06c94 100644 --- a/test/components/preprocessors/test_sentence_tokenizer.py +++ b/test/components/preprocessors/test_sentence_tokenizer.py @@ -4,12 +4,17 @@ import time from pathlib import Path +from typing import Any from unittest.mock import patch import pytest from pytest import LogCaptureFixture -from haystack.components.preprocessors.sentence_tokenizer import QUOTE_SPANS_RE, SentenceSplitter +from haystack.components.preprocessors.sentence_tokenizer import ( + QUOTE_SPANS_RE, + CustomPunktLanguageVars, + SentenceSplitter, +) def test_apply_split_rules_no_join() -> None: @@ -187,3 +192,14 @@ def test_split_sentences_performance() -> None: end = time.time() assert end - start < 2, f"Execution time exceeded 2 seconds: {end - start:.2f} seconds" + + +def test_period_context_re_does_not_swallow_unexpected_errors() -> None: + class FailingLanguageVars(CustomPunktLanguageVars): + def __getattr__(self, name: str) -> Any: + if name == "_re_period_context": + raise RuntimeError("unexpected") + raise AttributeError(name) + + with pytest.raises(RuntimeError, match="unexpected"): + FailingLanguageVars().period_context_re()