Fix/translation startup - #22
Open
Ayan-josh-05 wants to merge 2 commits into
Open
Conversation
…zing/unreachable status Previously /health called Ollama synchronously and blocked for the full 2+ minute cold model load. A background monitor now pings Ollama with an untimed chat() call on its own task, and /health just reads the last observed state instantly. Retries fast on failure, backs off after 12 consecutive failures, and keeps reconfirming "ok" so a later Ollama outage is caught too. As a side effect, the monitor's first ping also warms up the model automatically on startup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…120s The gateway proxied every backend call through one shared 120s httpx timeout, but OCR/translation/field-mapping can legitimately run for minutes (cold Ollama model loads, multi-page OCR) — the frontend already budgets 5 minutes for these same calls. The gateway was giving up first, returning a spurious 503 while the backend was still working. Added OCR_REQUEST_TIMEOUT_SECONDS / TRANSLATION_REQUEST_TIMEOUT_SECONDS / FIELD_MAPPING_REQUEST_TIMEOUT_SECONDS (default 300s each, env-overridable) and threaded them through _proxy(); health-check proxying keeps a short 5s timeout separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new translation health monitor can cause readiness flapping and has incomplete shutdown/cancellation handling that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves startup reliability for the containerized lending POC by (1) making the translation service /health endpoint non-blocking during Ollama cold starts via a background monitor, and (2) adjusting the gateway to use per-service request timeouts so long-running OCR/translation/field-mapping operations don’t fail prematurely.
Changes:
- Introduces per-service, env-configurable gateway timeouts for OCR/translation/field-mapping requests while keeping health-proxy timeouts short.
- Adds a background Ollama readiness monitor and exposes a non-blocking health status API (
ok/initializing/unreachable). - Updates compose + env examples to reflect the new timeout configuration knobs.
File summaries
| File | Description |
|---|---|
| lending-poc/gateway/main.py | Adds per-service proxy timeouts and keeps health proxy calls short. |
| lending-poc/document_processing/translation/translation_service/translator.py | Exposes adapter-backed non-blocking health status and monitor lifecycle methods. |
| lending-poc/document_processing/translation/translation_service/config.py | Adds env-configurable monitor cadence/backoff settings. |
| lending-poc/document_processing/translation/translation_service/adapters/ollama_adapter.py | Implements background monitor loop and cached health status for Ollama. |
| lending-poc/document_processing/translation/translation_service/adapters/base.py | Extends adapter interface with health_status + monitoring hooks. |
| lending-poc/document_processing/translation/api/routes.py | Updates /health route to return cached status + human-readable detail. |
| lending-poc/document_processing/translation/api/models.py | Tightens /health schema with explicit status literals and adds detail. |
| lending-poc/document_processing/translation/api_server.py | Starts/stops the health monitor during app lifespan. |
| lending-poc/docker-compose.yml | Wires per-service gateway timeout env vars into the gateway container. |
| lending-poc/.env.example | Documents the new gateway timeout env vars. |
Review details
Suppressed comments (1)
lending-poc/document_processing/translation/translation_service/adapters/ollama_adapter.py:113
- _monitor_loop() sets self._status = "initializing" on every iteration, which will temporarily downgrade /health from "ok" to "initializing" during routine rechecks (even when Ollama is healthy). This can cause readiness to flap once the service is already ready.
while True:
self._status = "initializing"
try:
await asyncio.to_thread(self._ping)
self._status = "ok"
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+94
to
+98
| async def stop_monitoring(self) -> None: | ||
| """Stop the background monitor started by start_monitoring().""" | ||
| if self._monitor_task and not self._monitor_task.done(): | ||
| self._monitor_task.cancel() | ||
|
|
Comment on lines
+51
to
+57
| Return one of "ok", "initializing", "unreachable" for the FastAPI | ||
| /health endpoint. Non-blocking by contract — should be an instant read | ||
| of previously-observed state, not a fresh call to the backend. | ||
|
|
||
| Default implementation falls back to a blocking health_check() call, | ||
| for adapters that don't track finer-grained state. Override alongside | ||
| start_monitoring()/stop_monitoring() to report real-time state instead. |
Comment on lines
84
to
87
| return HealthResponse( | ||
| status="ok" if reachable else "degraded", | ||
| status=status, | ||
| detail=STATUS_DETAIL[status], | ||
| model=MODEL_NAME, |
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.
Fix Translation Health Checks and Gateway Request Timeouts
Summary
Fixed two startup and reliability issues found during end-to-end testing of the containerized stack:
/healthendpoint could block for several minutes while Ollama loaded the model.The fixes make translation health checks non-blocking and introduce realistic, per-service gateway timeouts.
What's Fixed
1. Translation
/healthno longer blocks on Ollama cold loadsPreviously,
/healthcalled Ollama synchronously. During a cold model load, this could block for 2+ minutes, causing orchestrators or load balancers polling the endpoint to time out instead of receiving a quick "not ready" response.Now, a background monitor task checks Ollama independently, while
/healthsimply returns the latest observed state immediately:ok— model is loaded and respondinginitializing— Ollama is reachable, but the model is still loadingunreachable— Ollama is not reachableThe monitor:
OLLAMA_HEALTH_RETRY_SECONDS(default: 5s)OLLAMA_HEALTH_MAX_FAST_RETRIES(default: 12) consecutive fast retriesOLLAMA_HEALTH_BACKOFF_SECONDS(default: 120s) after repeated failuresOLLAMA_HEALTH_RECHECK_SECONDS(default: 30s)The first background ping also warms up the model automatically during startup.
Commit:
226bdd72. Gateway no longer uses a flat 120s timeout
Previously, the gateway used a shared 120-second
httpxtimeout for all backend requests. This was too short for operations such as:The frontend already allows up to 5 minutes for these requests, but the gateway could terminate them first and return a spurious 503 while the backend was still processing.
Added separate, environment-configurable timeouts for each service:
OCR_REQUEST_TIMEOUT_SECONDSTRANSLATION_REQUEST_TIMEOUT_SECONDSFIELD_MAPPING_REQUEST_TIMEOUT_SECONDSAll default to 300 seconds (5 minutes) and are passed through
_proxy().Health-check proxy requests retain a separate 5-second timeout, since health checks are now expected to respond immediately.
Commit:
b0b1860How to Test
1. Start the stack
cd lending-poc docker compose up --build2. Test translation health
Hit the translation service's
/healthimmediately after startup.Expected behavior:
status: "initializing"status: "ok"once the background monitor successfully pings OllamaThe endpoint should no longer hang during the initial model load.
3. Test long-running gateway requests
After a cold start, trigger an OCR, translation, or field-mapping request through the gateway before Ollama has warmed up.
Expected behavior:
4. Optional configuration testing
The following variables can be overridden through
.envto verify the tuning behavior:Result
Translation health checks are now fast and non-blocking, while the gateway can accommodate legitimately long-running backend operations without prematurely returning 503 errors.