diff --git a/.config.schema.yaml b/.config.schema.yaml index d2f12057..cfca8537 100644 --- a/.config.schema.yaml +++ b/.config.schema.yaml @@ -24,7 +24,11 @@ properties: properties: provider: type: string - enum: ["openai", "ollama"] + # `openai` and `openai-responses` are the same endpoint through the two + # API shapes. Pick the one the endpoint publishes: a gateway that has no + # chat/completions operation returns its own 404 regardless of the model, + # and Azure's Codex deployments are Responses-only in any case. + enum: ["openai", "openai-responses", "ollama"] model: type: string description: "e.g. gpt-4o-mini, gpt-5.6-luna. LLM_MODEL overrides this." diff --git a/.dockerignore b/.dockerignore index 17c2f58e..fba5c101 100644 --- a/.dockerignore +++ b/.dockerignore @@ -3,6 +3,8 @@ embeddings/ embeddings_bak/ csv_files/ records/ +.dockerignore +sockets/ .venv/ .env .env.* diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c443ce34..e4ecfca0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,43 @@ jobs: - name: Run tests run: poetry run pytest + schema: + # initdb/init.sql is hand-written and Chainlit's data layer sends every + # non-None key of the dict it is handed, so a key added upstream becomes + # an INSERT into a column that does not exist here. That went unnoticed + # for 22 months and surfaced as UndefinedColumn on every user request. + # The guard has to run against a database actually built from init.sql, + # which is what this job is. + runs-on: ubuntu-latest + services: + postgres: + image: postgres:16 + env: + POSTGRES_PASSWORD: ci-only + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 5s + --health-timeout 5s + --health-retries 10 + steps: + - uses: actions/checkout@v4 + + - name: Set up Python and Poetry + uses: ./.github/actions/install_python_poetry + + - name: Create the databases from initdb/init.sql + run: | + psql postgresql://postgres:ci-only@localhost:5432/postgres \ + -v ON_ERROR_STOP=1 -f initdb/init.sql + + - name: Check the schema against the installed Chainlit + run: | + PYTHONPATH=./src poetry run python bin/check_schema.py \ + --dsn postgresql://postgres:ci-only@localhost:5432/chatbio_chainlit + + poetry-check: # No event filter. It used to run only on pull_request and # workflow_dispatch, so every push to main skipped it -- and GitHub counts diff --git a/.gitignore b/.gitignore index 0a018e87..026ea620 100644 --- a/.gitignore +++ b/.gitignore @@ -183,3 +183,4 @@ data/ # belongs in the repository. *.pem *.key +sockets/ diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index e70740fe..89a8a8dc 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -52,6 +52,7 @@ from util import captcha_cookie from util.chainlit_helpers import ( PrefixedS3StorageClient, + TextStreamCallbackHandler, is_feature_enabled, message_rate_limited, save_openai_metrics, @@ -89,6 +90,50 @@ f"{len(_mounted)} of {len(SECRET_NAMES)} secrets supplied as Docker secrets" ) + +def _sanitize_llm_settings() -> None: + """Stop non-JSON values reaching the persisted generation settings. + + `_build_llm_settings` is the one place in chainlit's GenerationHelper that + does not run its output through `ensure_values_serializable` -- `_start_trace` + does, for run.inputs. So anything unserializable goes straight into + SQLAlchemyDataLayer.create_step's json.dumps and the INSERT dies, which + deletes the llm step from thread history entirely. + + Ours is `response_format`: with_structured_output(method="json_schema") binds + the pydantic CLASS and serializes it only at request time. That method is + deliberate -- the gpt-5.6 family refuses function tools on + /v1/chat/completions -- so the class stays in the kwargs and is made safe + here instead. + + Patched at the tracer rather than in a data-layer subclass because the same + dict is also handed to context.emitter.update_step, so the socket sees it. + """ + from chainlit.langchain.callbacks import GenerationHelper + + original = GenerationHelper._build_llm_settings + + def patched( + self: Any, + serialized: dict[str, Any], + invocation_params: dict[str, Any] | None = None, + ) -> tuple[Any, ...]: + result: tuple[Any, ...] = original(self, serialized, invocation_params) + if len(result) != 4: + return result + provider, model, tools, settings = result + return ( + provider, + model, + tools, + self.ensure_values_serializable(settings) if settings else settings, + ) + + GenerationHelper._build_llm_settings = patched # type: ignore[method-assign] + + +_sanitize_llm_settings() + config: Config | None = Config.from_yaml() profiles: list[ProfileName] = config.profiles if config else [ProfileName.React_to_Me] @@ -552,7 +597,7 @@ async def answer_with_model(content: str, message_id: str) -> None: thread_id: str = current_thread_id() - chainlit_cb = cl.AsyncLangchainCallbackHandler( + chainlit_cb = TextStreamCallbackHandler( stream_final_answer=True, force_stream_final_answer=True, # we're not using prefix tokens ) diff --git a/bin/chat-fastapi.py b/bin/chat-fastapi.py index 78df4617..09979040 100644 --- a/bin/chat-fastapi.py +++ b/bin/chat-fastapi.py @@ -2,7 +2,6 @@ import time from collections.abc import AsyncIterator, Awaitable, Callable from contextlib import asynccontextmanager -from string import Template from typing import Any from urllib.parse import urlsplit @@ -29,9 +28,12 @@ from api.answer import router as answer_router from api.handoff import router as handoff_router from util import captcha_cookie +from util.branding import Branding, branding_for from util.caller_token import load_verifying_key from util.captcha_scope import is_captcha_exempt +from util.config_yml import Config from util.embedding_environment import EmbeddingEnvironment +from util.landing_page import render_error_page, render_landing_page from util.logging import logging @@ -115,23 +117,33 @@ async def lifespan(_app: FastAPI) -> AsyncIterator[None]: "so the human check could not be shown." ) -ERROR_PAGE_TEMPLATE = Template( - f""" - - -

$error_title

-

If you believe this to be in error, please contact the maintainers to report an issue: help@reactome.org

-
- -
- - -""" -) +# Which knowledgebase this deployment is. `profiles:` in config.yml already +# decides which graph answers questions and which chat profiles are offered, and +# config.yml is the one file a Plant Reactome deployment replaces (see +# compose.yaml), so the pages around the chat follow it rather than a second +# switch that could disagree with the first. No config.yml means +# config_default.yml, which names React-to-Me, so Reactome does nothing to keep +# what it has. +# +# Read once, at startup, like the release in `lifespan`. A page that changed brand +# because config.yml was edited under a running process -- while the graph kept +# answering from the old one -- would be worse than a stale one. +config: Config | None = Config.from_yaml() +BRANDING: Branding = branding_for(config.profiles if config else []) HEADER_DONT_CACHE = {"Cache-Control": "no-store"} +def _error_page(error_title: str) -> Response: + """What a failed human check ends on, in this deployment's brand.""" + return Response( + content=render_error_page(BRANDING, error_title, CHAINLIT_URI or "/"), + status_code=400, + headers=HEADER_DONT_CACHE, + media_type="text/html", + ) + + def _gated(path: str) -> bool: """Whether the human check applies to this path.""" return not is_captcha_exempt( @@ -271,21 +283,24 @@ async def captcha_page() -> Response: @app.post(f"{CHAINLIT_URI}/verify_captcha") async def verify_captcha(request: Request) -> Response: - # Bounded: anyone can reach this route, and the defaults put no cap on - # file parts, which were spooled to the disk the whole host shares - # (review, area 1b). The form has one field. + # Bounded: anyone can reach this route. max_files=0 refuses file parts + # outright -- they were spooled to the disk the whole host shares (review, + # area 1b) -- and max_part_size holds the one field, a Turnstile token a + # couple of hundred bytes long, to 8 KiB rather than Starlette's 1 MiB + # default. + # + # max_part_size needs Starlette >= 0.44, which added it to Request.form(). + # The published image shipped 0.41.3, and starlette arrives transitively via + # chainlit with nothing pinning the floor, so this raised + # `TypeError: Request.form() got an unexpected keyword argument + # 'max_part_size'` -- a 500 on the entry path, for every visitor, and only + # on the image. A transitive version is not a floor; the floor lives in + # pyproject now. Starlette 1.3.1 is where these limits are actually enforced + # in FormParser, which is the floor declared there. form_data = await request.form(max_files=0, max_fields=4, max_part_size=8192) cf_turnstile_response = form_data.get("cf-turnstile-response") if not isinstance(cf_turnstile_response, str): - error_html = ERROR_PAGE_TEMPLATE.substitute( - error_title="CAPTCHA response is invalid", - ) - return Response( - content=error_html, - status_code=400, - headers=HEADER_DONT_CACHE, - media_type="text/html", - ) + return _error_page("CAPTCHA response is invalid") client_ip: str if request.client: @@ -293,15 +308,7 @@ async def verify_captcha(request: Request) -> Response: elif "X-Forwarded-For" in request.headers: client_ip = request.headers["X-Forwarded-For"].split(",")[0] else: - error_html = ERROR_PAGE_TEMPLATE.substitute( - error_title="Could not determine client host", - ) - return Response( - content=error_html, - status_code=400, - headers=HEADER_DONT_CACHE, - media_type="text/html", - ) + return _error_page("Could not determine client host") # Verify the CAPTCHA with Cloudflare url = "https://challenges.cloudflare.com/turnstile/v0/siteverify" @@ -329,15 +336,7 @@ async def verify_captcha(request: Request) -> Response: # If CAPTCHA validation fails, return an error if not result.get("success"): - error_html = ERROR_PAGE_TEMPLATE.substitute( - error_title="CAPTCHA verification failed", - ) - return Response( - content=error_html, - status_code=400, - headers=HEADER_DONT_CACHE, - media_type="text/html", - ) + return _error_page("CAPTCHA verification failed") redirect_response = RedirectResponse( url=f"{CHAINLIT_URI}/", status_code=302, headers=HEADER_DONT_CACHE @@ -351,116 +350,19 @@ async def verify_captcha(request: Request) -> Response: @app.get("/chat/") async def landing_page() -> HTMLResponse: - html_content = Template( - """ - - - - - - -
- -

