feat: add Gotenberg converter integration - #3839
Conversation
|
Hi @maxdswain, thanks for your interest in contributing to Haystack! 🙏 This is an automated message to help us keep the review queue healthy. |
github-actions
Bot
added
topic:CI
type:documentation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| """ | ||
| Convert files, UTF-8 HTML or Markdown, and web URLs to ordered PDFs with Gotenberg. | ||
|
|
||
| Resources are supported only for HTML and Markdown conversions. |
There was a problem hiding this comment.
Lets add a short Usage example in the docstrings here
| Resources are supported only for HTML and Markdown conversions. | ||
| """ | ||
|
|
||
| def __init__(self, url: str = "http://localhost:3000", timeout: float = 30.0) -> None: |
There was a problem hiding this comment.
Lets make sure to add docstrings for the init method
| return default_from_dict(cls, data) | ||
|
|
||
| @staticmethod | ||
| def _pdf(response: SingleFileResponse | ZipFileResponse) -> ByteStream: |
There was a problem hiding this comment.
Even for these private methods could we add docstrings as well? It makes it easier to understand and review.
| """ | ||
| with self._prepare(sources, conversion_type, resources) as (prepared, resource_paths): | ||
| with SyncGotenbergClient(self.url, timeout=self.timeout, backend="httpx") as client: | ||
| output = [self._pdf(_route(client, source, resource_paths).run()) for source in prepared] |
There was a problem hiding this comment.
when doing function calls in general it would be nice to use the variable names in the call signature
| class GotenbergFileConverterOutput(TypedDict): | ||
| """The output returned by `GotenbergFileConverter`.""" | ||
|
|
||
| output: list[ByteStream] |
There was a problem hiding this comment.
This feels a bit overkill, could we just update the return signature to be dict[str, list[ByteStream]] instead of using a typed dict?
| from haystack_integrations.components.converters.gotenberg.converter import ConversionType, GotenbergFileConverter | ||
|
|
||
| __all__ = ["ConversionType", "GotenbergFileConverter"] |
There was a problem hiding this comment.
I don't think we need to re-export ConversionType here. If its okay with you lets remove it.
There was a problem hiding this comment.
That's okay with me, I'll remove it.
| Convert sources to PDF using the selected Gotenberg route. | ||
|
|
||
| :param sources: Sources interpreted according to `conversion_type`. | ||
| :param conversion_type: Route to use: `libreoffice`, `html`, `markdown`, or `url`. |
There was a problem hiding this comment.
Lets expand on this docstring. Currently this is lacking information to properly understand what this routing means. Does it mean the incoming sources are all of type conversion_type?
| def run( | ||
| self, sources: list[Source], conversion_type: ConversionType, resources: list[Path] | None = None | ||
| ) -> GotenbergFileConverterOutput: |
There was a problem hiding this comment.
Typically in our converters we also like to support
meta: dict[str, Any] | list[dict[str, Any]] | None = None,
as a second argument to allow users to attach metadata to sources they are converting. Please check out how our Markdown converter handles this such that it also preserves any metadata that might already be attached to a Source that is a ByteStream
| self, sources: list[Source], conversion_type: ConversionType, resources: list[Path] | None = None | ||
| ) -> GotenbergFileConverterOutput: | ||
| """ | ||
| Convert sources to PDF using the selected Gotenberg route. |
There was a problem hiding this comment.
Also general design question why do we force users to have to pick a conversion type? Why not auto route based on the detected mime type of the incoming source? The str, and Path sources should have a suffix and we can throw an error if ByteStream doesn't come with a populated mime type.
There was a problem hiding this comment.
I was modelling this integration off of the existing libreoffice integration where we forced users to pick a conversation, but yeah you're right in that we don't need to force users for this integration. I'll implement auto detection of mime type.
|
Thanks for the review @sjrl, I think I've addressed all of your comments now. |
2 participants
Related Issues
Proposed Changes:
Add new integration that uses gotenberg docker containers to scale document conversion from a variety of formats.
How did you test it?
Added unit tests, integration tests and instructions for running tests locally using a docker container.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.