From c004c9dec3e93ceea8f54924c5a4a94e1d2e9170 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 4 Sep 2026 21:28:21 +0000 Subject: [PATCH 1/2] Default the embedding model to the one that built the installed bundle EMBEDDING_MODEL defaulted to the literal "bge-m3", which OpenAI has no such model for, so any deployment that did not set the variable got a 404 on its first query: The model `bge-m3` does not exist or you do not have access to it. Confirmed against the API. This arrived on main with the plantreactome merge earlier today; the lint and type gates cannot see it because it is a runtime configuration mismatch, and nothing exercised it. Queries are embedded and compared against vectors already in Chroma, so the model that built the bundle is the only correct answer -- not a constant kept in sync by hand. resolve_embedding_model() now reads it from the installed bundle, falls back to text-embedding-3-large when nothing is installed, and still honours EMBEDDING_MODEL. A configured value that disagrees with the bundle is logged as an error, because that combination produces meaningless retrieval rather than an obvious failure. Note EmbeddingEnvironment.get_model raises KeyError for a database that is not installed, while its sibling get_dir returns None for the same condition. Handled at the call site rather than changed, since it is a shared API. Five tests, including one asserting the default is never "bge-m3" specifically. env_template and the beta template now document both model variables. Co-Authored-By: Claude Opus 5 --- deploy/beta/env.beta.template | 7 ++ env_template | 7 ++ src/agent/graph.py | 38 +++++++++- .../agent/test_embedding_model_resolution.py | 70 +++++++++++++++++++ 4 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 tests/agent/test_embedding_model_resolution.py diff --git a/deploy/beta/env.beta.template b/deploy/beta/env.beta.template index d3f2787..8a2f7e2 100644 --- a/deploy/beta/env.beta.template +++ b/deploy/beta/env.beta.template @@ -16,3 +16,10 @@ UVICORN_LOG_LEVEL=info # Chainlit is mounted here; the landing page lives at /chat/ CHAINLIT_URI=/chat/guest CHAINLIT_URL=https://beta.reactome.org + +# Embedding model. Leave unset: it defaults to whatever built the installed +# bundle, which is what the stored vectors were produced with. Setting it to +# anything else makes retrieval meaningless. +#EMBEDDING_MODEL= +# LLM used for answers; defaults to gpt-4o-mini. +#LLM_MODEL= diff --git a/env_template b/env_template index 48f0ea2..e096380 100644 --- a/env_template +++ b/env_template @@ -14,3 +14,10 @@ CHAINLIT_IMAGE=public.ecr.aws/reactome/reactome-chatbot:latest CHAINLIT_URI=/chat CHAINLIT_ROOT_PATH= TAVILY_API_KEY= + +# Embedding model. Leave unset: it defaults to whatever built the installed +# bundle, which is what the stored vectors were produced with. Setting it to +# anything else makes retrieval meaningless. +#EMBEDDING_MODEL= +# LLM used for answers; defaults to gpt-4o-mini. +#LLM_MODEL= diff --git a/src/agent/graph.py b/src/agent/graph.py index 7b50cf7..aba7355 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -16,6 +16,7 @@ from agent.models import get_embedding, get_llm from agent.profiles import ProfileName, create_profile_graphs from agent.profiles.base import InputState, OutputState +from util.embedding_environment import EmbeddingEnvironment from util.logging import logging LANGGRAPH_DB_URI = f"postgresql://{os.getenv('POSTGRES_USER')}:{os.getenv('POSTGRES_PASSWORD')}@postgres:5432/{os.getenv('POSTGRES_LANGGRAPH_DB')}?sslmode=disable" @@ -24,13 +25,48 @@ logging.warning("POSTGRES_LANGGRAPH_DB undefined; falling back to MemorySaver.") +DEFAULT_EMBEDDING_MODEL = "text-embedding-3-large" + + +def resolve_embedding_model() -> str: + """Pick the embedding model, defaulting to whatever built the installed bundle. + + A query is embedded with this model and compared against vectors produced by + whichever model built the bundle. If they differ the comparison is + meaningless, so the bundle is the right source of truth rather than a + constant that has to be kept in sync by hand. + + The default used to be a literal "bge-m3", which OpenAI has no such model + for -- so any deployment that did not set EMBEDDING_MODEL got a 404 on its + first query. + """ + configured = os.getenv("EMBEDDING_MODEL") + try: + installed = EmbeddingEnvironment.get_model("reactome") + except KeyError: + # get_model raises when the database is not installed, unlike get_dir + # which returns None for the same condition. + return configured or DEFAULT_EMBEDDING_MODEL + + # get_model returns "/"; the provider is supplied separately. + bundle_model = installed.split("/", 1)[-1] + if configured and configured != bundle_model: + logging.error( + f"EMBEDDING_MODEL is {configured!r} but the installed bundle was built " + f"with {bundle_model!r}. Queries will be embedded with a different " + "model than the stored vectors, so retrieval results will be " + "meaningless. Unset EMBEDDING_MODEL, or install a matching bundle." + ) + return configured or bundle_model + + class AgentGraph: def __init__( self, profiles: list[ProfileName], ) -> None: # Get base models - embedding_model = os.getenv("EMBEDDING_MODEL", "bge-m3") + embedding_model = resolve_embedding_model() llm_model = os.getenv("LLM_MODEL", "gpt-4o-mini") llm_base_url = os.getenv("LLM_BASE_URL", None) llm: BaseChatModel = get_llm( diff --git a/tests/agent/test_embedding_model_resolution.py b/tests/agent/test_embedding_model_resolution.py new file mode 100644 index 0000000..83e3f17 --- /dev/null +++ b/tests/agent/test_embedding_model_resolution.py @@ -0,0 +1,70 @@ +"""The query embedding model must match the one that built the bundle. + +Queries are embedded and compared against vectors already in Chroma. If the two +models differ the comparison is meaningless -- and if the model does not exist at +the provider, the first query 404s. + +The default used to be a literal "bge-m3", which OpenAI has no such model for, so +any deployment that did not set EMBEDDING_MODEL was broken. That reached main in +the plantreactome merge. +""" + +from pathlib import Path + +import pytest + +import util.embedding_environment as ee +from agent.graph import DEFAULT_EMBEDDING_MODEL, resolve_embedding_model + +BUNDLE = "openai/text-embedding-3-large/reactome/Release95" + + +@pytest.fixture +def _installed(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(ee, "EM_ARCHIVE", tmp_path) + monkeypatch.setattr(ee, "EM_CURRENT", tmp_path / "current") + (tmp_path / "current").write_text(BUNDLE) + + +@pytest.mark.usefixtures("_installed") +def test_defaults_to_the_model_that_built_the_bundle( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("EMBEDDING_MODEL", raising=False) + assert resolve_embedding_model() == "text-embedding-3-large" + + +@pytest.mark.usefixtures("_installed") +def test_never_defaults_to_a_model_the_provider_does_not_have( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The specific regression: "bge-m3" is not an OpenAI model.""" + monkeypatch.delenv("EMBEDDING_MODEL", raising=False) + assert resolve_embedding_model() != "bge-m3" + + +@pytest.mark.usefixtures("_installed") +def test_env_override_is_respected(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("EMBEDDING_MODEL", "text-embedding-3-small") + assert resolve_embedding_model() == "text-embedding-3-small" + + +@pytest.mark.usefixtures("_installed") +def test_mismatch_is_reported( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A silent mismatch produces meaningless retrieval, so it must be loud.""" + monkeypatch.setenv("EMBEDDING_MODEL", "text-embedding-3-small") + with caplog.at_level("ERROR"): + resolve_embedding_model() + assert "text-embedding-3-large" in caplog.text + assert "meaningless" in caplog.text + + +def test_falls_back_when_no_bundle_is_installed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(ee, "EM_ARCHIVE", tmp_path) + monkeypatch.setattr(ee, "EM_CURRENT", tmp_path / "current") + monkeypatch.delenv("EMBEDDING_MODEL", raising=False) + assert resolve_embedding_model() == DEFAULT_EMBEDDING_MODEL From 836b4a1daaadbba0efce063428c5c2ab51c23686 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 4 Sep 2026 21:32:35 +0000 Subject: [PATCH 2/2] Resolve the embedding model from every installed bundle, not just Reactome's Corrects the previous commit, which hardcoded the Reactome bundle and so would have broken Plant Reactome instead. "bge-m3" is not a bad model -- it is the right one for Plant Reactome, which serves it from a self-hosted OpenAI-compatible endpoint via OPENAI_BASE_URL. It is wrong only as a *default*, because every bundle published for Reactome uses text-embedding-3-large and gets a 404 from api.openai.com. Hardcoding either model breaks the other deployment, so neither is hardcoded: the bundle path records which model built it, and that is the source of truth. AgentGraph builds one embedding shared by every profile, so all installed bundles must agree. Bundles built with different models are now reported as the misconfiguration they are, rather than one of them silently returning nonsense. The default arrived in a8aa04c, "Initial commit of adding PlantReactome profile", which reached main through the integration PR without ever having had a pull request of its own. Seven tests: Reactome resolves to text-embedding-3-large, Plant Reactome to bge-m3, an override is honoured, an override disagreeing with the bundle is reported, mixed bundles are reported, and nothing installed falls back. Co-Authored-By: Claude Opus 5 --- src/agent/graph.py | 58 +++++++++------ .../agent/test_embedding_model_resolution.py | 70 +++++++++++-------- 2 files changed, 77 insertions(+), 51 deletions(-) diff --git a/src/agent/graph.py b/src/agent/graph.py index aba7355..4fafd9d 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -29,33 +29,51 @@ def resolve_embedding_model() -> str: - """Pick the embedding model, defaulting to whatever built the installed bundle. - - A query is embedded with this model and compared against vectors produced by - whichever model built the bundle. If they differ the comparison is - meaningless, so the bundle is the right source of truth rather than a - constant that has to be kept in sync by hand. - - The default used to be a literal "bge-m3", which OpenAI has no such model - for -- so any deployment that did not set EMBEDDING_MODEL got a 404 on its - first query. + """Pick the embedding model, defaulting to whatever built the installed bundles. + + A query is embedded with this model and compared against vectors already in + Chroma, so it has to match the model that produced them. The bundle path + records that -- `openai/text-embedding-3-large/reactome/Release95` -- which + makes it the source of truth rather than a constant kept in sync by hand. + + The default used to be a literal "bge-m3". That is the right model for the + Plant Reactome deployment, which serves it from a self-hosted + OpenAI-compatible endpoint via OPENAI_BASE_URL, but it is wrong for every + bundle published for Reactome: against api.openai.com it is a 404 on the + first query. Hardcoding either one breaks the other deployment, so neither + is hardcoded. + + AgentGraph builds a single embedding shared by every profile, so all + installed bundles must agree on the model. Disagreement is a real + misconfiguration and is reported rather than silently resolved. """ configured = os.getenv("EMBEDDING_MODEL") - try: - installed = EmbeddingEnvironment.get_model("reactome") - except KeyError: - # get_model raises when the database is not installed, unlike get_dir - # which returns None for the same condition. + + # Bundle paths are "///"; the provider is + # supplied separately to get_embedding, so only the model is wanted here. + installed = { + bundle.parent.parent.name for bundle in EmbeddingEnvironment.get_dict().values() + } + + if not installed: + return configured or DEFAULT_EMBEDDING_MODEL + + if len(installed) > 1: + logging.error( + f"Installed bundles were built with different embedding models " + f"({', '.join(sorted(installed))}), but one embedding is shared by " + "every profile. Retrieval will be meaningless for whichever does not " + "match. Install bundles built with the same model." + ) return configured or DEFAULT_EMBEDDING_MODEL - # get_model returns "/"; the provider is supplied separately. - bundle_model = installed.split("/", 1)[-1] + bundle_model = installed.pop() if configured and configured != bundle_model: logging.error( f"EMBEDDING_MODEL is {configured!r} but the installed bundle was built " - f"with {bundle_model!r}. Queries will be embedded with a different " - "model than the stored vectors, so retrieval results will be " - "meaningless. Unset EMBEDDING_MODEL, or install a matching bundle." + f"with {bundle_model!r}. Queries would be embedded with a different " + "model than the stored vectors, making retrieval meaningless. Unset " + "EMBEDDING_MODEL, or install a matching bundle." ) return configured or bundle_model diff --git a/tests/agent/test_embedding_model_resolution.py b/tests/agent/test_embedding_model_resolution.py index 83e3f17..f8682c8 100644 --- a/tests/agent/test_embedding_model_resolution.py +++ b/tests/agent/test_embedding_model_resolution.py @@ -1,12 +1,13 @@ """The query embedding model must match the one that built the bundle. Queries are embedded and compared against vectors already in Chroma. If the two -models differ the comparison is meaningless -- and if the model does not exist at -the provider, the first query 404s. +models differ the comparison is meaningless, and if the model does not exist at +the configured endpoint the first query fails outright. -The default used to be a literal "bge-m3", which OpenAI has no such model for, so -any deployment that did not set EMBEDDING_MODEL was broken. That reached main in -the plantreactome merge. +The default used to be the literal "bge-m3". That is correct for the Plant +Reactome deployment, which serves it from a self-hosted OpenAI-compatible +endpoint via OPENAI_BASE_URL, and wrong for every bundle published for Reactome, +where it 404s against api.openai.com. Hardcoding either breaks the other. """ from pathlib import Path @@ -16,55 +17,62 @@ import util.embedding_environment as ee from agent.graph import DEFAULT_EMBEDDING_MODEL, resolve_embedding_model -BUNDLE = "openai/text-embedding-3-large/reactome/Release95" +REACTOME = "openai/text-embedding-3-large/reactome/Release95" +PLANT = "openai/bge-m3/plantreactome/Release68" @pytest.fixture -def _installed(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def archive(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: monkeypatch.setattr(ee, "EM_ARCHIVE", tmp_path) monkeypatch.setattr(ee, "EM_CURRENT", tmp_path / "current") - (tmp_path / "current").write_text(BUNDLE) + monkeypatch.delenv("EMBEDDING_MODEL", raising=False) + return tmp_path -@pytest.mark.usefixtures("_installed") -def test_defaults_to_the_model_that_built_the_bundle( - monkeypatch: pytest.MonkeyPatch, -) -> None: - monkeypatch.delenv("EMBEDDING_MODEL", raising=False) +def test_reactome_bundle_resolves_to_its_own_model(archive: Path) -> None: + (archive / "current").write_text(REACTOME) assert resolve_embedding_model() == "text-embedding-3-large" -@pytest.mark.usefixtures("_installed") -def test_never_defaults_to_a_model_the_provider_does_not_have( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The specific regression: "bge-m3" is not an OpenAI model.""" - monkeypatch.delenv("EMBEDDING_MODEL", raising=False) +def test_plantreactome_bundle_resolves_to_bge_m3(archive: Path) -> None: + """Plant Reactome legitimately uses bge-m3; it must not be overridden.""" + (archive / "current").write_text(PLANT) + assert resolve_embedding_model() == "bge-m3" + + +def test_never_silently_defaults_to_a_model_no_bundle_used(archive: Path) -> None: + """The regression: "bge-m3" was the default regardless of what was installed.""" + (archive / "current").write_text(REACTOME) assert resolve_embedding_model() != "bge-m3" -@pytest.mark.usefixtures("_installed") -def test_env_override_is_respected(monkeypatch: pytest.MonkeyPatch) -> None: +def test_env_override_is_respected( + archive: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + (archive / "current").write_text(REACTOME) monkeypatch.setenv("EMBEDDING_MODEL", "text-embedding-3-small") assert resolve_embedding_model() == "text-embedding-3-small" -@pytest.mark.usefixtures("_installed") -def test_mismatch_is_reported( - monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +def test_override_that_disagrees_with_the_bundle_is_reported( + archive: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: - """A silent mismatch produces meaningless retrieval, so it must be loud.""" + (archive / "current").write_text(REACTOME) monkeypatch.setenv("EMBEDDING_MODEL", "text-embedding-3-small") with caplog.at_level("ERROR"): resolve_embedding_model() assert "text-embedding-3-large" in caplog.text - assert "meaningless" in caplog.text -def test_falls_back_when_no_bundle_is_installed( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +def test_bundles_built_with_different_models_are_reported( + archive: Path, caplog: pytest.LogCaptureFixture ) -> None: - monkeypatch.setattr(ee, "EM_ARCHIVE", tmp_path) - monkeypatch.setattr(ee, "EM_CURRENT", tmp_path / "current") - monkeypatch.delenv("EMBEDDING_MODEL", raising=False) + """One embedding is shared by every profile, so the bundles must agree.""" + (archive / "current").write_text(f"{REACTOME}:{PLANT}") + with caplog.at_level("ERROR"): + resolve_embedding_model() + assert "different embedding models" in caplog.text + + +def test_falls_back_when_nothing_is_installed(archive: Path) -> None: assert resolve_embedding_model() == DEFAULT_EMBEDDING_MODEL