Meet the React-to-Me AI Chatbot!

-

Your new guide to Reactome. Whether you're looking for specific genes and pathways or just browsing, our AI Chatbot is here to assist you!

- -
- Guest Access - Log In - -
- -

Choose Guest Access to try the chatbot out. Log In will give an increased query allowance and securely stores your chat history so you can save and continue conversations.

-

We encourage you to use the Feedback button to tell us about your experience with the chatbot and help us improve it.

-
- - - """ - ).substitute(CHAINLIT_URL=CHAINLIT_URL) + """The page that introduces the chat, in this deployment's brand. - return HTMLResponse(content=html_content) + Rendered in `util/landing_page.py`, where a test can render it without + importing this module -- which mounts Chainlit and refuses to start without + Turnstile keys. Only the branding varies; the layout is shared, so a Plant + Reactome reader gets the same page with their knowledgebase's name, logo and + contact on it. + """ + # Empty rather than the literal "None" this interpolated when CHAINLIT_URL is + # unset, which made every button read "None/chat/guest/". With nothing in + # front of them they resolve against this origin, which is what an unset + # CHAINLIT_URL should always have meant. + return HTMLResponse(content=render_landing_page(BRANDING, CHAINLIT_URL or "")) # Ensure all other endpoints remain mounted diff --git a/bin/check_schema.py b/bin/check_schema.py new file mode 100755 index 00000000..faf39bbf --- /dev/null +++ b/bin/check_schema.py @@ -0,0 +1,241 @@ +#!/usr/bin/env python +"""Fail when the Chainlit schema has drifted from the installed Chainlit. + +initdb/init.sql is the only thing standing between a Chainlit upgrade and +`psycopg.errors.UndefinedColumn` on every read and write. It drifted for 22 +months once and nothing complained, because the data layer builds its INSERT +column list from every non-None key of the dict it is handed -- there is no +whitelist -- so a key added upstream becomes a column this repository does not +have, and the first anyone hears of it is a user-visible failure. + +So the invariant is one-directional: **each table must be a superset of the +shape written into it.** Extra columns are harmless. Missing ones are the bug. + +Three modes, because two different gaps need covering: + + PYTHONPATH=./src .venv/bin/python bin/check_schema.py --sql initdb/init.sql + Parse the file. Needs no database, so it runs on a developer's machine + and from checks.sh. Catches init.sql missing a column. + + PYTHONPATH=./src .venv/bin/python bin/check_schema.py + Query a live database. Uses the app's own path -- util.secrets.get_db_uri + -- so the unix socket and any Vault credential are honoured. That socket + exists only inside the container, so on the host this exits 2. + + PYTHONPATH=./src .venv/bin/python bin/check_schema.py \ + --dsn postgresql://user:pw@localhost:5432/chatbio_chainlit + The above against an explicit DSN. This is what CI runs, against a + database actually built from initdb/init.sql, which is the only way to + also catch a live database that was never migrated. + +The venv is not optional: psycopg lives in it, and the shebang resolves `python` +through PATH. Inside the container that is already the venv, which is why the +short form works there and nowhere else. + +Exit codes: 0 matches, 1 drift, 2 could not look. Keep them distinct -- a +connection problem must not read as a pass, which is the same reason checks.sh +exists at all. +""" + +import argparse +import os +import re +import sys +from dataclasses import fields, is_dataclass +from importlib.metadata import version +from pathlib import Path +from typing import Any + +import psycopg + +from util.secrets import get_db_uri + + +def _shape_keys(shape: Any) -> set[str]: + """Every field name of a TypedDict, pydantic model or dataclass. + + All three appear here: StepDict and ElementDict are TypedDicts, Feedback is + a pydantic model. Resolving them from the installed package rather than from + a list in this file is the point -- a literal list would drift exactly the + way init.sql did. + """ + model_fields = getattr(shape, "model_fields", None) + if model_fields: + return set(model_fields) + if is_dataclass(shape): + return {f.name for f in fields(shape)} + keys: set[str] = set() + for base in reversed(shape.__mro__): + keys.update(getattr(base, "__annotations__", {})) + return keys + + +def _expected() -> dict[str, tuple[set[str], dict[str, str]]]: + """table -> (fields written into it, fields never written, and why).""" + from chainlit.element import ElementDict + from chainlit.step import StepDict + from chainlit.types import Feedback, ThreadDict + + return { + "steps": ( + _shape_keys(StepDict), + {"feedback": "read path only: Step.to_dict() never emits it"}, + ), + "elements": (_shape_keys(ElementDict), {}), + "threads": ( + _shape_keys(ThreadDict), + { + "steps": "nested read model, not a column", + "elements": "nested read model, not a column", + }, + ), + # upsert_feedback serialises asdict(Feedback). FeedbackDict is what the + # read path constructs by hand and is a different shape -- checking that + # one instead would let the real write shape drift unchecked. + "feedbacks": (_shape_keys(Feedback), {}), + # create_user is a literal INSERT, so the shape is that literal. UserDict + # carries display_name, which is never written; checking it here would + # report a missing column that must never exist. + "users": ({"id", "identifier", "createdAt", "metadata"}, {}), + } + + +def _columns_from_sql(text: str) -> dict[str, set[str]]: + """Column names per table, read out of initdb/init.sql. + + A text parse rather than a live query, so the check runs where the database + cannot be reached -- which is every developer's machine, since Postgres is + unix-socket-only and /sockets/postgres/ exists only inside the container. + It is a regex over a file this repository writes, in one fixed layout. + + It does NOT catch a live database that was never migrated from an older + init.sql. The CI job, which builds a real Postgres from the same file, is + what catches that. + """ + tables: dict[str, set[str]] = {} + pattern = r'CREATE TABLE (?:IF NOT EXISTS )?"?(\w+)"?\s*\((.*?)\n\)' + for match in re.finditer(pattern, text, re.DOTALL): + body = match.group(2) + # A column line opens with a quoted name. FOREIGN KEY and CONSTRAINT + # lines open with a keyword, so the anchor excludes them without needing + # a list of keywords to skip. + tables[match.group(1)] = set( + re.findall(r'^\s*"([A-Za-z_]\w*)"', body, re.MULTILINE) + ) + return tables + + +def _columns(conn: psycopg.Connection, table: str) -> set[str]: + sql = """ + SELECT column_name + FROM information_schema.columns + WHERE table_name = %s AND table_schema = current_schema() + """ + with conn.cursor() as cur: + cur.execute(sql, (table,)) + return {row[0] for row in cur.fetchall()} + + +def _compare(have: dict[str, set[str]]) -> int: + """The one verdict both modes reach through.""" + drift: list[str] = [] + for table, (keys, excludes) in _expected().items(): + columns = have.get(table, set()) + if not columns: + drift.append(f"{table}: table does not exist") + continue + wanted = keys - set(excludes) + missing = wanted - columns + if missing: + drift.append(f'{table}: missing {", ".join(sorted(missing))}') + unused = columns - wanted + if unused: + print(f' note: {table} has unused {", ".join(sorted(unused))}') + + if drift: + print("Chainlit schema drift:", file=sys.stderr) + for line in drift: + print(f" {line}", file=sys.stderr) + print( + "\nThe data layer sends every non-None key, so a column missing " + "here fails the INSERT that uses it.", + file=sys.stderr, + ) + return 1 + + print("schema matches the installed Chainlit") + return 0 + + +def _print_expected() -> int: + """What each table must hold, for writing initdb/init.sql. + + Names only. The SQL type is not inferred: showInput is TEXT in the schema + although it is typed bool | str, and a guessed type that is wrong is worse + than no type. + """ + for table, (keys, excludes) in _expected().items(): + wanted = keys - set(excludes) + print(f"-- {table}: {len(wanted)} columns") + for name in sorted(wanted): + print(f' "{name}" ...') + return 0 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Check the Chainlit schema against the installed Chainlit." + ) + parser.add_argument( + "--dsn", + default=None, + help="Defaults to get_db_uri(POSTGRES_CHAINLIT_DB).", + ) + parser.add_argument( + "--sql", + default=None, + help="check the text of initdb/init.sql; needs no database", + ) + parser.add_argument( + "--print-expected", + action="store_true", + help="print the required columns and exit; needs no database", + ) + args = parser.parse_args(argv) + + print(f"chainlit {version('chainlit')}") + + if args.print_expected: + return _print_expected() + + if args.sql: + text = Path(args.sql).read_text(encoding="utf-8") + return _compare(_columns_from_sql(text)) + + dsn = args.dsn or get_db_uri(os.getenv("POSTGRES_CHAINLIT_DB")) + if not dsn: + print( + "no DSN: pass --dsn or --sql, or set POSTGRES_CHAINLIT_DB and run " + "where the Postgres socket exists", + file=sys.stderr, + ) + return 2 + + live: dict[str, set[str]] = {} + conn: psycopg.Connection | None = None + try: + conn = psycopg.connect(dsn) + for table in _expected(): + live[table] = _columns(conn, table) + except psycopg.Error as exc: + print(f"cannot read the schema: {exc}", file=sys.stderr) + return 2 + finally: + if conn is not None: + conn.close() + + return _compare(live) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/checks.sh b/checks.sh index 696eb088..2e475187 100755 --- a/checks.sh +++ b/checks.sh @@ -34,6 +34,7 @@ step "ruff check" "$PY" -m ruff check . step "ruff format" "$PY" -m ruff format --check . step "mypy" "$PY" -m mypy step "pytest" "$PY" -m pytest +step "schema" env PYTHONPATH=./src "$PY" bin/check_schema.py --sql initdb/init.sql rm -f /tmp/checks-$$.log failed=0 diff --git a/compose.yaml b/compose.yaml index 0329d8ff..738d49f3 100644 --- a/compose.yaml +++ b/compose.yaml @@ -40,6 +40,10 @@ services: - OPENAI_API_KEY - TAVILY_API_KEY volumes: + # `profiles:` in this file is also what decides which brand the landing + # page and the captcha error pages wear (src/util/branding.py). Reactome's + # is the default in code, so a deployment that supplies no config.yml at + # all -- or one naming React-to-Me -- serves exactly what it serves today. - ./config.yml:/app/config.yml:ro # Welcome screen. The image ships the Reactome one at /app/chainlit.md; # the Plant Reactome deployment mounts its variant over it, the same way diff --git a/env_template b/env_template index f98d6809..11e1a0fb 100644 --- a/env_template +++ b/env_template @@ -35,6 +35,13 @@ TAVILY_API_KEY= # about; see resolve_temperature in src/agent/graph.py. #LLM_TEMPERATURE= +# Header the LLM key is sent in. Azure API Management reads its subscription key +# from `api-key` and answers the OpenAI SDK's default `Authorization: Bearer` +# with 401 "Access denied due to missing subscription key", so an APIM-fronted +# endpoint needs this set to `api-key`. Leave unset for api.openai.com and other +# OpenAI-compatible servers, which want only the bearer token. +#LLM_API_KEY_HEADER=api-key + # Locale and timezone for the Postgres container's initdb (ICU collation). LOCALE=en_US TIMEZONE=America/Toronto diff --git a/initdb/init.sql b/initdb/init.sql index 78ca3759..76249416 100644 --- a/initdb/init.sql +++ b/initdb/init.sql @@ -1,9 +1,19 @@ +-- Chainlit 2.11.0 data-layer schema. +-- Derived from the TypedDicts in the installed package (chainlit.step.StepDict, +-- chainlit.element.ElementDict, chainlit.types.ThreadDict/FeedbackDict, +-- chainlit.user.UserDict) -- NOT from docs.chainlit.io, which is missing +-- autoCollapse, icon, path, autoPlay and playerConfig. +-- The data layer builds INSERT column lists from every non-None key, so each +-- table must be a superset of its TypedDict. Run bin/check_schema.py after any +-- chainlit upgrade. + CREATE DATABASE chatbio_chainlit; CREATE DATABASE chatbio_langgraph; CREATE DATABASE chatbio_langgraph_no_login; \c chatbio_chainlit +-- Literal INSERT (sql_alchemy.py:183): fixed columns, no drift risk. CREATE TABLE users ( "id" UUID PRIMARY KEY, "identifier" TEXT NOT NULL UNIQUE, @@ -28,6 +38,8 @@ CREATE TABLE IF NOT EXISTS steps ( "type" TEXT NOT NULL, "threadId" UUID NOT NULL, "parentId" UUID, + "command" TEXT, + "modes" JSONB, "streaming" BOOLEAN NOT NULL, "waitForAnswer" BOOLEAN, "isError" BOOLEAN, @@ -40,8 +52,11 @@ CREATE TABLE IF NOT EXISTS steps ( "end" TEXT, "generation" JSONB, "showInput" TEXT, + "defaultOpen" BOOLEAN, + "autoCollapse" BOOLEAN, "language" TEXT, - "indent" INT + "icon" TEXT, + FOREIGN KEY ("threadId") REFERENCES threads("id") ON DELETE CASCADE ); CREATE TABLE IF NOT EXISTS elements ( @@ -57,13 +72,21 @@ CREATE TABLE IF NOT EXISTS elements ( "page" INT, "language" TEXT, "forId" UUID, - "mime" TEXT + "mime" TEXT, + "props" JSONB, + "path" TEXT, + "autoPlay" BOOLEAN, + "playerConfig" JSONB, + FOREIGN KEY ("threadId") REFERENCES threads("id") ON DELETE CASCADE ); +-- "threadId" is nullable on purpose: FeedbackDict has no threadId key, so a +-- NOT NULL constraint here would reject feedback built from that dict. CREATE TABLE IF NOT EXISTS feedbacks ( "id" UUID PRIMARY KEY, "forId" UUID NOT NULL, - "threadId" UUID NOT NULL, + "threadId" UUID, "value" INT NOT NULL, - "comment" TEXT + "comment" TEXT, + FOREIGN KEY ("threadId") REFERENCES threads("id") ON DELETE CASCADE ); diff --git a/poetry.lock b/poetry.lock index 4e4f24cc..3a942d51 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 2.4.3 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.4.1 and should not be changed by hand. [[package]] name = "aiofiles" @@ -9176,4 +9176,4 @@ cffi = ["cffi (>=1.17,<2.0) ; platform_python_implementation != \"PyPy\" and pyt [metadata] lock-version = "2.1" python-versions = ">=3.12, <4" -content-hash = "f78f40deab1059d18ed2483bb95c5bc4b61369f9f79534419de4d7a293f7a386" +content-hash = "4abf7f631f3655972df44f35ea084f362122abae5b71046422e43c2134912dcb" diff --git a/pyproject.toml b/pyproject.toml index 0baad421..3c2c6816 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,14 @@ cryptography = ">=42,<51" langchain-classic = "^1.0.8" # Same: data_generation imports RecursiveCharacterTextSplitter from it. langchain-text-splitters = "^1.1.2" +# Declared, not inherited, for the reason the two above are. bin/chat-fastapi.py +# passes max_part_size to Request.form(), a parameter Starlette added in 0.44.0 +# and only actually enforced in FormParser from 1.3.1. Starlette reached this +# project through chainlit with no floor stated anywhere, and the published +# image carried 0.41.3 -- below the floor -- so every visitor to the chat got a +# 500 TypeError on the captcha POST, on the image only, with nothing in the +# dependency set to explain it. A transitive version is not a floor. +starlette = ">=1.3.1" # 2.x, required by langchain-openai 1.x (openai>=2.45). Used directly only by # bin/probe_model_temperature, for models.list(). openai = "^2.45.0" @@ -202,11 +210,24 @@ docstring-code-format = true [tool.pytest.ini_options] testpaths = ["tests"] pythonpath = ["src", "bin"] -addopts = "-q --strict-markers --strict-config" +addopts = [ + "-q", + "--strict-markers", + "--strict-config", + # Deselect the live-model tests unless asked for. Without this, bare + # `pytest` -- which is what CI's `pytest` step and ./checks.sh both run -- + # spent ~160s and real money per invocation on a developer's machine, + # where OPENAI_API_KEY happens to be set. CI has no key so it skipped + # them by luck rather than by design; this makes the default explicit. + # Opt back in with `pytest -m requires_live_model` -- a CLI -m overrides + # this one because addopts are inserted ahead of the command line. + "-m", + "not requires_live_model", +] markers = [ "requires_embeddings: needs an installed embeddings bundle (see ./bin/embeddings_manager)", "requires_retrieval_stack: needs the full langchain/chromadb dependency set installed", - "requires_live_model: calls a real LLM and needs OPENAI_API_KEY plus embeddings", + "requires_live_model: calls a real LLM; needs LLM_MODEL and LLM_API_KEY (the model the app actually answers with) plus OPENAI_API_KEY for embeddings", ] [tool.mypy] diff --git a/src/agent/graph.py b/src/agent/graph.py index 28fcaa1f..87422b5c 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -23,6 +23,7 @@ from agent.profiles import create_profile_graphs from agent.profiles.base import InputState, OutputState from reactome_mcp.session import get_mcp_tools +from util.answer_text import answer_text from util.config_yml.models import LLMConfig from util.embedding_environment import EmbeddingEnvironment from util.logging import logging @@ -315,16 +316,26 @@ def __init__( logging.info( f"Answering with {llm_provider}/{llm_model} at temperature {temperature}" ) + llm_api_key = os.getenv("LLM_API_KEY") + embedding_api_key = os.getenv("OPENAI_API_KEY") + # The header the key travels in. Azure API Management only reads + # `api-key` and 401s the SDK's `Authorization: Bearer`; see get_llm. + llm_api_key_header = os.getenv("LLM_API_KEY_HEADER") llm: BaseChatModel = get_llm( llm_provider, llm_model, base_url=llm_base_url, request_timeout=360.0, temperature=temperature, + api_key=llm_api_key, + api_key_header=llm_api_key_header, ) embedding_base_url = os.getenv("OPENAI_BASE_URL", None) embedding: Embeddings = get_embedding( - "openai", embedding_model, base_url=embedding_base_url + "openai", + embedding_model, + base_url=embedding_base_url, + api_key=embedding_api_key, ) self.uncompiled_graph: dict[str, StateGraph] = create_profile_graphs( @@ -582,10 +593,10 @@ async def astream_answer( if event.get("metadata", {}).get("langgraph_node") != ANSWER_NODE: continue - text = getattr(event["data"].get("chunk"), "content", "") + text = answer_text(event["data"].get("chunk")) if text: answered = True - yield AnswerEvent(kind="token", text=str(text)) + yield AnswerEvent(kind="token", text=text) yield AnswerEvent( kind="done", state="answered" if answered else "nothing_found" diff --git a/src/agent/models.py b/src/agent/models.py index d524c868..e16552ee 100644 --- a/src/agent/models.py +++ b/src/agent/models.py @@ -6,6 +6,7 @@ from langchain_ollama.chat_models import ChatOllama from langchain_openai.chat_models.base import ChatOpenAI from langchain_openai.embeddings import OpenAIEmbeddings +from pydantic import SecretStr #: Seconds for one embedding request. EMBEDDING_TIMEOUT_SECONDS = 30.0 @@ -24,6 +25,7 @@ def get_embedding( *, device: str | None = "cpu", base_url: str | None = None, + api_key: str | None = None, ) -> Embeddings: if model is None: provider, model = provider.split("/", 1) @@ -33,7 +35,13 @@ def get_embedding( # question, which no cancellation could free, until every to_thread # in the process queued behind them (review, area 3). return OpenAIEmbeddings( - model=model, base_url=base_url, timeout=EMBEDDING_TIMEOUT_SECONDS + model=model, + base_url=base_url, + timeout=EMBEDDING_TIMEOUT_SECONDS, + # SecretStr, not str: langchain-openai 1.x types api_key as SecretStr, + # and the wrapper also keeps the key out of repr(), so a future + # logging of the constructed client cannot write a key to the log. + api_key=SecretStr(api_key) if api_key else None, ) if provider == "huggingfacehub": return HuggingFaceEndpointEmbeddings(model=model) @@ -50,6 +58,7 @@ def get_llm( provider: ( Literal[ "openai", + "openai-responses", "ollama", ] | str @@ -59,6 +68,8 @@ def get_llm( base_url: str | None = None, request_timeout: float | None = None, temperature: float = 0.0, + api_key: str | None = None, + api_key_header: str | None = None, ) -> BaseChatModel: """Build a chat model. See `agent.graph.resolve_temperature` for the value. @@ -67,19 +78,46 @@ def get_llm( Some models refuse it, which is why this is a parameter rather than the constant it used to be. - There is no way to send *no* temperature on langchain-openai 0.2.14: - omitting the argument makes ChatOpenAI send its own pydantic default of 0.7, - which the models that refuse 0.0 refuse just as firmly. So the caller must - pass a value the model accepts; it cannot opt out. + There is no way to send *no* temperature on ChatOpenAI: omitting the + argument makes it send its own pydantic default of 0.7, which the models + that refuse 0.0 refuse just as firmly. So the caller must pass a value the + model accepts; it cannot opt out. + + `openai` and `openai-responses` are the same endpoint reached through the + two API shapes -- Chat Completions and Responses. Which one works is decided + by the endpoint, not by the model: an Azure APIM gateway that publishes only + `POST /openai/responses` answers `POST /openai/chat/completions` with its own + `{"statusCode": 404, "message": "Resource not found"}` before the deployment + name in the body is ever read, so no value of `LLM_MODEL` can fix it. The + Codex family compounds that -- it is Responses-only by design -- but the + gateway is the binding constraint, and it is the one that is not visible in + any error the model would have produced. + + `use_responses_api` needs langchain-openai >= 0.3.9. It is passed as None + rather than False for the plain provider so that the library's own + auto-routing (a Responses-only feature forces that API) still applies. """ if model is None: provider, model = provider.split("/", 1) - if provider == "openai": + if provider in ("openai", "openai-responses"): return ChatOpenAI( model=model, temperature=temperature, base_url=base_url, request_timeout=request_timeout, # type: ignore[call-arg] # pydantic-generated __init__ + api_key=SecretStr(api_key) if api_key else None, + use_responses_api=True if provider == "openai-responses" else None, + # Azure API Management reads its subscription key from an `api-key` + # header and answers the OpenAI SDK's default `Authorization: Bearer` + # with 401 "Access denied due to missing subscription key" -- so the + # key has to travel in the header the gateway looks at, not only the + # one the SDK prefers. Measured against the endpoint: api-key 200, + # bearer 401, both 200. An extra header is ignored by the + # OpenAI-compatible servers that need only the bearer token, so this + # stays off unless LLM_API_KEY_HEADER names it. + default_headers=( + {api_key_header: api_key} if api_key and api_key_header else None + ), ) if provider == "ollama": return ChatOllama( diff --git a/src/agent/tasks/cross_database/rewrite_reactome_with_uniprot.py b/src/agent/tasks/cross_database/rewrite_reactome_with_uniprot.py index ee91dad6..da62a655 100644 --- a/src/agent/tasks/cross_database/rewrite_reactome_with_uniprot.py +++ b/src/agent/tasks/cross_database/rewrite_reactome_with_uniprot.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + reactome_rewriter_message = """ You are a query optimization expert with deep knowledge of molecular biology and extensive experience curating the Reactome Pathway Knowledgebase. You are also skilled in leveraging UniProt data to enhance search precision. Your task is to generate a new, optimized search question that incorporates relevant UniProt-derived context to improve search results within Reactome. @@ -42,6 +43,6 @@ def create_reactome_rewriter_w_uniprot(llm: BaseChatModel) -> Runnable: - return (reactome_rewriter_prompt | llm | StrOutputParser()).with_config( + return (reactome_rewriter_prompt | llm | AnswerTextParser()).with_config( run_name="rewrite_reactome_query" ) diff --git a/src/agent/tasks/cross_database/rewrite_uniprot_with_reactome.py b/src/agent/tasks/cross_database/rewrite_uniprot_with_reactome.py index 5e5d65f3..c0b3f885 100644 --- a/src/agent/tasks/cross_database/rewrite_uniprot_with_reactome.py +++ b/src/agent/tasks/cross_database/rewrite_uniprot_with_reactome.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + uniprot_rewriter_message = """ You are a query optimization expert with deep knowledge of molecular biology and extensive experience curating the UniProt Knowledgebase. You are also skilled in leveraging Reactome Pathway Knowledgebase data to enhance search precision. Your task is to reformulate user questions to maximize retrieval efficiency within UniProt’s Knowledgebase, which contains comprehensive information on human genes, proteins, protein domains/motifs, and protein functions. @@ -43,6 +44,6 @@ def create_uniprot_rewriter_w_reactome(llm: BaseChatModel) -> Runnable: - return (uniprot_rewriter_prompt | llm | StrOutputParser()).with_config( + return (uniprot_rewriter_prompt | llm | AnswerTextParser()).with_config( run_name="rewrite_uniprot_query" ) diff --git a/src/agent/tasks/cross_database/summarize_reactome_uniprot.py b/src/agent/tasks/cross_database/summarize_reactome_uniprot.py index 37ead803..538f0c63 100644 --- a/src/agent/tasks/cross_database/summarize_reactome_uniprot.py +++ b/src/agent/tasks/cross_database/summarize_reactome_uniprot.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + summarization_message = """ You are an expert in molecular biology with significant experience as a curator for the UniProt Database and the Reactome Pathway Knowledgebase. Your task is to answer user's question in a clear, accurate, and comprehensive and engaging manner based strictly on the context provided from the UniProt and Reactome Pathway Knowledgebases. @@ -49,6 +50,6 @@ def create_reactome_uniprot_summarizer( ) -> Runnable: if streaming: llm = llm.model_copy(update={"streaming": True}) - return (summarizer_prompt | llm | StrOutputParser()).with_config( + return (summarizer_prompt | llm | AnswerTextParser()).with_config( run_name="summarize_answer" ) diff --git a/src/agent/tasks/detect_language.py b/src/agent/tasks/detect_language.py index 0c1eba2d..2b795913 100644 --- a/src/agent/tasks/detect_language.py +++ b/src/agent/tasks/detect_language.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + language_detection_message = """ You are an expert linguist capable of identifying languages from text input. Your task is to determine the language of the user's question and return it as a single-word response. @@ -24,5 +25,5 @@ def create_language_detector(llm: BaseChatModel) -> Runnable: return ( language_detection_prompt | llm - | StrOutputParser().with_config(run_name="detect_language") + | AnswerTextParser().with_config(run_name="detect_language") ) diff --git a/src/agent/tasks/rephrase.py b/src/agent/tasks/rephrase.py index 23aaeaf5..6c4df713 100644 --- a/src/agent/tasks/rephrase.py +++ b/src/agent/tasks/rephrase.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate, MessagesPlaceholder from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + contextualize_q_system_prompt = """ You are an expert in question formulation with deep expertise in molecular biology and experience as a Reactome curator. Your task is to analyze the conversation history and the user's latest query to fully understand their intent and what they seek to learn. If the user's question is not in English, reformulate the question and translate it to English, ensuring the meaning and intent are preserved. @@ -28,6 +29,6 @@ def create_rephrase_chain(llm: BaseChatModel) -> Runnable: - return (contextualize_q_prompt | llm | StrOutputParser()).with_config( + return (contextualize_q_prompt | llm | AnswerTextParser()).with_config( run_name="rephrase_question" ) diff --git a/src/agent/tasks/unsafe_question.py b/src/agent/tasks/unsafe_question.py index 38e65d14..21eb1a4e 100644 --- a/src/agent/tasks/unsafe_question.py +++ b/src/agent/tasks/unsafe_question.py @@ -1,8 +1,9 @@ from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.output_parsers import StrOutputParser from langchain_core.prompts import ChatPromptTemplate from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + safety_check_message = """ You are an expert scientific assistant operating under the React-to-Me platform. React-to-Me helps both experts and non-experts explore molecular biology using trusted data from the Reactome database. @@ -40,6 +41,6 @@ def create_unsafe_answer_generator( ) -> Runnable: if streaming: llm = llm.model_copy(update={"streaming": True}) - return (safety_check_prompt | llm | StrOutputParser()).with_config( + return (safety_check_prompt | llm | AnswerTextParser()).with_config( run_name="unsafe_answer_generator" ) diff --git a/src/retrievers/rag_chain.py b/src/retrievers/rag_chain.py index 091e6864..25c8cc95 100644 --- a/src/retrievers/rag_chain.py +++ b/src/retrievers/rag_chain.py @@ -9,6 +9,8 @@ from langchain_core.retrievers import BaseRetriever from langchain_core.runnables import Runnable +from util.answer_text import AnswerTextParser + def create_rag_chain( llm: BaseChatModel, @@ -22,6 +24,10 @@ def create_rag_chain( llm=llm, prompt=qa_prompt, document_prompt=document_prompt, + # Without this the chain hands back whatever `content` is, and under the + # Responses API that is a list of blocks. `state["answer"]` then carries + # reasoning items into the chat UI, the chat history and the checkpoint. + output_parser=AnswerTextParser(), ) # Create the retrieval chain diff --git a/src/util/answer_text.py b/src/util/answer_text.py new file mode 100644 index 00000000..d8d39ed2 --- /dev/null +++ b/src/util/answer_text.py @@ -0,0 +1,85 @@ +"""Reduce a chat model's output to the text the reader should see. + +Chat Completions returns `content` as a plain string. The Responses API returns +a list of blocks -- `{'type': 'output_text', 'text': ...}` interleaved with +`{'type': 'reasoning', 'encrypted_content': ...}`. Anything that treats `content` +as a string then renders a Python repr of base64 reasoning into the answer, and +that repr is what reaches the chat, the checkpoint and the `steps` row. + +Lives here rather than in agent.graph because both the streaming boundary and the +RAG chain need it, and only one of them can filter by node. +""" + +from typing import Any, override + +from langchain_core.output_parsers.transform import BaseTransformOutputParser +from langchain_core.outputs import ChatGeneration, Generation + +#: Block types that carry answer prose. `output_text` is what the raw Responses +#: API emits; `text` is what langchain normalises it to. Both are needed: an +#: endpoint that passes Responses-API items through unconverted uses the former, +#: and `BaseMessage.text` -- which knows only about `text` -- returns "" for it. +_TEXT_TYPES = ("text", "output_text") + + +def answer_text(value: Any) -> str: + """The answer text in `value`, which may be a chunk, a message, or content. + + Accepts anything with a `content` attribute as well as raw content, so the + streaming loop and the output parser share one implementation. A bare dict -- + which is what a chunk's content actually is here -- is handled; a list-only + implementation would drop real answer text. + """ + content = getattr(value, "content", value) + if isinstance(content, str): + return content + blocks = content if isinstance(content, list) else [content] + parts: list[str] = [] + for block in blocks: + if isinstance(block, str): + parts.append(block) + elif isinstance(block, dict) and block.get("type") in _TEXT_TYPES: + parts.append(block.get("text", "")) + return "".join(parts) + + +class AnswerTextParser(BaseTransformOutputParser[str]): + """`StrOutputParser`, except it survives a Responses-API response. + + Overriding `parse` alone is useless, and the reason is worth recording: + + `invoke()` and `transform()` do not call `parse` with the message. Both call + `parse_result([ChatGeneration(message=...)])`, and `ChatGeneration` flattens + the content in a model validator -- `text = message.text` -- before anything + of ours sees it. `BaseMessage.text` keeps only blocks with `type == "text"`, + so a `parse` override is handed an already-collapsed string and can never + inspect a block. For `output_text` blocks that collapsed string is empty: + the answer is silently blank rather than wrong, which is the worse failure. + + So the work happens in `parse_result`, which is the one choke point every + path -- `invoke`, `ainvoke`, `transform`, `atransform` -- goes through, and + which still has the message. + + Extends the *transform* base rather than `BaseOutputParser` because every + call site this replaced used `StrOutputParser`, which does. Plain + `BaseOutputParser` still streams -- `Runnable.transform` falls back to + buffering the input and calling `ainvoke` -- but it streams as one chunk, + which would quietly take token-at-a-time output away from the answer node. + """ + + @override + def parse(self, text: str) -> str: + return answer_text(text) + + @override + def parse_result(self, result: list[Generation], *, partial: bool = False) -> str: + first = result[0] + # `first.text` is already flattened by this point, so a chat generation + # has to be read through its message or the blocks are unrecoverable. + if isinstance(first, ChatGeneration): + return answer_text(first.message.content) + return answer_text(first.text) + + @property + def _type(self) -> str: + return "answer_text" diff --git a/src/util/branding.py b/src/util/branding.py new file mode 100644 index 00000000..00a14019 --- /dev/null +++ b/src/util/branding.py @@ -0,0 +1,116 @@ +"""Which knowledgebase a deployment is, and how it should introduce itself. + +The landing page and the pages a failed human check ends on had Reactome's name, +Reactome's logo and Reactome's support address written into them. The Plant +Reactome deployment is the same image with its own `config.yml` bind-mounted over +the default (see `compose.yaml`), so it was introducing Plant Reactome's readers +to a human knowledgebase -- the same defect `chainlit.plantreactome.md` exists to +fix on the welcome screen, and `tests/test_welcome_pages.py` records how that one +used to be backwards. + +The switch is `profiles:` in config.yml. It already decides which graph answers +and which chat profiles are offered, so reusing it means there is one place where +a deployment says who it is; a second switch would be a second thing that can +disagree with the first. + +Reactome's branding is the default *in code*, not something its deployment has to +ask for: a checkout with no config.yml falls back to config_default.yml, which +names React-to-Me, and an empty profile list lands here too. Reactome changes +nothing to keep what it has. +""" + +from collections.abc import Sequence +from dataclasses import dataclass + +from agent.profile_names import ProfileName +from util.logging import logging + + +@dataclass(frozen=True) +class Branding: + """What one knowledgebase calls itself on the pages we serve.""" + + #: In prose: "Your new guide to Reactome" / "... to Plant Reactome". + knowledgebase: str + #: Used in the page title. Reactome's chatbot has its own name; the Plant + #: Reactome one is named for the knowledgebase, which is also the name of + #: its chat profile and of the `{chat_profile}` in its welcome message. + chatbot_name: str + #: The knowledgebase's own site, which the logo links to. + home_url: str + logo_url: str + favicon_url: str + #: The Feedback button's target. A mailto for Reactome; Plant Reactome + #: publishes no address, only a form (their site's Contact Us page). + feedback_url: str + #: Shown as text, not as a link, on the error pages -- so it has to read + #: well on its own. Plant Reactome's is Gramene's published contact + #: address, since Plant Reactome is a Gramene database. Worth confirming + #: with them before this page goes public. + support_email: str + + +REACTOME = Branding( + knowledgebase="Reactome", + chatbot_name="React-to-Me", + home_url="https://reactome.org", + logo_url="https://reactome.org/templates/favourite/images/logo/logo.png", + favicon_url="https://reactome.org/templates/favourite/favicon.ico", + feedback_url="mailto:help@reactome.org", + support_email="help@reactome.org", +) + +PLANT_REACTOME = Branding( + knowledgebase="Plant Reactome", + chatbot_name="Plant Reactome", + home_url="https://plantreactome.gramene.org", + logo_url="https://plantreactome.gramene.org/templates/favourite/images/logo/logo.png", + # Their site serves no icon at the usual /favicon.ico path; the logo scales + # down to one, and a browser asks for an image either way. + favicon_url="https://plantreactome.gramene.org/templates/favourite/images/logo/logo.png", + feedback_url=( + "https://plantreactome.gramene.org/index.php" + "?Itemid=446&id=1&lang=en&option=com_contact&view=contact" + ), + support_email="gramene@gramene.org", +) + +# Keyed by the lower-cased name, as `agent.profiles.CHAT_PROFILES` is. Keying by +# the ProfileName member itself would not survive a plain string: `Enum` hashes +# on the member's *name* (`React_to_Me`) while comparing equal to its value +# (`React-to-Me`), so a string key would miss and land on the default. +BRANDING: dict[str, Branding] = { + ProfileName.React_to_Me.lower(): REACTOME, + # Reactome's second profile answers from Reactome and UniProt, and is served + # from Reactome's site, so it is Reactome's page. + ProfileName.Cross_Database_Prototype.lower(): REACTOME, + ProfileName.Plant_Reactome.lower(): PLANT_REACTOME, +} + +#: What a deployment that has not said otherwise gets. +DEFAULT_BRANDING = REACTOME + + +def branding_for(profiles: Sequence[str] | None) -> Branding: + """The branding for a deployment's configured profiles. + + Unanimous, or the default. A deployment configured for both knowledgebases at + once is the case this cannot answer -- one page cannot be the guide to both -- + so it says which one it served rather than picking one silently. + """ + if not profiles: + return DEFAULT_BRANDING + + chosen = { + BRANDING.get(str(profile).lower(), DEFAULT_BRANDING) for profile in profiles + } + if len(chosen) > 1: + logging.warning( + "The configured profiles %s do not agree on one brand; serving the %s " + "landing page. A deployment that offers both knowledgebases needs a " + "landing page that names both.", + [str(profile) for profile in profiles], + DEFAULT_BRANDING.knowledgebase, + ) + return DEFAULT_BRANDING + return chosen.pop() diff --git a/src/util/chainlit_helpers.py b/src/util/chainlit_helpers.py index 2fa2d977..613f3ace 100644 --- a/src/util/chainlit_helpers.py +++ b/src/util/chainlit_helpers.py @@ -10,6 +10,7 @@ from langchain_community.callbacks import OpenAICallbackHandler from tools.external_search.state import WebSearchResult +from util.answer_text import answer_text from util.config_yml import Config from util.config_yml.messages import TriggerEvent from util.config_yml.usage_limits import MessageRate @@ -199,3 +200,39 @@ async def update_user() -> None: user: cl.User | None = cl.user_session.get("user") if user: await get_data_layer().create_user(user) + + +class TextStreamCallbackHandler(cl.AsyncLangchainCallbackHandler): + """Chainlit's langchain tracer, minus the non-text stream blocks. + + `on_llm_new_token` stringifies anything that is not a str -- + `"".join(map(str, token))` for a list -- and with + force_stream_final_answer=True the result goes straight into the reader's + message. The Responses API streams reasoning items, so the reader saw + `{'type': 'reasoning', 'encrypted_content': 'gAAAAAB...'}` as the answer. + + This is the surface the browser shows: `final_stream` is the answer, not + `state["answer"]`, so no output parser can reach it. + """ + + async def on_llm_new_token( + self, + token: Any, + *, + chunk: Any = None, + run_id: Any = None, + parent_run_id: Any = None, + **kwargs: Any, + ) -> Any: + if not isinstance(token, str): + text = answer_text(token) + if not text: + return None + token = text + return await super().on_llm_new_token( + token, + chunk=chunk, + run_id=run_id, + parent_run_id=parent_run_id, + **kwargs, + ) diff --git a/src/util/landing_page.py b/src/util/landing_page.py new file mode 100644 index 00000000..3deb5dd2 --- /dev/null +++ b/src/util/landing_page.py @@ -0,0 +1,179 @@ +"""The pages around the chat: the landing page, and the pages a failed human +check ends on. + +Both are rendered here rather than in `bin/chat-fastapi.py` so that they can be +rendered in a test. Importing that module mounts Chainlit, whose configuration is +global, and refuses to start without Turnstile keys -- which is why the checks in +`tests/api` shell out to a probe or grep the source instead of calling anything +(see `tests/api/test_chat_requires_human.py`). Nothing here needs the app. + +Everything a deployment varies comes from `util.branding`; the two functions take +a `Branding` rather than reading config themselves, so which one is in effect is +decided in one place, at startup, where it can be logged. + +`string.Template` rather than `str.format`: these pages are full of CSS braces. +""" + +from string import Template + +from util.branding import Branding + +# The `$` placeholders are filled in by `render_landing_page`. The prose below the +# buttons names no knowledgebase, so it needs no placeholder. +_LANDING_PAGE = Template( + """ + + + + + + +
+ +

