Fix: harden Ollama request handling and response validation#233
Fix: harden Ollama request handling and response validation#233Aryama-srivastav wants to merge 3 commits intofireform-core:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens the Ollama request path used by LLM.main_loop() to avoid hangs and provide clearer, field-aware failures when Ollama is slow/unavailable or returns bad responses, and adds focused regression tests for those failure modes.
Changes:
- Add a request timeout and more specific exception handling/validation for the Ollama request + JSON response parsing.
- Add reliability tests covering timeout, HTTP error, invalid JSON, and missing
responsekey scenarios. - Update the sample input text used by the project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/llm.py |
Adds timeout + explicit error handling and response validation for the Ollama call in main_loop(). |
src/test/test_llm_reliability.py |
Adds tests to validate behavior for timeout/HTTP/JSON/missing-key failure paths. |
src/inputs/input.txt |
Replaces the example input text used for extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| except requests.exceptions.ConnectionError: | ||
| raise ConnectionError( | ||
| f"Could not connect to Ollama at {ollama_url}. " | ||
| "Please ensure Ollama is running and accessible." | ||
| ) |
There was a problem hiding this comment.
The connection error path raises a built-in ConnectionError without including the field context, and it differs from the PR description which states transport-layer failures should raise a contextual RuntimeError. Consider including the field name and chaining the original exception (and/or using a consistent exception type across transport/HTTP failures) so callers can handle failures uniformly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
This PR fixes reliability issues in the Ollama request path used by LLM extraction.
Previously, the request flow could hang without a timeout, fail with low-context errors on transport/HTTP failures, and crash with unclear behavior when the response payload was malformed or missing the response field.
This change makes the request path resilient and explicit:
Fixes #228
Type of change
Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Test A (timeout path):
Ran:
python -m pytest [test_llm_reliability.py] -q -k timeout
Verified behavior:
Raises TimeoutError
Error message includes field and Ollama URL context
Test B (HTTP error path):
Ran:
python -m pytest [test_llm_reliability.py] -q -k http_error
Verified behavior:
Raises RuntimeError for HTTP failure
Message includes field-specific context
Test C (invalid JSON path):
Ran:
python -m pytest [test_llm_reliability.py] -q -k invalid_json
Verified behavior:
Raises ValueError with invalid JSON context
Test D (missing response key path):
Ran:
python -m pytest [test_llm_reliability.py] -q -k missing_response_key
Verified behavior:
Raises ValueError indicating missing response key
Full focused test run:
Ran:
python -m pytest [test_llm_reliability.py] -q
Verified output:
4 passed in 0.16s
Test Configuration:
Firmware version: N/A
Hardware: Local development machine (Windows)
SDK: N/A
Python: 3.13
Shell: PowerShell
Checklist:
My code follows the style guidelines of this project