Tooling, quality gates, and integration of upgrade-langchain / plantreactome / userguide-qa - #167
Merged
Merged
Conversation
This reverts commit f7a5e5c.
The repo had no tests, no ruff/black/isort/pytest configuration, and a mypy config that disabled nearly every check (allow_untyped_defs, allow_untyped_calls, allow_untyped_globals), so "mypy passes" in CI carried little signal. Tooling, all consolidated into pyproject.toml (mypy.ini removed): - ruff replaces black and isort; its I rules sort imports and `ruff format` is black-compatible. Rule set widened from the default (~pyflakes) to E,F,I,UP,B, SIM,RUF. E501 is left to the formatter, which is the only thing that can split a long line. - mypy ratcheted up to meet the code, which was already 73% annotated. Modules that do not yet clear the bar get an explicit override carrying a TODO rather than a blanket relaxation. - pytest configured with markers for the tests that need heavy dependencies. CI now runs `ruff check`, `ruff format --check`, `mypy` and `pytest`, on pushes to main as well as pull requests. Previously only lint ran, and only on PRs. Enabling the rules surfaced real defects, fixed here: three mutable default arguments; CSV_GENERATION_MAP and generate_csv annotated to return a single Neo4jDict where every generator returns a list; update_search_results typed for list[dict[str, str]] but only ever called with list[WebSearchResult]; a storage_client narrowing bug; and four exceptions re-raised without `from`. ProfileName moves to its own leaf module. util.config_yml is pure configuration logic but transitively imported torch, because it needed one enum from agent.profiles whose __init__ eagerly builds the graph registry. agent.profiles re-exports it, so existing imports are unaffected. This is what lets the new tests run without the ML stack installed. The tests are characterization tests: they pin current behaviour so the coming LangChain upgrade has a tripwire. Where that behaviour looks wrong the test asserts it anyway and carries a BUG: comment -- notably that a malformed `interval` in config.yml silently disables rate limiting, since nothing validates config.yml against .config.schema.yaml at runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documents how the chatbot runs behind beta: which embeddings bundles exist on S3, the guest-only environment (no OAuth, no Postgres, captcha self-bypassing), and the docker run that binds it to loopback so Apache is the only route in. The Apache side lives in the WebsiteAngular repo alongside the other beta vhost config. .gitignore gains `.env.*`: the existing `.env` pattern does not match `.env.beta`, so a per-environment file holding API keys was committable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
parse_interval() returned timedelta(0) for anything it could not parse. A zero-length window means every queued timestamp is already outside it, so the message queue drained on every call and no user was ever rate limited -- one typo in config.yml silently removed the quota. It now raises, and the fields that feed it (MessageRate.interval, Trigger.freq_max) carry the same pattern .config.schema.yaml documents, so a bad value is rejected when the config loads rather than reaching the limiter. Nothing validated config.yml against that schema at runtime; this closes the gap for the fields where it mattered. Config.from_yaml no longer fails open. A ValidationError used to return None, and None disables every config-driven feature including rate limiting, so an unreadable config removed the quota for everybody. Invalid or unreadable configs now log at ERROR and fall back to config_default.yml, which does carry a quota; None is reserved for the defaults themselves being unusable. IsADirectoryError is handled explicitly because docker-compose bind-mounts ./config.yml, and Docker creates a directory there when the host file is missing. Trigger start/end compared bounds by stripping tzinfo, which discards an offset instead of converting it, shifting the window by the host's UTC offset -- config_default.yml writes these as "...Z". Both sides are now compared in UTC. freq_max keeps its own naive local clock, matching the timestamps chainlit_helpers writes. match_user([""]) raised IndexError on `entry[0]`; an empty entry is now skipped. Also: evaluator.py derived its BM25 CSV and Chroma collection from absolute paths inside a developer's home directory, so it could not run anywhere else. Both now come from an --embeddings-dir argument defaulting to the installed bundle, and a dead parameter is dropped from process_testset. UniProtDataCleaner.df is declared rather than assigned None, which was one root cause behind ~21 mypy errors; its baseline override is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding a test job doubled the cost of a CI run, because nothing was cached and every job reinstalled the full dependency set from scratch -- including a ~200 MB torch wheel. poetry.toml already puts the environment in ./.venv, so it is now cached wholesale, keyed on poetry.lock. The poetry-check job used tj-actions/changed-files to answer "did poetry.lock change in this PR". This workflow can assume an AWS role, so a third-party action is meaningful supply-chain surface for a question `git diff` answers in one line. Removed. id-token: write was granted workflow-wide, so lint, test, poetry-check and docker-build all carried a credential only docker-push uses. Moved to that job. Added a concurrency group: a second push to a PR made the first run's result irrelevant, but both ran to completion and queued behind each other. Also bumped setup-python v4 -> v5 and build-push-action v5 -> v6. Housekeeping in the same pass: - template.env removed. It held one line, nothing referenced it, and env_template is the file the README actually points at. - PYTHON_PATH dropped from env_template. Python reads PYTHONPATH; the Dockerfile sets that itself and docker-compose never forwarded this variable, so the line has never done anything. - .dockerignore gains .git/, records/, the cache directories and .env.* . Tests for util.config_yml.features, which had none: the postprocessing gate decides whether a Tavily call happens on every message. Includes a test pinning that get_feature() returns True for unknown feature ids -- fail-open, recorded rather than endorsed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings langchain-core 0.3.63 -> 0.3.84 and the embedding chunk_size reductions (reactome 500 -> 400, uniprot 800 -> 500). This is the agreed LangChain ceiling for now; the larger upgrade and the retriever rework it forces are a separate piece of work.
Adds the Plant Reactome profile: retriever, prompt and metadata_info, a profile graph, and ORCiD as an OAuth provider. Conflicts resolved: - Plant_Reactome joins ProfileName in agent/profile_names.py rather than in agent/profiles/__init__.py, where this branch added it. The enum moved to its own leaf module so util.config_yml stops transitively importing torch. - data_generation/reactome/__init__.py keeps `from pathlib import Path`, which this branch's CSV-existence check needs, and drops `typing.Optional`, which nothing uses since those annotations became `X | None`. poetry.lock is KEPT, overriding this branch's "Stop tracking poetry.lock" commit (cd06433), which deleted the file and added it to .gitignore. Without a lockfile there is no way to hold LangChain at a chosen version -- every install would float to the newest release satisfying `^0.3.4` -- which would silently undo the decision to cap langchain-core at 0.3.84. It would also break the Dockerfile's `COPY pyproject.toml poetry.lock ./` and the CI cache key. If dropping the lockfile was deliberate, it needs its own discussion rather than arriving inside a feature branch. The lint and type gates flagged 13 issues in the incoming code, fixed here: unsorted imports, deprecated typing.Dict/List/Tuple, an unused import, and a mutable class attribute on ORCIDOAuthProvider that is now ClassVar. retrievers/plantreactome/rag.py repeats the import-time EmbeddingEnvironment.get_dir() default, so it joins the other two in the mypy baseline and carries the same TODO. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds Reactome user guide Q&A: an HTML loader and fetcher under data_generation/userguide, a userguide retriever and prompt, an intent classifier, and routing in the React-to-Me profile. Brings beautifulsoup4, lxml and requests as dependencies. Merged before the LangChain upgrade deliberately. Code written against the current API has to be ported whenever it lands; landing it first means it is ported once, in the same pass, under the same golden-output baseline. The lockfile merged cleanly and kept langchain-core at 0.3.84. Worth verifying with `poetry check --lock` on a machine with poetry: a textual merge of two lock files keeps one side's content-hash, and pyproject now carries both sides' dependencies. CI's poetry-check job covers this. Gates caught seven issues in the incoming code: unsorted imports, four Optional[...] annotations, and the now-familiar import-time EmbeddingEnvironment.get_dir() default, which this adds for a fourth retriever and which joins the others in the baseline. Two type errors are baselined rather than fixed: - react_to_me nodes return partial ReactToMeState, same shape as the cross_database TypedDict debt. - preprocess() narrows BaseState to ReactToMeState, an LSP violation. The fix is making BaseGraphBuilder generic in its state type, but base.py is also modified by the safetycheck and analysis branches, so a generics refactor there would collide with both. Deferred until those are reconciled. Note for the retriever rewrite: retrievers/userguide/retriever.py is already a plain BaseRetriever taking embeddings_directory as a real argument with explicit errors, rather than inheriting MultiQueryRetriever and overriding its internals. It is the pattern the main retriever should move to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cherry-picks 921a30d (ragas evaluation toolkit) and 1f41e00 (expert survey analysis pipeline) from origin/analysis. The rest of that branch, and all of origin/safetycheck, is superseded: both branched from e398a37 in Nov 2025 and their safety work reached main via 65dcaa4 (#101) three months later, which also added language detection that the branches do not have. The branch copied rather than moved evaluator.py and test_generator.py, leaving byte-identical duplicates under analysis/ragas_evaluation/ alongside src/evaluation/ -- including the two hardcoded /Users/... paths removed from the src/ copy earlier today. The duplicates and the duplicated example CSVs are dropped; src/evaluation/ stays canonical because it is type-checked and already has the fix. Its README is kept and moved to src/evaluation/README.md, which had no documentation before, repointed at the real paths, with the `raga_evaluations` directory typo corrected, the stale `ragas==0.1.21` pin replaced by a reference to poetry.lock, and the embeddings prerequisite rewritten around --embeddings-dir. Config paths are now anchored to the repo instead of the working directory. CONFIG_YML and CONFIG_DEFAULT_YML were Path("config.yml") and Path("config_default.yml"), so running from any directory other than the repo root silently lost the config and fell back to defaults. They now derive from __file__ the way util.embedding_environment already does, which also holds inside the container where /app is the root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Type annotations are now complete: 250/250 functions, up from 73% this morning. That let disallow_untyped_defs, disallow_incomplete_defs, warn_no_return and extra_checks move from per-module overrides to the [tool.mypy] section, so there is a single standard rather than a lenient default with exceptions. The evaluation.* leniency is gone. mypy's scope widens from bin/src/tests to include .github and export_csvs.py, so every Python file in the repo is checked. Two strict flags are deliberately not enabled yet, with a TODO saying why: warn_return_any and disallow_untyped_decorators fire on third-party calls that return Any only when those libraries' types are absent, and they cannot be calibrated without the full dependency set installed. The ruff rule set widens from E,F,I,UP,B,SIM,RUF to add S (bandit), C4, RET, PIE, PT, INT, ICN, TID, A, LOG, G, ERA and N. Rules needing a decision rather than a mechanical fix are listed in pyproject with the reason instead of being dropped silently: PTH (~41 os.path rewrites, worth its own commit), DTZ (the rate limiter deliberately stores naive local timestamps -- changing that is a storage-format decision), ARG (several are required by LangGraph node signatures), BLE and C901. Enabling the security rules surfaced four real problems, fixed here: - bin/chat-fastapi.py called the Cloudflare Turnstile endpoint with no timeout, inside the request path. A slow response would hang the worker indefinitely on a public endpoint. Three offline download helpers had the same gap. - src/evaluation/test_generator.py resolved --distributions keys with eval() on argv. The names it evaluated are not imported anywhere, so it raised NameError rather than working -- the script targets the ragas 0.1 API and needs porting to 0.2, which the replacement now says in a TODO. - bin/export_records.py interpolated a timestamp into SQL run against the production chat history. Now bound as a query parameter. - export_csvs.py, which arrived with the plantreactome branch, carried a hardcoded Neo4j password. Read from the environment instead. One false positive is silenced at the site: ORCIDOAuthProvider.token_url is a URL, flagged only because the name contains "token". ISC001 is excluded because ruff itself warns it conflicts with the formatter, and S101 is allowed in tests because assert is how pytest asserts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything until now was checked with ruff, mypy and 76 tests running without
langchain, chainlit, chromadb or torch installed -- 49 of 63 modules could not
even be imported. With the full environment in place, three things came out.
The retriever characterization test that had never executed FAILED, and the
assumption was mine, not a bug in the code. weighted_reciprocal_rank does not
produce an order-independent result for equally-scored documents: when two docs
tie, sorted() is stable, so the winner is whichever appears first in
chain.from_iterable(doc_lists). In HybridRetriever those lists are the query
variants, and BM25 results precede vector results within each, so ties favour
earlier query variants and BM25. The test now pins that, plus a second test
pinning that the uniform [1/n]*n weights cannot affect ordering at all.
evaluator.py called qa_system.get_context(question). create_rag_chain returns a
create_retrieval_chain runnable, which has no such method -- that raised
AttributeError at runtime. It now invokes with {"input": question}, whose
{input, context, answer} result is exactly what the following lines read.
create_bm25_chroma_ensemble_retriever was annotated to return MergerRetriever
while returning a HybridRetriever. mypy could not see it until from_subdirectory
gained a return type.
Also from the real type check: ORCIDOAuthProvider assigned os.environ.get()
results to fields the base class types as str, so a missing variable became
None; its `env` cannot be narrowed to ClassVar because the base declares it as an
instance variable; and four functions leaked Any through their signatures from
driver and HTTP-body calls.
warn_return_any is now on, calibrated against real libraries rather than
guessed. disallow_untyped_decorators stays off with a note: chainlit's @cl.*
decorators are untyped upstream, so it only ever fires on chat-chainlit.py.
Verified with the full environment: ruff, ruff format, mypy over 74 files, 85
tests, and verify_imports.py importing every entry point -- which is the first
time the merge conflict resolutions in profiles/__init__.py and react_to_me.py
have actually been executed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macOS poetry-check job failed: pipx gives Poetry its own interpreter, which on the macos-15-intel runner is Homebrew's Python 3.14. Poetry then built the project venv with 3.14, pydantic-core has no 3.14 wheel, so it fell back to compiling and PyO3 0.22 does not support 3.14. Installing into the interpreter from actions/setup-python restores the previous behaviour while keeping the venv cache, which does not need Poetry on PATH first the way setup-python's own poetry caching would. Regression from the CI caching change in 65ebc0e. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records what each retriever returns for a fixed question set, per Chroma collection, so a change to retrieval can be diffed rather than argued about. Two steps of the pipeline call an LLM and are not reproducible: the multi-query expansion, and SelfQueryRetriever's translation of a question into a structured query. The harness skips the expansion and feeds questions to the retrievers directly, which makes BM25 exactly reproducible -- verified byte-identical across 80/80 question-collections on two runs -- and plain vector search nearly so at 78/80. Documents are recorded by Reactome stable ID, which survives a bundle rebuild. It is not wired in as a regression gate. The current fusion has defects, so pinning today's output would make fixing them look like breaking a test. See the issues filed alongside this. `overlap` compares SelfQuery against plain vector search within one capture, which is the evidence for whether SelfQuery can be replaced by a plain semantic retriever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An invalid config.yml is now fatal rather than silently substituted. The earlier change made a ValidationError fall back to config_default.yml, which is wrong in a way the original was not: an operator who turns postprocessing off and makes an unrelated typo would get the defaults back, silently re-enabling external web search and its per-message cost, and replacing their quota with 100. Returning None was also wrong -- it disabled the quota entirely. Refusing to start is the only option that cannot quietly do the wrong thing; the typo surfaces at deploy rather than in a bill. An absent config.yml still falls back, including the docker-compose case where a missing host file becomes a directory, because that means "no config supplied" rather than "broken config". A test pins the re-enabling scenario so the fallback is not reintroduced. export_csvs.py now says which variable is missing. Removing the hardcoded Neo4j password left os.environ.get returning None, which the driver accepts and then fails on at connect time with an error that never mentions NEO4J_PASSWORD. The retrieval overlap metric measured less than it appeared to. It used set(), so on `reactions` it compared roughly five distinct documents rather than ten results -- collapsing exactly the duplication that issue #169 is about. The report now shows set overlap, rank agreement, and the distinct-to-k ratio side by side. Rank agreement between selfquery and plain vector is 0.19 against a set overlap of 0.48, so the difference is larger than the original number suggested. Also corrects the harness docstring, which claimed SelfQuery varies run to run. Measured over two identical runs it is stable: 78/80 byte-identical, the same as plain vector search. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
Branch disposition after this mergesEvidence for what is safe to delete, since Safe to delete — fully contained in main after this PR:
Do NOT delete — commits are not in main:
Keep:
Neither 🤖 Generated with Claude Code |
Closes #169 and #170. Both are defects in this repo's fusion logic, independent of the LangChain version. #170: retrieve_documents appended `bm25_docs + vector_docs` as a single list, so RRF ran across query variants and never fused the two retrievers against each other despite the name. Because BM25's ten results came first, every vector result entered at rank 11 or worse and the best of them scored 1/71 against BM25's 1/61. They are now separate lists, so both top hits score 1/61 and the [1/n]*n weights become a real BM25-vs-vector dial rather than a constant multiplier that cannot change ordering. #169: one Reactome entity occupies several CSV rows -- a reaction appears once per pathway/input/output/catalyst combination -- and those rows have distinct page_content, so RRF's page_content de-duplication does not collapse them. Measured on Release95, vector search returned ten results containing 4.8 distinct reactions on average, worst case 4. The vector store is now asked for three times as many results and collapsed to the highest-ranked row per st_id. On the real bundle that takes reactions from 4-5 distinct to a full 10. De-duplication falls back to page_content when a document has no st_id, so a collection without that metadata behaves as before rather than raising. Both the sync and async paths are changed together; they had drifted into slightly different shapes and are now the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 4, 2026
weighted_reciprocal_rank returns every unique document across the lists it is given, not a top-N. So the retriever ranked roughly 222 documents by relevance and then sent all of them to the model -- about 32,000 tokens, a quarter of gpt-4o-mini's context window, on every message. The ranking was decorative: nothing downstream acted on it, so retrieval precision could not affect the answer, only recall could. Measured on Release95, capping at 10 per collection: before 222 documents ~32,000 tokens after 40 documents ~ 6,300 tokens The cap is per collection rather than global on purpose. reactions, summations, complexes and ewas hold different kinds of information, and a single global top-N would let one collection crowd the others out; per collection guarantees each contributes. The measured runs show an even 10/10/10/10 split. The value is a starting point, not a tuned one -- it matches what a single retriever returns. Changing it trades recall against the model's difficulty attending to the middle of a long context, so the right number should come from an answer-quality evaluation rather than from taste. It is a single named constant for that reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integrates three branches into main and brings the repo up to a single quality standard. Everything here is verified with the full dependency set installed: ruff, ruff format, mypy over 74 files, 85 tests,
poetry check --lock, andverify_imports.py.Tooling
E,F,I,UP,B,SIM,RUF,S,C4,RET,PIE,PT,INT,ICN,TID,A,LOG,G,ERA,N.allow_untyped_defs,allow_untyped_calls,allow_untyped_globals). Annotation coverage is now 100% (250/250 functions, up from 73%), sodisallow_untyped_defs,disallow_incomplete_defs,warn_return_any,warn_no_returnandextra_checksare enforced repo-wide rather than per-module. Scope widened frombin/src/teststo every Python file.pyproject.toml;mypy.iniremoved.ruff check,ruff format --check,mypyandpytest, on pushes to main as well as PRs. The venv is cached — previously every job reinstalled the full dependency set including a ~200 MB torch wheel.tj-actions/changed-filesdropped in favour of one line ofgit diff; this workflow can assume an AWS role, so a third-party action was meaningful supply-chain surface.id-token: writenarrowed from workflow-wide to the one job that uses it.Branches merged
upgrade-langchain— langchain-core 0.3.63 -> 0.3.84, embedding chunk sizes.plantreactome— Plant Reactome profile and ORCiD OAuth. The branch commit "Stop tracking poetry.lock" was not taken: without a lockfile there is no way to hold LangChain at a chosen version, and it would break the DockerfileCOPYand the CI cache key.feature/userguide-qa— user guide Q&A, intent routing.analysis(ragas toolkit, expert-survey pipeline). Their duplicatedevaluator.py/test_generator.pywere dropped in favour of the existingsrc/evaluation/copies.safetycheckand the remainder ofanalysisare superseded: both branched frome398a37in Nov 2025, and their safety work reached main via65dcaa4(#101) three months later, which also added language detection they lack. Recommend closing them.Bugs found and fixed
parse_intervalreturnedtimedelta(0)for anything unparseable, making the window zero-length so the queue drained every call and nobody was limited. It now raises, and the config fields carry the pattern.config.schema.yamldocuments.Config.from_yamlfailed open. AValidationErrorreturnedNone, andNonedisables every config-driven feature including the quota. Invalid configs now fall back toconfig_default.yml.export_records.pyinterpolated a timestamp into SQL run against production chat history; now a bound parameter.test_generator.pyraneval()on argv (and was already broken — the names it evaluated are not imported).evaluator.pycalledqa_system.get_context(), which does not exist on acreate_retrieval_chainrunnable — anAttributeErrorat runtime. Its hardcoded/Users/...paths are gone too.export_csvs.pycarried a hardcoded Neo4j password; now read from the environment.config.yml.Known and deliberately deferred
Characterization tests pin current behaviour where it looks wrong, each with a
BUG:comment, so changing them is deliberate. Notably: RRF ties are broken by list position, so BM25 and earlier query variants win ties; and the uniform[1/n]*nweights cannot affect ordering at all.PTH,DTZ,ARG,BLEandC901are listed inpyproject.tomlwith the reason they are not yet enforced rather than dropped silently. Three mypy baseline entries carryTODO(phase-2).🤖 Generated with Claude Code