$TITLE

+

$TAGLINE

+ +
+ Guest Access + Log In + +
+ +

Choose Guest Access to try the chatbot out. Log In will give an increased query allowance and securely stores your chat history so you can save and continue conversations.

+

We encourage you to use the Feedback button to tell us about your experience with the chatbot and help us improve it.

+
+ + + """ +) + +_ERROR_PAGE = Template( + """ + + +

$ERROR_TITLE

+

If you believe this to be in error, please contact the maintainers to report an issue: $SUPPORT_EMAIL

+
+ +
+ + +""" +) + + +def render_landing_page(branding: Branding, chat_url: str) -> str: + """The page at `/chat/` that sends a reader into the chat. + + `chat_url` is CHAINLIT_URL -- the site the chat is reached at, since the two + chat containers are mounted at their own paths under it. + """ + return _LANDING_PAGE.substitute( + FAVICON_URL=branding.favicon_url, + HOME_URL=branding.home_url, + LOGO_URL=branding.logo_url, + KNOWLEDGEBASE=branding.knowledgebase, + TITLE=f"Meet the {branding.chatbot_name} AI Chatbot!", + TAGLINE=( + f"Your new guide to {branding.knowledgebase}. Whether you're looking " + "for specific genes and pathways or just browsing, our AI Chatbot is " + "here to assist you!" + ), + CHAT_URL=chat_url, + FEEDBACK_URL=branding.feedback_url, + ) + + +def render_error_page(branding: Branding, error_title: str, retry_url: str) -> str: + """What a failed human check shows. + + `error_title` is one of the caller's own literals, never anything from the + request, so it goes in as written. + """ + return _ERROR_PAGE.substitute( + ERROR_TITLE=error_title, + SUPPORT_EMAIL=branding.support_email, + RETRY_URL=retry_url, + ) diff --git a/tests/agent/test_astream_answer.py b/tests/agent/test_astream_answer.py index b802cd5b..de3f3fe2 100644 --- a/tests/agent/test_astream_answer.py +++ b/tests/agent/test_astream_answer.py @@ -27,11 +27,11 @@ def __init__(self, st_id: str, name: str = "") -> None: class _Chunk: - def __init__(self, content: str) -> None: + def __init__(self, content: Any) -> None: self.content = content -def _token(text: str, node: str = ANSWER_NODE) -> dict[str, Any]: +def _token(text: Any, node: str = ANSWER_NODE) -> dict[str, Any]: return { "event": "on_chat_model_stream", "metadata": {"langgraph_node": node}, @@ -133,3 +133,29 @@ def test_an_unknown_profile_fails_rather_than_hanging() -> None: async def _collect_unknown(graph: AgentGraph) -> list[Any]: return [e async for e in graph.astream_answer("q", "nope", thread_id="t")] + + +def test_reasoning_blocks_never_reach_the_answer() -> None: + events = _collect_sync( + [ + _retrieved([]), + _token({"type": "reasoning", "id": "rs_1", "encrypted_content": "gAAAAAB"}), + _token( + [ + {"type": "output_text", "text": "Arabidopsis "}, + {"type": "output_text", "text": "closes stomata."}, + ] + ), + ] + ) + text = "".join(e.text for e in events if e.kind == "token") + assert text == "Arabidopsis closes stomata." + + +def test_a_reasoning_only_stream_reports_nothing_found() -> None: + """`answered` means real text, so reasoning alone is not an answer.""" + events = _collect_sync( + [_retrieved([]), _token({"type": "reasoning", "encrypted_content": "gAAAAAB"})] + ) + assert [e for e in events if e.kind == "token"] == [] + assert events[-1].state == "nothing_found" diff --git a/tests/api/test_answer_matches_chat.py b/tests/api/test_answer_matches_chat.py index 8c65100e..7c7f3df2 100644 --- a/tests/api/test_answer_matches_chat.py +++ b/tests/api/test_answer_matches_chat.py @@ -64,8 +64,17 @@ def test_the_only_configured_difference_is_one_that_cannot_change_the_answer() - @pytest.mark.requires_live_model @pytest.mark.requires_embeddings -@pytest.mark.skipif(not os.getenv("OPENAI_API_KEY"), reason="needs a live model") -def test_both_surfaces_answer_substantively_live() -> None: +@pytest.mark.skipif( + not (os.getenv("LLM_MODEL") and os.getenv("LLM_API_KEY")), + reason="needs the model the app answers with. Gating on OPENAI_API_KEY " + "instead let this run while LLM_MODEL/LLM_BASE_URL/LLM_API_KEY were " + "unset, so it measured the gpt-4o-mini default against api.openai.com " + "rather than the deployed model. Confirm the startup line reads " + "`Answering with /` before trusting a result.", +) +def test_both_surfaces_answer_substantively_live( + monkeypatch: pytest.MonkeyPatch, +) -> None: """Opt-in: both paths, one graph, one question. This does **not** assert the two answers match, and the task that asked for @@ -85,6 +94,16 @@ def test_both_surfaces_answer_substantively_live() -> None: from agent.registry import build_graph from util.anchor_strip import AnchorStripper + # Postgres is reachable only over a unix socket that exists inside the + # container: util/secrets.POSTGRES_SOCKET is the literal "/sockets/postgres/" + # and the server runs with `--auth-host reject`, so there is no network path + # to fall back to. On a host that socket is absent, and this test died in + # PoolTimeout -- 30s of retries, 180s wall clock -- before it asked a single + # question. Checkpointing is not what this test is about; whether the two + # surfaces answer substantively from one graph is. With this unset, + # create_checkpointer() returns a MemorySaver and the pool is never opened. + monkeypatch.delenv("POSTGRES_LANGGRAPH_DB", raising=False) + question = "What does CDK5 do in neurons?" def _strip(text: str) -> str: diff --git a/tests/util/test_answer_text.py b/tests/util/test_answer_text.py new file mode 100644 index 00000000..e939ab4a --- /dev/null +++ b/tests/util/test_answer_text.py @@ -0,0 +1,67 @@ +from types import SimpleNamespace + +from langchain_core.messages import AIMessage, AIMessageChunk + +from util.answer_text import AnswerTextParser, answer_text + + +def test_plain_string_passes_through_unchanged() -> None: + assert answer_text("plain token") == "plain token" + + +def test_a_chunk_is_read_through_to_its_content() -> None: + assert answer_text(SimpleNamespace(content="hello")) == "hello" + + +def test_a_bare_reasoning_block_yields_nothing() -> None: + assert answer_text({"type": "reasoning", "encrypted_content": "gAAAAAB"}) == "" + + +def test_only_text_blocks_are_kept() -> None: + assert ( + answer_text( + [ + {"type": "reasoning", "encrypted_content": "gAAAAAB"}, + {"type": "output_text", "text": "Arabidopsis "}, + {"type": "text", "text": "closes stomata."}, + ] + ) + == "Arabidopsis closes stomata." + ) + + +def test_the_parser_reduces_a_message_to_text() -> None: + """Goes through `invoke`, which is what LLMChain actually calls.""" + msg = AIMessage(content=[{"type": "output_text", "text": "stomata"}]) + assert AnswerTextParser().invoke(msg) == "stomata" + + +def test_the_parser_drops_reasoning_and_keeps_the_prose() -> None: + """The production shape: a reasoning item ahead of the answer text. + + `invoke` does not hand the message to `parse`. It wraps it in a + `ChatGeneration`, whose validator collapses the content to `message.text`, + and `BaseMessage.text` keeps only `type == "text"` -- so an `output_text` + answer arrives at `parse` as an empty string. A parser that overrides only + `parse` cannot see the blocks and blanks the answer instead of fixing it. + """ + msg = AIMessage( + content=[ + {"type": "reasoning", "encrypted_content": "gAAAAAB", "id": "rs_1"}, + {"type": "output_text", "text": "CDK5 phosphorylates "}, + {"type": "output_text", "text": "Tau."}, + ] + ) + assert AnswerTextParser().invoke(msg) == "CDK5 phosphorylates Tau." + + +def test_the_parser_streams_one_item_per_chunk() -> None: + """`StrOutputParser` streamed; so must this. + + Extending plain `BaseOutputParser` would not fail -- `Runnable.transform` + falls back to buffering the input and invoking once -- it would just yield + the whole answer as a single item, which quietly takes token-at-a-time + output away from the answer node. + """ + chunks = [AIMessageChunk(content="alpha"), AIMessageChunk(content="beta")] + assert list(AnswerTextParser().transform(iter(chunks))) == ["alpha", "beta"] diff --git a/tests/util/test_landing_page.py b/tests/util/test_landing_page.py new file mode 100644 index 00000000..da7dd969 --- /dev/null +++ b/tests/util/test_landing_page.py @@ -0,0 +1,156 @@ +"""The pages around the chat, and which deployment's brand each one wears. + +Rendered directly rather than through `bin/chat-fastapi.py`: importing that mounts +Chainlit, whose configuration is global, and it refuses to start without Turnstile +keys -- see `tests/api/test_chat_requires_human.py` for the lengths that goes to. + +What is worth pinning is that the two deployments are the same image. So these +check which `config.yml` changes the page, and that the configuration needing no +change at all is Reactome's: its page has to come out the way it always did, from +the shipped defaults, with nothing for its operators to do. +""" + +import logging +from pathlib import Path + +import pytest + +from agent.profile_names import ProfileName +from util.branding import BRANDING, DEFAULT_BRANDING, Branding, branding_for +from util.config_yml import CONFIG_DEFAULT_YML, Config +from util.landing_page import render_error_page, render_landing_page + +REPO_ROOT = Path(__file__).parents[2] + +REACTOME_TITLE = "

