-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(config): add global document_timeout setting #46
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: archasek <[email protected]>
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
docling_serve/settings.py
Outdated
model_config = SettingsConfigDict(env_prefix="DOCLING_") | ||
model_config = SettingsConfigDict( | ||
env_prefix="DOCLING_", | ||
env_file="docling_serve/.env", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be simple .env
? otherwise we force the user on the given location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also think there should be a single .env
file in the root folder for the entire project. However, I noticed that there is already a .env.example
file in the docling_serve
folder. So, I decided not to change the current design at this point. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better with a single .env
. Let's align all options, and in case use prefixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please have a look dolfim. I also noticed an issue with error handling and fixed that (PR description updated). LMK if it's acceptable.
Signed-off-by: archasek <[email protected]>
Signed-off-by: archasek <[email protected]>
Signed-off-by: archasek <[email protected]>
Signed-off-by: archasek <[email protected]>
@@ -69,9 +72,13 @@ def _export_document_as_content( | |||
if export_doctags: | |||
document.doctags_content = new_doc.export_to_document_tokens() | |||
elif conv_res.status == ConversionStatus.SKIPPED: | |||
raise HTTPException(status_code=400, detail=conv_res.errors) | |||
raise HTTPException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the single document, this makes perfect sense.
But this might not be the expected behavior when the user provides multiple document.
In this proposal, if 10 documents are provided and 1 fails, it will raise the error and produce no results.
In the latter, it would be better to have some manifest response file in the zip, where the system lists the result for each file.
API - When there's timeout, this kind of response (500) is returned:
Gradio:

Logs: