From 949c4d50ce7c2d67393e585d09ebe13581d99f6b Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 16:55:07 +0000 Subject: [PATCH] Plan and task out spec 003, model configuration Spec Kit has been half-used here: three specs written, one plan, and nothing ever carried through to tasks. This takes 003 the rest of the way -- plan.md, quickstart.md, tasks.md -- before writing any code. The plan settles three things the spec left implicit. Precedence is environment over file, which is the reverse of util/secrets.py, and the reason is worth stating rather than discovering: a mounted secret comes from the deployment and should beat a checked-in file, while LLM_MODEL is how one container overrides a committed config.yml. Both are "the more specific wins". Validation happens at startup and against the table alone (D1 as recommended): no API call, so it works offline and cannot make startup depend on OpenAI being reachable. And US2 is P1 alongside US1, not after it, because shipping "you can configure a model" without "you find out at startup when it is wrong" adds two new ways to be wrong and no new way to notice. quickstart.md is the validation guide: seven scenarios, each a thing that can be observed failing, including the one that matters most -- that a config with no llm section behaves exactly as it does today. No research.md, data-model.md or contracts/. The unknowns were settled by measurement in #189, the data model is three optional fields on an existing pydantic class, and the only external contract is config.yml, whose schema file is edited in Stage 1. Generating empty scaffolding would be ceremony, as it was for 001. Co-Authored-By: Claude Opus 5 --- specs/003-model-configuration/plan.md | 157 ++++++++++++++++++++ specs/003-model-configuration/quickstart.md | 90 +++++++++++ specs/003-model-configuration/tasks.md | 119 +++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 specs/003-model-configuration/plan.md create mode 100644 specs/003-model-configuration/quickstart.md create mode 100644 specs/003-model-configuration/tasks.md 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.