Meet the React-to-Me AI Chatbot!

" +REACTOME_TAGLINE = ( + "Your new guide to Reactome. Whether you're looking for specific genes and " + "pathways or just browsing, our AI Chatbot is here to assist you!" +) +REACTOME_LOGO = "https://reactome.org/templates/favourite/images/logo/logo.png" +REACTOME_FAVICON = "https://reactome.org/templates/favourite/favicon.ico" +HELP_EMAIL = "help@reactome.org" + + +def _landing(branding: Branding, chat_url: str = "https://example.org") -> str: + return render_landing_page(branding, chat_url) + + +def test_no_config_at_all_is_still_the_reactome_page() -> None: + """A checkout with no config.yml, and a config that names no profile.""" + assert branding_for(None) is DEFAULT_BRANDING + assert branding_for([]) is DEFAULT_BRANDING + + +def test_the_shipped_default_config_yields_the_reactome_page() -> None: + """The fallback a Reactome deployment actually runs on. + + Not a fixture written here: this is the file that decides it, so reading the + shipped one is the only assertion that would notice if it changed. + """ + config = Config.from_yaml(CONFIG_DEFAULT_YML) + assert config is not None + assert branding_for(config.profiles) is DEFAULT_BRANDING + + +@pytest.mark.parametrize("profile", ["Plant Reactome", ProfileName.Plant_Reactome]) +def test_a_profile_and_its_plain_string_agree(profile: str) -> None: + """config.yml yields plain strings; the typed Config yields enum members. + + Worth a test because `Enum` hashes on the member's name (`Plant_Reactome`) + while comparing equal to its value (`Plant Reactome`), so a dict keyed by the + member would miss a string and fall back to Reactome's page -- silently, in + the one deployment that needs it not to. + """ + assert branding_for([profile]) is BRANDING["plant reactome"] + + +def test_profiles_that_disagree_on_a_brand_fall_back_and_say_so( + caplog: pytest.LogCaptureFixture, +) -> None: + """One page cannot be the guide to two knowledgebases, so it picks the + default -- but not quietly.""" + profiles = [ProfileName.React_to_Me, ProfileName.Plant_Reactome] + with caplog.at_level(logging.WARNING): + assert branding_for(profiles) is DEFAULT_BRANDING + assert "do not agree" in caplog.text + + +def test_every_profile_has_a_branding() -> None: + """A profile added without one would get Reactome's page in silence.""" + assert set(BRANDING) == {profile.lower() for profile in ProfileName} + + +def test_the_reactome_page_is_the_page_it_has_always_been() -> None: + page = _landing(DEFAULT_BRANDING) + assert REACTOME_TITLE in page + assert REACTOME_TAGLINE in page + assert f'href="{REACTOME_FAVICON}"' in page + assert f'src="{REACTOME_LOGO}"' in page + assert f'href="mailto:{HELP_EMAIL}"' in page + assert "Plant Reactome" not in page + + +def test_the_plant_page_names_plant_reactome() -> None: + plant = BRANDING["plant reactome"] + page = _landing(plant) + assert "

Meet the Plant Reactome AI Chatbot!

" in page + assert "Your new guide to Plant Reactome." in page + assert 'alt="Plant Reactome Logo"' in page + assert plant.home_url in page + assert plant.logo_url in page + assert plant.feedback_url in page + + +def test_the_plant_page_sends_no_one_to_reactome() -> None: + """The reason this exists: a Plant Reactome reader asking Reactome for help. + + `plantreactome.gramene.org` contains "reactome", so the checks below name the + Reactome paths rather than the domain. + """ + page = _landing(BRANDING["plant reactome"]) + assert HELP_EMAIL not in page + assert "reactome.org/templates" not in page + assert 'href="https://reactome.org"' not in page + + +@pytest.mark.parametrize("branding", list(BRANDING.values())) +def test_nothing_is_left_unrendered(branding: Branding) -> None: + """A placeholder the render does not fill is a page with `$LOGO_URL` on it.""" + assert "$" not in _landing(branding) + assert "$" not in render_error_page(branding, "Try again", "/chat/guest") + + +@pytest.mark.parametrize("branding", list(BRANDING.values())) +def test_every_page_offers_both_ways_in_and_feedback(branding: Branding) -> None: + page = _landing(branding, "https://example.org") + assert 'href="https://example.org/chat/guest/"' in page + assert 'href="https://example.org/chat/personal/"' in page + assert branding.feedback_url in page + + +def test_an_unset_chat_url_gives_relative_links_not_the_word_none() -> None: + """CHAINLIT_URL used to interpolate as the literal "None", so every button + read "None/chat/guest/" and went nowhere.""" + page = _landing(DEFAULT_BRANDING, "") + assert 'href="/chat/guest/"' in page + assert "None/chat" not in page + + +def test_the_error_page_names_the_same_knowledgebase() -> None: + """The page a failed human check ends on is part of the introduction.""" + reactome = render_error_page(DEFAULT_BRANDING, "CAPTCHA failed", "/chat/guest") + assert "

CAPTCHA failed

" in reactome + assert HELP_EMAIL in reactome + assert 'action="/chat/guest"' in reactome + + plant = render_error_page(BRANDING["plant reactome"], "CAPTCHA failed", "/chat") + assert HELP_EMAIL not in plant + assert BRANDING["plant reactome"].support_email in plant + + +def test_the_landing_page_is_not_duplicated_in_the_app() -> None: + """The HTML lives in one place, so the two deployments cannot drift.""" + source = (REPO_ROOT / "bin" / "chat-fastapi.py").read_text() + assert "render_landing_page" in source + assert "feedback-button" not in source diff --git a/tests/util/test_secrets.py b/tests/util/test_secrets.py index 0f855d95..1a1a5b0e 100644 --- a/tests/util/test_secrets.py +++ b/tests/util/test_secrets.py @@ -70,6 +70,12 @@ def test_loading_writes_only_file_backed_values( ) -> None: import os + # The name must start absent or the last assertion below proves nothing. + # Passing in CI -- where the environment is clean -- and failing on a host + # where `.env` had been sourced is the tell: this test was reading the + # developer's shell rather than the condition it claims to pin. + monkeypatch.delenv("CLOUDFLARE_SECRET_KEY", raising=False) + (mounted / "TAVILY_API_KEY").write_text("tvly-secret") (mounted / "CLOUDFLARE_SECRET_KEY").write_text(" ") # placeholder monkeypatch.setenv("OPENAI_API_KEY", "sk-from-env")