Conversation
On the RQ engine the converter fetched HTTP source URLs internally and (docling >= 2.102) swallowed fetch errors into an error-less "invalid" document, so transient upstream failures were neither retried nor surfaced -- the task reported success with an empty errors list. Fetch HTTP sources in the worker instead, reusing the shared async fetch and the existing failure classifier. Retryable failures (429/502/503/504, connection/timeout) are retried with a fixed delay; permanent failures (4xx, oversize) are not. A source that still cannot be fetched becomes a document-level FAILURE carrying the real cause, so a batch reports partial_success instead of silently dropping the document. - expand_task_sources() gains an optional http_materializer and now also returns per-input origin indices (fixing result attribution when one source expands to several inputs); ray/local callers updated. - max_task_retries (default 3) and retry_delay (default 5s) added to RQOrchestratorConfig. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Imber <dimber@m62.ai>
Contributor
|
✅ DCO Check Passed Thanks @dan-m62, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…etry Signed-off-by: Daniel Imber <dimber@m62.ai> # Conflicts: # docling_jobkit/convert/source_expansion.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On the RQ engine, an async conversion whose HTTP source URL is temporarily unavailable (for example a
503from the upstream host) is neither retried nor reported as a failure — the task completes as a success and the per-documenterrorsarray comes back empty. This PR makes the RQ worker fetch HTTP sources itself so those failures can be retried when they are transient and surfaced with their real cause when they are not.This follows up on docling-project/docling-serve#648 (enhancing 5xx handling for HTTP sources on the async engine), and adopts the same retry, classification and configuration patterns the Ray engine already uses, so the two engines behave consistently. The companion docling-serve change that exposes the new settings as environment variables is in docling-project/docling-serve#650
Background
On the RQ path,
HttpSourceURLs are handed to the converter as plain strings and docling fetches them internally. Since docling 2.102, a fetch error inside_DocumentConversionInput.docs()is caught and turned into an error-less "invalid" input document, so the exception never reaches the worker. The result is that a genuinely failed fetch looks like this:No retry, and no indication of why the document failed (a
503is indistinguishable from a404or a parse error).What this changes
HttpSourceinputs before conversion, reusing the existingfetch_http_source_bytes_asyncfromdocling_jobkit/convert/materialization.py, so docling receives bytes (aDocumentStream) rather than a URL.classify_public_task_failureclassifier:429,502,503,504and connection/timeout errors are retryable; permanent failures (401/403/404/413/415/422, oversize) are not and fail immediately. Retries use a fixed delay, up to a configurable maximum.FAILUREcarrying the real cause (built withbuild_public_error_item), so a batch reportspartial_successand the other sources still convert, rather than the source being silently dropped.RQOrchestratorConfig:max_task_retries(default3) andretry_delay(default5.0seconds), matching the Ray engine's names and defaults.expand_task_sourcesgains an optionalhttp_materializercallback and now also returns an origin-index list so callers can attribute conversion results back to the originating source. This incidentally fixes a latent mislabeling case where one source expands into several converter inputs (S3). The Ray and local callers are updated for the new return shape.Files
docling_jobkit/convert/http_retry.py— new:fetch_http_source_with_retryandbuild_http_failure_document.docling_jobkit/convert/source_expansion.py— optionalhttp_materializer, origin indices.docling_jobkit/orchestrators/rq/worker.py— materialize HTTP sources, collect per-source failures, merge them into the results.docling_jobkit/orchestrators/rq/orchestrator.py—max_task_retries/retry_delayonRQOrchestratorConfig.docling_jobkit/orchestrators/ray/serve_deployment.py,docling_jobkit/orchestrators/local/worker.py— updated for the newexpand_task_sourcesreturn shape.tests/test_http_retry.py— new tests;tests/test_rq_orchestrator.py,tests/test_s3_source_orchestrators.py— updated for the new signatures.Consistency with the Ray engine
The intent is to reuse what the Ray engine already established rather than invent a parallel mechanism:
classify_public_task_failure(whichclassify_ray_public_task_failurewraps) and the sameretryableflag._run_with_retry: a fixedretry_delaybetween attempts,max_task_retriesadditional attempts after the first._build_materialization_failure_result: anExportableDocumentwithstatus=ConversionStatus.FAILURE,errors=[build_public_error_item(exc)]andsource_uri=source_to_public_uri(source).max_task_retries/retry_delay.The retry uses a fixed delay (not exponential back-off) deliberately, to match Ray's
retry_delaysemantics.Behaviour change
Terminal HTTP fetch failures are now visible instead of silent. For a single-source task this means
status: "failure"with the cause recorded; for a batch it meanspartial_successwith a per-document failure. This is a deliberate improvement over the previous silent-success behaviour, but it is a behavioural change worth calling out for anyone currently relying on the old (empty-error) result.Testing
Unit tests cover the retry matrix (retry-then-succeed, exhaustion, no-retry on
404/oversize, connection-error retry, zero-retries), the failure-document builder, theexpand_task_sourcesseam, and the worker wiring (including a mixed good/bad batch producing a per-document failure).Beyond the mocked unit tests, I validated the real fetch path against a local server (real
httpx, real retry loop counting requests) and then ran the whole thing end-to-end against a running docling-serve API and RQ worker built from this branch. Against the same reproduction that produced the silent success above, the behaviour flips:Related
successfor unfetchable HTTP sources (no error surfaced, no retry) docling-serve#648