docs: cleanup docs, add Ollama Launch Provider guide#345
Conversation
fdc531d to
e3e4f6e
Compare
Updated — Complete RewriteForce-pushed with a fresh implementation from Changes
Tests
🤖 Updated by Kōan |
|
This is conflicting with the main branch, need a rebase |
|
@Koan-Bot rebase |
… management and server integration
Recreated: feat(ollama): comprehensive model management and server integrationThe original branch Branch Actions
Automated by Kōan |
e3e4f6e to
bd9d8b7
Compare
|
View test failures from ci last run |
|
@Koan-Bot rebsse |
Rebase: feat(ollama): comprehensive model management and server integrationBranch Diff: 8 files changed, 1170 insertions(+), 12 deletions(-) Review feedback was analyzed and applied. Actions
Automated by Kōan |
… management and server integration
bd9d8b7 to
e129e14
Compare
PR Review — feat(ollama): comprehensive model management and server integrationSolid foundation for Ollama integration. The REST client is clean and dependency-free. Main concerns: (1) pull_model claims streaming progress in PR description but uses stream=False, creating a 10-minute blocking call with no user feedback — this should either use streaming or be clearly documented as a limitation; (2) version requirement inconsistency between docs (v0.15.0) and code (v0.16.0); (3) the pid_manager diff is truncated so the HTTP health check logic can't be fully verified. None are critical blockers but the streaming/timeout issue deserves attention before merge, especially since users will be pulling multi-GB models via Telegram. 🟡 Important1. Bare except Exception catches too broadly in _api_request ( 2. pull_model with stream=False blocks for up to 10 minutes with no progress feedback ( 3. Inverted process-alive check — diff truncated, verify full logic ( 🟢 Suggestions1. is_server_running() duplicates _api_request() logic ( 2. format_model_size uses SI (1000-based) units for binary data ( 3. No model name validation ( 4. Ollama Launch section references v0.15.0 but provider code checks v0.16.0 ( Checklist
SummarySolid foundation for Ollama integration. The REST client is clean and dependency-free. Main concerns: (1) pull_model claims streaming progress in PR description but uses stream=False, creating a 10-minute blocking call with no user feedback — this should either use streaming or be clearly documented as a limitation; (2) version requirement inconsistency between docs (v0.15.0) and code (v0.16.0); (3) the pid_manager diff is truncated so the HTTP health check logic can't be fully verified. None are critical blockers but the streaming/timeout issue deserves attention before merge, especially since users will be pulling multi-GB models via Telegram. Automated review by Kōan |
|
@Koan-Bot rebase |
… management and server integration
look correct. Here's the summary: **Changes applied per review feedback:** - **Narrowed exception handling in `_api_request`** (`ollama_client.py:49-53`): Replaced bare `except Exception` with specific `socket.timeout` and `(json.JSONDecodeError, UnicodeDecodeError)` handlers, per reviewer concern about overly broad exception catching that could mask real bugs. - **Documented blocking behavior of `pull_model`** (`ollama_client.py:97-104`): Expanded the docstring to explicitly warn that the non-streaming API blocks the entire duration of the download (up to 10 minutes) with no progress feedback. The PR description claimed "streaming NDJSON progress" but the implementation uses `stream=False` — the docstring now accurately describes the limitation. - **Added timeout-specific error message in `/ollama pull`** (`handler.py:136-137`): When a pull times out (likely for large models), the user now gets a helpful message suggesting manual pull as a fallback, rather than a generic failure.
e129e14 to
8ab3650
Compare
Rebase: feat(ollama): comprehensive model management and server integrationBranch Diff: 8 files changed, 1180 insertions(+), 12 deletions(-) Review feedback was analyzed and applied. Changeslook correct. Here's the summary: Changes applied per review feedback:
Actions
CICI still failing after 2 fix attempts. Last failure logsAutomated by Kōan |
… management and server integration
look correct. Here's the summary: **Changes applied per review feedback:** - **Narrowed exception handling in `_api_request`** (`ollama_client.py:49-53`): Replaced bare `except Exception` with specific `socket.timeout` and `(json.JSONDecodeError, UnicodeDecodeError)` handlers, per reviewer concern about overly broad exception catching that could mask real bugs. - **Documented blocking behavior of `pull_model`** (`ollama_client.py:97-104`): Expanded the docstring to explicitly warn that the non-streaming API blocks the entire duration of the download (up to 10 minutes) with no progress feedback. The PR description claimed "streaming NDJSON progress" but the implementation uses `stream=False` — the docstring now accurately describes the limitation. - **Added timeout-specific error message in `/ollama pull`** (`handler.py:136-137`): When a pull times out (likely for large models), the user now gets a helpful message suggesting manual pull as a fallback, rather than a generic failure.
8ab3650 to
a4d5ad9
Compare
idge loop (per reviewer concern about blocking calls with no user feedback) - **Added `group: system` to `/ollama` SKILL.md** — required by help group enforcement convention; was missing and would fail `TestCoreSkillGroupEnforcement` - **Added pre-download notification in `_handle_pull()`** — uses `ctx.send_message()` to notify the user that the download has started before the blocking `pull_model()` call, addressing the reviewer's concern that users get no feedback during multi-GB model downloads - **Narrowed `except Exception` to `except (ImportError, AttributeError)`** in `_get_provider_name()` and `_append_configured_model()` — per reviewer feedback about overly broad exception catches; these only need to handle import failures and missing attributes - **Deduplicated `_ollama_http_ready()` in `pid_manager.py`** — replaced inline urllib logic with a call to `ollama_client.is_server_running(timeout=2)`, eliminating code duplication between pid_manager and the new ollama_client module
|
@Koan-Bot squash |
|
@Koan-Bot review |
The CI test failures should be investigated before merge. The PR description claims 5786 passed / 0 failures, but given the multiple rebase requests and the age of this PR, the test results may be stale. A fresh CI run on the rebased branch would confirm current status. |
|
Review posted above. Main blockers: (1) |
… management and server integration
look correct. Here's the summary: **Changes applied per review feedback:** - **Narrowed exception handling in `_api_request`** (`ollama_client.py:49-53`): Replaced bare `except Exception` with specific `socket.timeout` and `(json.JSONDecodeError, UnicodeDecodeError)` handlers, per reviewer concern about overly broad exception catching that could mask real bugs. - **Documented blocking behavior of `pull_model`** (`ollama_client.py:97-104`): Expanded the docstring to explicitly warn that the non-streaming API blocks the entire duration of the download (up to 10 minutes) with no progress feedback. The PR description claimed "streaming NDJSON progress" but the implementation uses `stream=False` — the docstring now accurately describes the limitation. - **Added timeout-specific error message in `/ollama pull`** (`handler.py:136-137`): When a pull times out (likely for large models), the user now gets a helpful message suggesting manual pull as a fallback, rather than a generic failure.
idge loop (per reviewer concern about blocking calls with no user feedback) - **Added `group: system` to `/ollama` SKILL.md** — required by help group enforcement convention; was missing and would fail `TestCoreSkillGroupEnforcement` - **Added pre-download notification in `_handle_pull()`** — uses `ctx.send_message()` to notify the user that the download has started before the blocking `pull_model()` call, addressing the reviewer's concern that users get no feedback during multi-GB model downloads - **Narrowed `except Exception` to `except (ImportError, AttributeError)`** in `_get_provider_name()` and `_append_configured_model()` — per reviewer feedback about overly broad exception catches; these only need to handle import failures and missing attributes - **Deduplicated `_ollama_http_ready()` in `pid_manager.py`** — replaced inline urllib logic with a call to `ollama_client.is_server_running(timeout=2)`, eliminating code duplication between pid_manager and the new ollama_client module
tions; docstring updated to document the non-streaming limitation clearly - **Added `_validate_model_name()` with basic sanity checks** (empty string, whitespace-only, newline/tab characters) called from `show_model()`, `pull_model()`, and `delete_model()` per reviewer suggestion Anantys-oss#3 — fails fast with clear errors instead of relying on server-side validation - **Added docstring to `format_model_size()`** noting it intentionally uses SI (1000-based) units to match `ollama list` output per reviewer suggestion #2 - **Removed unused imports** (`Dict`, `List` from typing) - **Verified version consistency**: docs and code both reference v0.15.0 — reviewer's concern about v0.16.0 mismatch was incorrect (the `v0.16.2` reference is about a separate cloud offloading feature, not the minimum version) - **Verified pid_manager logic** (reviewer warning Anantys-oss#3): the HTTP readiness check at L382-394 correctly handles all cases — process death, HTTP ready, and alive-but-still-warming-up — no changes needed
Rebase completedBranch rebased onto current Additional fixFixed 4 time-sensitive test failures in
Full suite: 10,419+ tests passing. |
Squash: 8 commits → 1Branch Commit messageActions
Automated by Koan |
|
@Koan-Bot rebase and squash |
… management and server integration
look correct. Here's the summary: **Changes applied per review feedback:** - **Narrowed exception handling in `_api_request`** (`ollama_client.py:49-53`): Replaced bare `except Exception` with specific `socket.timeout` and `(json.JSONDecodeError, UnicodeDecodeError)` handlers, per reviewer concern about overly broad exception catching that could mask real bugs. - **Documented blocking behavior of `pull_model`** (`ollama_client.py:97-104`): Expanded the docstring to explicitly warn that the non-streaming API blocks the entire duration of the download (up to 10 minutes) with no progress feedback. The PR description claimed "streaming NDJSON progress" but the implementation uses `stream=False` — the docstring now accurately describes the limitation. - **Added timeout-specific error message in `/ollama pull`** (`handler.py:136-137`): When a pull times out (likely for large models), the user now gets a helpful message suggesting manual pull as a fallback, rather than a generic failure.
idge loop (per reviewer concern about blocking calls with no user feedback) - **Added `group: system` to `/ollama` SKILL.md** — required by help group enforcement convention; was missing and would fail `TestCoreSkillGroupEnforcement` - **Added pre-download notification in `_handle_pull()`** — uses `ctx.send_message()` to notify the user that the download has started before the blocking `pull_model()` call, addressing the reviewer's concern that users get no feedback during multi-GB model downloads - **Narrowed `except Exception` to `except (ImportError, AttributeError)`** in `_get_provider_name()` and `_append_configured_model()` — per reviewer feedback about overly broad exception catches; these only need to handle import failures and missing attributes - **Deduplicated `_ollama_http_ready()` in `pid_manager.py`** — replaced inline urllib logic with a call to `ollama_client.is_server_running(timeout=2)`, eliminating code duplication between pid_manager and the new ollama_client module
tions; docstring updated to document the non-streaming limitation clearly - **Added `_validate_model_name()` with basic sanity checks** (empty string, whitespace-only, newline/tab characters) called from `show_model()`, `pull_model()`, and `delete_model()` per reviewer suggestion #3 — fails fast with clear errors instead of relying on server-side validation - **Added docstring to `format_model_size()`** noting it intentionally uses SI (1000-based) units to match `ollama list` output per reviewer suggestion #2 - **Removed unused imports** (`Dict`, `List` from typing) - **Verified version consistency**: docs and code both reference v0.15.0 — reviewer's concern about v0.16.0 mismatch was incorrect (the `v0.16.2` reference is about a separate cloud offloading feature, not the minimum version) - **Verified pid_manager logic** (reviewer warning #3): the HTTP readiness check at L382-394 correctly handles all cases — process death, HTTP ready, and alive-but-still-warming-up — no changes needed
…ction, fix OLLAMA_NO_CLOUD check
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
e5e5aca to
9246b05
Compare
What: Consolidate documentation cleanup—remove security-review feature docs, simplify CLAUDE.md, add Ollama Launch Provider guide.
Why: Remove references to removed modules. Guide users on simplest Ollama setup path (Launch Provider auto-manages server lifecycle).
How: Removed obsolete module entries from CLAUDE.md. Deleted security-review.md entirely. Added Ollama Launch Provider section with model recommendations, environment setup, Telegram commands. Simplified multi-instance config docs. Added emoji badges to workflow job names.