diff --git a/.env.example b/.env.example index 33a22c4..65286a1 100644 --- a/.env.example +++ b/.env.example @@ -213,11 +213,17 @@ UPLOAD_AUTORUN=true # exists to avoid. Stage 6 is the slowest stage and reaches the network, but # only one batch runs at a time, so nothing else competes with it. # -# LLM off, because use_llm gates only description generation in stage 2, and -# production runs USE_OLLAMA=false - turning it on there buys nothing and costs -# a connection timeout per row. +# LLM on, though in production it currently does nothing: USE_OLLAMA is false +# there, so the LLM call returns immediately without a request and the row keeps +# its blank description. Set true so the pipeline is already right for the day +# an Ollama server is reachable. +# +# If you DO set USE_OLLAMA=true, make sure something is actually listening on +# OLLAMA_BASE_URL. An unreachable server costs a 5s probe, and while that probe +# is cached per 30s rather than paid per row, a reachable-but-slow model is +# billed per row at OLLAMA_TIMEOUT_SECONDS. UPLOAD_AUTORUN_FETCH_IMAGES=true -UPLOAD_AUTORUN_USE_LLM=false +UPLOAD_AUTORUN_USE_LLM=true USE_OLLAMA=true OLLAMA_BASE_URL=http://localhost:11434 diff --git a/app/infrastructure/settings.py b/app/infrastructure/settings.py index 17e3fbd..16b7b1b 100644 --- a/app/infrastructure/settings.py +++ b/app/infrastructure/settings.py @@ -190,11 +190,21 @@ UPLOAD_AUTORUN = _bool("UPLOAD_AUTORUN", "true") # exists to avoid - stage 6 is the slowest stage and reaches the network, but # only one batch runs at a time so nothing else is competing with it. # -# LLM OFF, because `use_llm` gates only description generation in -# stage_2_row_intake, and production runs USE_OLLAMA=false: turning it on there -# buys nothing and costs a connection timeout per row. +# LLM ON. `use_llm` gates only description generation in stage_2_row_intake, and +# in production it is currently a no-op: USE_OLLAMA is false there, so +# ollama_service._ensure_client() returns on its first line without a request +# and the row simply keeps its blank description. It is set true so the pipeline +# is already configured correctly for the day an Ollama server exists. +# +# This default USED to be false, on the grounds that turning it on "costs a +# connection timeout per row". That was true, and it was about the OTHER branch +# of _ensure_client - USE_OLLAMA=true with nothing listening, which is any +# developer machine that has not run `ollama serve`. It is answered now by the +# TTL cache on that probe rather than by leaving the feature off: one probe per +# batch instead of one per row. Do not remove that cache and this default +# together without re-reading why both exist. UPLOAD_AUTORUN_FETCH_IMAGES = _bool("UPLOAD_AUTORUN_FETCH_IMAGES", "true") -UPLOAD_AUTORUN_USE_LLM = _bool("UPLOAD_AUTORUN_USE_LLM", "false") +UPLOAD_AUTORUN_USE_LLM = _bool("UPLOAD_AUTORUN_USE_LLM", "true") # --- Review inbox ---------------------------------------------------------- # The bound that applies only when UPLOAD_AUTORUN is false. Files then wait in diff --git a/app/services/ollama_service.py b/app/services/ollama_service.py index dddcbd5..dd51716 100644 --- a/app/services/ollama_service.py +++ b/app/services/ollama_service.py @@ -3,6 +3,8 @@ from __future__ import annotations from typing import List, Dict, Any, Optional import json import re +import time + import requests from app.infrastructure.settings import OLLAMA_BASE_URL, OLLAMA_MODEL_NAME, USE_OLLAMA, OLLAMA_TIMEOUT_SECONDS @@ -20,15 +22,58 @@ SYSTEM_PROMPT = ( ) +# Reachability is asked once per this many seconds, not once per caller. +# +# WHY THIS CACHE EXISTS. The probe below costs up to 5 seconds when nothing is +# listening, and `_ensure_client` is called per ROW by stage 2 of the ingestion +# pipeline (store_catalog_pipeline.stage_2_row_intake -> fetch_product_details). +# Uncached, a 2000-row sheet ingested with use_llm on, against a configured but +# unreachable Ollama, spends up to ~2.8 hours doing nothing but timing out - and +# presents as a batch that has hung rather than one that has failed. That is not +# hypothetical: USE_OLLAMA=true pointing at localhost:11434 is the default +# developer configuration, and `ollama serve` is not always running beside it. +# +# /api/health calls this too (app/api/routers/system.py), so the same cache +# stops a down Ollama adding 5s to every health request. +# +# A TTL rather than a permanent memo, deliberately: this is a liveness fact, not +# configuration. Cached forever, an Ollama started after the API would never be +# noticed and /api/health would report it down until a redeploy. +_PROBE_TTL_SECONDS = 30.0 +_probe_cache: tuple[float, bool] | None = None + + +def reset_reachability_cache() -> None: + """Forget the cached probe. For tests, and for anything that knows the + answer just changed.""" + global _probe_cache + _probe_cache = None + + def _ensure_client(): + """None when Ollama is switched off, True/False for reachable or not. + + Three return values, not two - `system.py` relies on telling "disabled" from + "configured but down", so do not collapse this to a bool. + """ if not USE_OLLAMA: + # No network call on this path, so nothing worth caching. return None + + global _probe_cache + now = time.monotonic() + if _probe_cache is not None and now - _probe_cache[0] < _PROBE_TTL_SECONDS: + return _probe_cache[1] + # Verify Ollama is reachable try: resp = requests.get(f"{OLLAMA_BASE_URL}/api/tags", timeout=5) - return resp.status_code == 200 + reachable = resp.status_code == 200 except Exception: - return False + reachable = False + + _probe_cache = (now, reachable) + return reachable def _generate(system: str, user_prompt: str, max_retries: int = 2) -> str: diff --git a/docs/INGESTION_API.md b/docs/INGESTION_API.md index aa03852..8227fff 100644 --- a/docs/INGESTION_API.md +++ b/docs/INGESTION_API.md @@ -116,9 +116,14 @@ curl -X POST https://mcp.nearle.ai.in/api/uploads/catalog \ | `files` | **required** | The spreadsheets. Repeat the field for more than one; up to 20. | | `sender` | optional | A label for the inbox, so the admin can see who sent what. Free text, trimmed to 60 chars. Defaults to `anonymous`. | -`use_llm` and `fetch_images` are **no longer accepted here.** They decide how a run -behaves and commit the host to outbound work, so the choice belongs to the admin -pressing Start — not to the sender. Passing them is inert. +`use_llm` and `fetch_images` are **not accepted here.** They commit the host to +outbound work, and this endpoint's caller is anonymous, so the choice is not theirs to +make. Passing them is inert — the response reports what was actually used. + +Both are **on** for an auto-started run: image search fills the product images, and the +LLM fills blank descriptions wherever an Ollama server is reachable. There is none in +production today, so in practice descriptions arrive exactly as your sheet wrote them +and the run is otherwise unaffected. ### Which columns are read @@ -163,7 +168,7 @@ The bad file is kept as a failed member rather than dropped, so a sender who sub "files_done": 0, "files_failed": 1, "current_file": null, - "use_llm": false, // UPLOAD_AUTORUN_USE_LLM + "use_llm": true, // UPLOAD_AUTORUN_USE_LLM "fetch_images": true, // UPLOAD_AUTORUN_FETCH_IMAGES "runner": "inprocess", "totals": { "rows_total": 0, "products_built": 0, "inserted": 0, diff --git a/tests/test_ollama_reachability.py b/tests/test_ollama_reachability.py new file mode 100644 index 0000000..db7ea5e --- /dev/null +++ b/tests/test_ollama_reachability.py @@ -0,0 +1,121 @@ +"""The reachability probe in front of every Ollama call. + +WHY THIS FILE EXISTS +-------------------- +`_ensure_client()` asks Ollama for `/api/tags` with a 5-second timeout, and +`stage_2_row_intake` calls it once per ROW through `fetch_product_details`. +Uncached, a 2000-row sheet ingested with `use_llm` on, against a configured but +unreachable Ollama, spends up to ~2.8 hours doing nothing but timing out - and +shows as a batch that has hung, not one that has failed. + +That was survivable only while `use_llm` defaulted to false everywhere. It no +longer does: `UPLOAD_AUTORUN_USE_LLM` is true, so every auto-started upload now +takes this path. The cache is what makes that default safe, and the first test +below is the one that stops it being quietly removed in a later refactor. + +`/api/health` calls the same function, so a down Ollama also stops adding five +seconds to every health request. +""" +from __future__ import annotations + +import pytest + +from app.services import ollama_service + + +@pytest.fixture(autouse=True) +def _clean_probe_cache(): + """The cache is a module global and outlives a test.""" + ollama_service.reset_reachability_cache() + yield + ollama_service.reset_reachability_cache() + + +@pytest.fixture +def probe_calls(monkeypatch): + """Count the HTTP probes, and make every one of them fail. + + Failure is the case that matters: a reachable Ollama answers in + milliseconds, an unreachable one costs the full timeout, and it is the + second that used to be paid per row. + """ + calls: list = [] + + def boom(url, **kwargs): + calls.append(url) + raise OSError("connection refused") + + monkeypatch.setattr(ollama_service, "USE_OLLAMA", True) + monkeypatch.setattr(ollama_service.requests, "get", boom) + return calls + + +def test_an_unreachable_ollama_is_probed_once_not_once_per_call(probe_calls): + """The whole point. Ten rows must not be ten timeouts.""" + for _ in range(10): + assert ollama_service._ensure_client() is False + + assert len(probe_calls) == 1, ( + f"{len(probe_calls)} probes for 10 calls - the cache is not holding, and " + f"an ingest will pay the 5s timeout per row" + ) + + +def test_the_cache_expires_so_a_late_start_is_noticed(probe_calls, monkeypatch): + """A permanent memo would mean an Ollama started after the API is never + seen, and /api/health reports it down until someone redeploys.""" + clock = [1000.0] + monkeypatch.setattr(ollama_service.time, "monotonic", lambda: clock[0]) + + ollama_service._ensure_client() + assert len(probe_calls) == 1 + + clock[0] += ollama_service._PROBE_TTL_SECONDS + 1 + ollama_service._ensure_client() + assert len(probe_calls) == 2, "the probe never expired" + + +def test_a_reachable_ollama_is_also_cached(monkeypatch): + """Both outcomes are cached. Caching only the failure would leave the happy + path paying an HTTP round trip per row - cheap, but per row and pointless.""" + calls: list = [] + + class Ok: + status_code = 200 + + def ok(url, **kwargs): + calls.append(url) + return Ok() + + monkeypatch.setattr(ollama_service, "USE_OLLAMA", True) + monkeypatch.setattr(ollama_service.requests, "get", ok) + + assert [ollama_service._ensure_client() for _ in range(5)] == [True] * 5 + assert len(calls) == 1 + + +def test_disabled_stays_none_and_never_touches_the_network(monkeypatch): + """Three return values, not two: `system.py` tells "switched off" from + "configured but down", and /api/health's `ollama` field means different + things in each case. Collapsing this to a bool would break that. + """ + def never(*_args, **_kwargs): + raise AssertionError("USE_OLLAMA is false - nothing may be requested") + + monkeypatch.setattr(ollama_service, "USE_OLLAMA", False) + monkeypatch.setattr(ollama_service.requests, "get", never) + + assert ollama_service._ensure_client() is None + + +def test_a_row_keeps_its_description_when_ollama_is_absent(probe_calls): + """The pipeline's side of the contract. `use_llm` being on must be a no-op + against an unreachable server, not a failure - stage 2 is best-effort and + every later stage has to keep working.""" + from app.core.store_catalog_pipeline import stage_2_row_intake + + row = stage_2_row_intake( + {"product_name": "Amul Butter 100g", "brand": "Amul", "description": ""}, + use_llm=True, + ) + assert row["description"] == "" diff --git a/tests/test_uploads_autorun.py b/tests/test_uploads_autorun.py index c7deb85..8df6800 100644 --- a/tests/test_uploads_autorun.py +++ b/tests/test_uploads_autorun.py @@ -127,31 +127,44 @@ def test_the_run_is_attributed_to_the_sender_label(client): # --------------------------------------------------------------------------- # What the anonymous caller still cannot do # --------------------------------------------------------------------------- -def test_the_sender_cannot_switch_on_the_expensive_stages(client): +def test_the_request_cannot_move_either_stage(client): """Image search and the LLM reach the network and are chosen by settings, never by the request. An anonymous caller who could flip these could commit - a one-vCPU host to outbound work at will.""" + a one-vCPU host to outbound work at will. + + Both fields are sent OPPOSITE to their configured values, which is the only + way this proves anything: if the endpoint started honouring the request, the + response would have to disagree with the settings, and it must not. + """ body = client.post( UPLOAD, files=_files(("a.csv", _csv())), - data={"use_llm": "true", "fetch_images": "false"}, + data={"use_llm": "false", "fetch_images": "false"}, ).json() - # The configured values, not the ones asked for. - assert body["use_llm"] is False + assert body["use_llm"] is True assert body["fetch_images"] is True +def test_both_stages_are_on_for_an_auto_started_run(client): + """The requirement, pinned against the shipped defaults rather than against + a monkeypatched pair - so flipping either default in settings.py fails here + rather than passing quietly.""" + body = client.post(UPLOAD, files=_files(("a.csv", _csv()))).json() + assert body["fetch_images"] is True + assert body["use_llm"] is True + + def test_the_configured_defaults_are_what_reach_the_run(client, monkeypatch): """Both directions, so a wrong default cannot hide behind a matching one.""" from app.api.routers import uploads monkeypatch.setattr(uploads, "UPLOAD_AUTORUN_FETCH_IMAGES", False) - monkeypatch.setattr(uploads, "UPLOAD_AUTORUN_USE_LLM", True) + monkeypatch.setattr(uploads, "UPLOAD_AUTORUN_USE_LLM", False) body = client.post(UPLOAD, files=_files(("a.csv", _csv()))).json() assert body["fetch_images"] is False - assert body["use_llm"] is True + assert body["use_llm"] is False # ---------------------------------------------------------------------------