diff --git a/specs/003-model-configuration/plan.md b/specs/003-model-configuration/plan.md new file mode 100644 index 0000000..1f348be --- /dev/null +++ b/specs/003-model-configuration/plan.md @@ -0,0 +1,157 @@ +# Implementation Plan: Model Configuration + +**Branch**: `feat/model-configuration` | **Date**: 2026-09-10 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/003-model-configuration/spec.md` + +## Summary + +Move the answering model into `config.yml`, where a deployment already declares its +profiles, quotas, features and messages. Keep the model's *call requirements* — +temperature, structured-output method — derived in code, and refuse at startup a +configuration that contradicts them. + +Harvests the LLM half of #112 (@AaryanCode69) and #151 (@bhavyakeerthi3), and +rejects the embedding half of both. + +## Technical Context + +**Language/Version**: Python 3.12 + +**Primary Dependencies**: pydantic v2 (`Config` is already a `BaseModel`), +`langchain-openai` 1.6 via `agent.models.get_llm`. + +**Storage**: `config.yml` at the repo root, bind-mounted read-only into the +container; `config_default.yml` shipped in the image as the fallback. + +**Testing**: pytest. `tests/util/test_config.py` already pins the loader's failure +behaviour, including that an invalid file is fatal rather than silently defaulted. +`tests/agent/test_model_temperature.py` pins the derived table. + +**Target Platform**: Linux container, and developer machines. + +**Project Type**: Library within a single application repository. + +**Performance Goals**: None. This is a startup-time concern; it runs once. + +**Constraints**: A deployment with no model configured must behave exactly as it +does today (FR-002). `resolve_embedding_model()` must remain the only source of the +embedding model (FR-004). + +**Scale/Scope**: One new config section, one validation, four call sites at most. + +## Constitution Check + +*GATE: must pass before implementation, re-checked after.* + +| Article | How this plan satisfies it | +|---|---| +| I — verify the user path | The exit criterion is the server starting with a model set in `config.yml` and answering a question with it — not a unit test asserting the field parses. | +| II — measure, don't argue | Nothing here changes retrieval, so no baseline is owed. The model *choice* is spec 002's measurement, and this plan deliberately does not make it. | +| III — characterization tests pin behaviour | `test_config.py`'s existing tests must pass untouched: adding a section must not change what an invalid config does. | +| IV — fail loudly | The whole point of Stage 2. A contradictory model/temperature pair stops startup; it does not get silently corrected. | +| V — derive from the source of truth | FR-004 and FR-005 are this article restated: the embedding model comes from the bundle, the temperature from the measured table. Neither becomes configurable. | +| VI — bias to doing over filing | Two contributed PRs are harvested here rather than left open another six months. | + +**No violations.** The one judgement call is D1 (validate against the table, not the +API), taken as recommended in the spec: no network call at startup. + +## Project Structure + +### Documentation (this feature) + +```text +specs/003-model-configuration/ +├── spec.md # what and why, with D1 recommended +├── plan.md # this file +└── checklists/ + └── requirements.md +``` + +No `research.md`: there are no unresolved unknowns. The two contributed PRs are the +research, the temperature behaviour was measured empirically in #189, and D1 is +decided. No `data-model.md` or `contracts/` — the "model" is three optional fields +on an existing pydantic class, described below in full, and the only external +contract is `config.yml`, whose schema file is edited in Stage 1. + +### Source Code + +```text +src/util/config_yml/ +├── __init__.py # Config gains an optional `llm` section +└── models.py # new: LLMConfig, after #112's shape + +src/agent/ +├── graph.py # AgentGraph reads the config; resolve_temperature validates +└── models.py # unchanged -- get_llm already takes model and temperature + +.config.schema.yaml # the editor-facing schema gains the same section +config_default.yml # documents the section without setting it +``` + +## Implementation Stages + +### Stage 1 — The model becomes configurable, with today's behaviour as the default + +Add `LLMConfig` (`provider`, `model`, `base_url`) and an optional `llm` field on +`Config`. `AgentGraph` prefers it, falling back to `LLM_MODEL`, then to the current +default. + +Precedence is **environment over file** (FR-003), the opposite of Docker secrets in +`util/secrets.py`, and the difference is worth stating: a secret is mounted *by* the +deployment and should beat a checked-in file, whereas `LLM_MODEL` is how an operator +overrides a committed `config.yml` for one container. Both rules are "the more +specific thing wins"; they only look contradictory. + +Nothing is required. A `config.yml` with no `llm:` section behaves exactly as today, +which is what keeps every existing deployment working (FR-002). + +**Exit criteria**: `test_config.py` passes untouched; a `config.yml` naming +`gpt-5.6-luna` starts the server and answers a question with it; the effective model +appears in the startup log (FR-008). + +### Stage 2 — A contradictory configuration stops startup + +`resolve_temperature` already knows which models refuse `0.0`. If `config.yml` (or +`LLM_TEMPERATURE`) sets a temperature a model will reject, startup fails naming the +model, the value, and the fix. + +This is the reason the feature is a specification rather than a bump. Today that +mistake is a 400 on a user's first question — visible to a user, attributed to the +chatbot, and diagnosable only from logs. + +Per D1 the check is against the table alone. No API call, so it is instant, works +offline, and cannot make startup depend on OpenAI being reachable. A model the table +has not met does not block startup (FR-007). + +**Exit criteria**: `gpt-5.6-luna` with `temperature: 0` refuses to start and names +all three of model, value and fix; an unknown model starts normally; the existing +`LLM_TEMPERATURE` escape hatch still wins. + +### Stage 3 — Close #112 and #151 + +Both are superseded by Stages 1–2. Close them courteously, crediting each +contributor in the commit that lands their idea, and say plainly why the embedding +half was rejected — it bypasses `resolve_embedding_model()` and would silently break +Plant Reactome retrieval. + +**Exit criteria**: both PRs closed with credit; `.config.schema.yaml` and +`config_default.yml` document the new section; no configuration file can name an +embedding model (SC-004). + +## Complexity Tracking + +| Decision | Simpler alternative rejected | Why | +|---|---|---| +| A nested `llm:` section | A flat `llm: "openai/gpt-4o-mini"` string, as #151 does | Re-parsing a string to recover provider and model is work the loader can do once. `base_url` has nowhere to live in a flat string, and Plant Reactome needs it. | +| Validate at startup | Validate on first use | First use is a user's first question. The whole point is that the operator learns before a user does. | +| Table only, no probe | One API call at startup | D1. It catches the mistake actually being made, for free and offline. A mistyped model name fails immediately and unmistakably anyway. | +| Leave the embedding model alone | Make it configurable "for symmetry", as both PRs do | It is derived from the bundle that built the vectors. Configuring it is how you get silent nonsense instead of an error. | + +## Out of Scope + +- **Which model becomes the default.** [Spec 002](../002-default-llm-choice/spec.md), + gated on an answer-quality run. This plan makes the choice expressible, not made. +- Per-surface model selection (spec 003 User Story 3). Only chat exists today; the + nesting introduced in Stage 1 is what makes it cheap later. +- The embedding model, permanently. diff --git a/specs/003-model-configuration/quickstart.md b/specs/003-model-configuration/quickstart.md new file mode 100644 index 0000000..46b8756 --- /dev/null +++ b/specs/003-model-configuration/quickstart.md @@ -0,0 +1,90 @@ +# Quickstart: verifying model configuration + +How to prove this feature works, end to end, the way a deployment uses it. Every +step here is a thing that can be observed failing, not a unit test. + +## Prerequisites + +- an installed reactome bundle (`./bin/embeddings_manager which`) +- `OPENAI_API_KEY` in `.env` + +## 1. Nothing configured behaves exactly as before (FR-002) + +The most important scenario, because it is what every existing deployment does. + +```bash +grep -c '^llm:' config.yml || echo "no llm section — good" +poetry run chainlit run bin/chat-chainlit.py +``` + +**Expect**: the server starts and the startup log names `gpt-4o-mini`. Ask a +question; it answers. + +## 2. A model set in config.yml is the one that answers (FR-001, FR-008) + +```yaml +# config.yml +llm: + provider: openai + model: gpt-5.6-luna +``` + +**Expect**: the startup log names `gpt-5.6-luna`, and `resolve_temperature` has +silently sent `1.0` — that model refuses `0.0` and nobody had to know. + +## 3. The environment overrides the file (FR-003) + +```bash +LLM_MODEL=gpt-4o-mini poetry run chainlit run bin/chat-chainlit.py +``` + +**Expect**: `gpt-4o-mini` in the log, despite `config.yml` naming luna. This is how +one container is overridden without editing a committed file. + +## 4. A contradictory pair stops startup (FR-006, SC-003) + +```yaml +llm: + model: gpt-5.6-luna + temperature: 0 +``` + +**Expect**: the server **refuses to start**, naming the model, the value, and the +fix. It must not start and fail later — that is the entire point of Stage 2. + +Compare with what happens without this feature: the server starts, and the first +user to ask a question gets an error that reads as the chatbot being broken. + +## 5. A model the table has not met still starts (FR-007) + +```yaml +llm: + model: gpt-7-whatever +``` + +**Expect**: the server starts. An unknown model is not an error — the table is +empirical and always behind. It will fail on the first request with OpenAI's own +404, which is unambiguous, and `LLM_TEMPERATURE` remains the escape hatch. + +## 6. No configuration file can name an embedding model (FR-004, SC-004) + +```bash +grep -rn "embedding" .config.schema.yaml config_default.yml +``` + +**Expect**: no `embedding` model field anywhere. The embedding model is read from +the bundle path by `resolve_embedding_model()`, because a query embedded with a +different model than built the vectors returns nonsense rather than an error. + +```bash +poetry run pytest tests/agent/test_embedding_model_resolution.py -q +``` + +## 7. The gates + +```bash +poetry run ruff check . && poetry run mypy . && poetry run pytest +``` + +`tests/util/test_config.py` must pass **untouched**: adding a section must not +change what the loader does with an invalid file. diff --git a/specs/003-model-configuration/tasks.md b/specs/003-model-configuration/tasks.md new file mode 100644 index 0000000..f6a2836 --- /dev/null +++ b/specs/003-model-configuration/tasks.md @@ -0,0 +1,119 @@ +--- +description: "Task list for model configuration" +--- + +# Tasks: Model Configuration + +**Input**: Design documents from `/specs/003-model-configuration/` + +**Prerequisites**: [plan.md](./plan.md), [spec.md](./spec.md), [quickstart.md](./quickstart.md) + +**Tests**: Included. This repository's constitution (Article III) makes tests the +tripwire for behaviour, and Article IV's "fail loudly" requirement is only real if a +test asserts the failure. They are written with the code they cover, not after. + +**Branch**: `feat/model-configuration` + +## Phase 1: Setup + +- [ ] T001 Create branch `feat/model-configuration` from `origin/main` +- [ ] T002 Re-read #112 and #151 with `gh pr diff`, to credit them accurately in the commits that land their idea + +## Phase 2: Foundational + +**Blocking: every user story below depends on the config field existing.** + +- [ ] T003 Create `LLMConfig` (`provider: str = "openai"`, `model: str | None = None`, `base_url: str | None = None`, `temperature: float | None = None`) in `src/util/config_yml/models.py`, after the shape in #112 and crediting @AaryanCode69 +- [ ] T004 Add `llm: LLMConfig | None = None` to `Config` in `src/util/config_yml/__init__.py` — optional, so a config without it is unchanged (FR-002) +- [ ] T005 [P] Add the matching `llm` object to `.config.schema.yaml`, with **no** embedding field (FR-004) +- [ ] T006 [P] Document the section, commented out, in `config_default.yml` + +## Phase 3: User Story 1 — A deployment names its model beside its other settings (P1) + +**Goal**: the model comes from `config.yml`. + +**Independent test**: set a model in `config.yml`, start the server, ask a question, +confirm from the log which model answered. Quickstart steps 1–3. + +- [ ] T007 [US1] Add `resolve_llm_model(config)` to `src/agent/graph.py`: `LLM_MODEL` beats `config.llm.model` beats the current default, and document why the precedence is the reverse of `util/secrets.py` (both are "the more specific wins") +- [ ] T008 [US1] Wire `AgentGraph.__init__` to it, passing `base_url` and `provider` from the config when present, in `src/agent/graph.py` +- [ ] T009 [US1] Log the effective model at startup in `src/agent/graph.py` (FR-008) — the name only, never a key +- [ ] T010 [P] [US1] Test in `tests/agent/test_model_configuration.py`: no `llm` section behaves exactly as today (FR-002) +- [ ] T011 [P] [US1] Test in `tests/agent/test_model_configuration.py`: a configured model is the one selected +- [ ] T012 [P] [US1] Test in `tests/agent/test_model_configuration.py`: `LLM_MODEL` overrides `config.yml` (FR-003) +- [ ] T013 [US1] Run quickstart steps 1–3 against a real bundle and confirm the log names the expected model each time (constitution Article I) + +## Phase 4: User Story 2 — An unusable model stops the server, not the conversation (P1) + +**Goal**: a contradictory model/temperature pair is refused at startup. + +**Independent test**: `gpt-5.6-luna` with `temperature: 0` must refuse to start. +Quickstart steps 4–5. + +- [ ] T014 [US2] Extend `resolve_temperature` in `src/agent/graph.py` to accept a configured temperature and raise `SystemExit` naming model, value and fix when the model refuses it (FR-006) +- [ ] T015 [P] [US2] Test in `tests/agent/test_model_temperature.py`: luna + `temperature: 0` exits, and the message contains all three of model, value and remedy +- [ ] T016 [P] [US2] Test in `tests/agent/test_model_temperature.py`: a model absent from the table starts normally (FR-007) +- [ ] T017 [P] [US2] Test in `tests/agent/test_model_temperature.py`: `LLM_TEMPERATURE` still wins over the configured value +- [ ] T018 [US2] Perturbation check: delete the guard and confirm T015 fails — a test that cannot fail is not a tripwire (Article III) +- [ ] T019 [US2] Run quickstart steps 4–5 and confirm the server refuses to start rather than failing on the first question + +## Phase 5: User Story 3 — Surfaces choose their own model (P2) + +**Goal**: not built now; make sure Stage 1's shape does not preclude it. + +**Independent test**: none — this phase ships no behaviour. + +- [ ] T020 [US3] Confirm `LLMConfig` is nestable per surface without a schema break, and record in `specs/003-model-configuration/plan.md` what a second surface would add +- [ ] T021 [US3] Cross-reference spec 002's latency table (22.5s vs 41.2s per question) in the spec as the reason surfaces will want to differ + +## Phase 6: Polish & Cross-Cutting + +- [ ] T022 [P] Verify `grep -rn embedding .config.schema.yaml config_default.yml` finds no embedding model field (SC-004), and add a test asserting it +- [ ] T023 [P] Confirm `tests/util/test_config.py` passes **untouched** — adding a section must not change what an invalid config does (Article III) +- [ ] T024 Run `ruff check`, `ruff format --check`, `mypy`, `pytest` +- [ ] T025 Close #112 with credit to @AaryanCode69, stating plainly that the LLM half is harvested and the embedding half rejected because it bypasses `resolve_embedding_model()` and would silently break Plant Reactome +- [ ] T026 Close #151 with credit to @bhavyakeerthi3, noting the flat-string shape was reasonable but `base_url` has nowhere to live in it +- [ ] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended + +## Dependencies + +```text +Phase 1 (T001-T002) + | +Phase 2 (T003-T006) <- blocking; nothing below works without the field + | + +-- Phase 3 US1 (T007-T013) P1 + | | + +-- Phase 4 US2 (T014-T019) P1, needs US1's resolution to validate + | + +-- Phase 5 US3 (T020-T021) P2, documentation only + | +Phase 6 (T022-T027) +``` + +US2 depends on US1: there is nothing to validate until the model is configurable. +US3 depends on neither and ships no code. + +## Parallel Opportunities + +- **T005, T006** — different files, no shared state +- **T010, T011, T012** — three tests in one new file; write together, they share fixtures +- **T015, T016, T017** — same, in the existing temperature test file +- **T022, T023** — independent checks + +`T025` and `T026` are deliberately **not** parallel with the rest: close a +contributor's PR only once the thing that replaces it has actually landed and its +gates are green. + +## Implementation Strategy + +**MVP is Phase 2 + Phase 3 (US1).** That alone delivers FR-001 through FR-003 and +FR-008 — a deployment can name its model — and is independently shippable. + +**Phase 4 (US2) is the reason this is a specification.** It is P1 alongside US1 +rather than after it, because shipping US1 alone adds two new ways to configure a +model wrongly without adding any way to find out before a user does. + +Phase 5 ships nothing and could be dropped without loss; it exists so the nesting +decision in T003 is made deliberately rather than discovered later, which is the +mistake the context budget made in spec 001.