add Ollama health check endpoint#197
Conversation
|
🎉 Thank you @lakshay122007 for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
github-actions
Bot
added
backend
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
hi @ionfwsrijan @arpit2006 Please review it. Thanks! |
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
Approve with 2 minor (non-blocking) notes
The feature is correct, safe, and well-tested. Here's the full breakdown:
✅ What's Good
| Area | Detail |
|---|---|
| Core goal met | Endpoint never returns 500 on connection refusal — exactly what the linked issue required |
async with usage |
httpx.AsyncClient is correctly used as an async context manager, no resource leaks |
| Response schema | All three code paths (success, non-200, exception) return the same { available, models, base_url } shape |
| Test mock correctness | create_mock_client properly mocks __aenter__/__aexit__ — the correct way to mock an async context manager, a common gotcha |
| Test coverage | All 3 branches covered: online ✅, ConnectError ✅, non-200 status ✅ |
| No new dependency | httpx was already in requirements.txt |
⚠️ Two Minor Notes (Non-Blocking)
1. Overly-broad exception handler
# Current — catches everything including AttributeError, KeyError, etc.
except Exception as e:Genuine bugs inside the try block (e.g. a bad .json() parse) get silently swallowed. Recommend narrowing to:
except (httpx.ConnectError, httpx.TimeoutException, httpx.RequestError) as e:A corresponding test_ollama_health_timeout test case is also missing.
2. Hardcoded base_url
base_url = "http://localhost:11434" # not configurableUsers on Docker (http://ollama:11434) or a remote host can't configure this without a code change. A one-liner fix:
OLLAMA_BASE_URL = os.getenv("OLLAMA_BASE_URL", "http://localhost:11434")These are quality-of-life improvements, not blockers. The PR achieves its stated goal cleanly. Safe to approve — the two notes above can be addressed in follow-up issues if the maintainer agrees.
LGTM!
The remaining changes are Non - Blocking can be addressed in future refactor!
@ionfwsrijan , Ready to Merge!
|
@lakshay122007 , Approved! |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
hi @arpit2006 Made the minor changes as you mentioned.. |
3 participants
Linked issue
Closes #196
What this PR does
This PR introduces a dedicated health check endpoint (
GET /api/health/ollama) to monitor the local Ollama LLM service. It safely wraps thehttpxrequest to gracefully catch connection refusals and timeouts, ensuring the backend returns a cleanavailable: falseJSON payload instead of crashing with a500 Internal Server Error.Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
GET /api/health/ollamaroute inapp/main.pywith custom exception handling for timeouts/refusals.tests/test_health.pyutilizingAsyncMockto simulatehttpx.AsyncClientcontext managers for complete test coverage of both online and offline states.Frontend
New dependencies
Database / schema changes
Testing
How did you test this?
Executed the pytest suite (
python -m pytest tests/test_health.py --cov=app) which validates successful JSON parsing of model tags, as well as fallback behavior forhttpx.ConnectErrorand 404/500 status codes. All tests pass with 100% green coverage.Checklist
console.erroror unhandled Python exceptions introducedrequirements.txt/package.jsonupdated if new dependencies added.pkl,.pt, etc.) are gitignored, not committedAnything reviewers should focus on
The test file uses a custom
create_mock_clienthelper to properly mock the async context manager (async with httpx.AsyncClient()) so that FastAPI resolves the coroutine safely in the test client.Screenshots (if UI changed)