Repository navigation
Plantreactome - #321
Open
elserj wants to merge 8 commits into
Open
Plantreactome#321elserj wants to merge 8 commits into
elserj wants to merge 8 commits into
Conversation
initdb/init.sql had not been touched in 22 months while chainlit was bumped to 2.11. The data layer builds its INSERT column list from every non-None key of the dict it is handed -- there is no whitelist -- so each table must be a superset of its TypedDict. With no CI coverage for initdb, the drift went unnoticed until every read and write raised psycopg.errors.UndefinedColumn. Derive the schema from the installed TypedDicts (StepDict, ElementDict, ThreadDict, FeedbackDict) rather than docs.chainlit.io, which is missing autoCollapse, icon, path, autoPlay and playerConfig. Add command and modes to steps, path/autoPlay/playerConfig to elements, and make feedbacks.threadId nullable because FeedbackDict has no threadId key. Add bin/check_schema.py, which validates each table against the fields the data layer actually writes. It has two modes: --sql parses initdb/init.sql and needs no database, so it runs on a developer's machine and from checks.sh; --dsn queries a live database, which is what CI runs against a Postgres service built from initdb/init.sql. Also ignore sockets/ in .gitignore and .dockerignore so the unix socket directory is never committed.
bin/chat-fastapi.py calls Request.form(max_files=0, max_part_size=8192). max_part_size was added to Request.form() in Starlette 0.44.0 and is only enforced in FormParser from 1.3.1. Starlette arrives transitively via chainlit with no floor stated anywhere, and the published image shipped 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. Declare starlette >= 1.3.1 directly. A transitive version is not a floor.
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 404 before the deployment name in the body is ever read, so no value of LLM_MODEL can fix it. Add the openai-responses provider, which sets use_responses_api=True. Add LLM_API_KEY_HEADER so the key travels in the `api-key` header that APIM reads; the SDK's default Authorization: Bearer gets a 401. Leave unset for api.openai.com and other OpenAI-compatible servers, which want only the bearer token. Wrap api_key in SecretStr so langchain-openai 1.x accepts it and the key stays out of repr(), preventing a future logging of the constructed client from writing a key to the log.
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 renders a Python repr of
base64 reasoning into the answer.
Add src/util/answer_text.py with answer_text(), which handles str /
list / bare dict / anything with a .content attribute, and
AnswerTextParser(BaseTransformOutputParser[str]).
Overriding parse() alone is dead code: invoke() and transform() call
parse_result([ChatGeneration(message=...)]), and ChatGeneration flattens
content in a model validator via message.text, which keeps only blocks
with type == "text". For output_text blocks that collapsed string is
empty, so a parse-only override silently blanks the answer. The work
happens in parse_result, the one choke point every path goes through,
which still has the message.
Extend BaseTransformOutputParser rather than BaseOutputParser because
every call site this replaces used StrOutputParser, which streams.
Plain BaseOutputParser would buffer the input and yield the whole
answer as one chunk, quietly taking token-at-a-time output away from
the answer node.
Three independent paths treated model content as a string: - astream_answer did str(chunk.content), so the reasoning repr reached the caller's panel. - create_stuff_documents_chain had no output parser, so state["answer"] carried content blocks into the chat UI, the chat history and the steps row. - Six task chains used StrOutputParser(), which is a no-op passthrough on the invoke/transform path and would silently blank answers if output_text blocks ever arrived. Replace StrOutputParser with AnswerTextParser everywhere, and route the streaming boundary through answer_text(). The chat UI uses ainvoke, not astream_answer, so the streaming fix alone never touched the browser -- that is fixed separately. Add two tests pinning the production shape: reasoning items ahead of output_text blocks through invoke(), and one item per chunk through transform().
answer_with_model builds the callback handler with
force_stream_final_answer=True, so every llm token streams into the
reader's final_stream message. on_llm_new_token stringifies anything
that is not a str -- "".join(map(str, token)) for a list -- and the
Responses API streams reasoning items as a list of dicts. The reader
saw {'type': 'reasoning', 'encrypted_content': 'gAAAAAB...'} as the
answer text.
This is the surface the browser shows: final_stream is the answer, not
state["answer"], so no output parser can reach it. Add
TextStreamCallbackHandler, which reduces non-str tokens through
answer_text() and drops empty results. Text deltas pass through
unchanged; output_text blocks reduce to their prose rather than their
repr; reasoning items reduce to "" and are dropped before they reach
the socket.
with_structured_output(method="json_schema") binds the pydantic CLASS
as response_format, and langchain-openai serializes it only at request
time. _build_llm_settings copies invocation_params verbatim into
generation.settings, and SQLAlchemyDataLayer.create_step does
json.dumps(step_dict.get("generation", {})). A pydantic model class
(ModelMetaclass) is not JSON serializable, so the INSERT died and the
llm step was deleted from thread history entirely. The exception was
swallowed as "Task exception was never retrieved".
Patch GenerationHelper._build_llm_settings to run
ensure_values_serializable() on the settings dict before it reaches
json.dumps. The class becomes its str() representation, which is
serializable and informative in the UI. The 2-tuple guard preserves the
existing None, None return path when invocation_params is absent.
Bare pytest 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. Make the default explicit with -m "not requires_live_model" in addopts; a CLI -m overrides it. The live test also gated on OPENAI_API_KEY while the app answers with LLM_MODEL/LLM_API_KEY, so it ran against the gpt-4o-mini default on api.openai.com rather than the deployed model. Gate on LLM_MODEL and LLM_API_KEY instead, and unset POSTGRES_LANGGRAPH_DB so the test uses MemorySaver instead of timing out for 30s on a host where the Postgres socket does not exist. Finally, make test_loading_writes_only_file_backed_values hermetic by deleting CLOUDFLARE_SECRET_KEY first; it was passing in CI because the environment is clean and failing on hosts where .env had been sourced.
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
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.
Summary
Fixes two production defects that were hiding in plain sight:
psycopg.errors.UndefinedColumnon every chainlit read/write —initdb/init.sqlhad drifted 22 months behind the installed chainlit. The data layer builds INSERT column lists from every non-None key with no whitelist, so missing columns failed every INSERT.contentas a string: the streaming boundary, the RAG output parser, and chainlit's token stream. The Responses API streams reasoning items as{'type': 'reasoning', 'encrypted_content': ...}, and stringifying them put base64 reprs into the answer, the chat history, and thestepsrow.Also includes the dependency floor that was missing for the captcha route, and makes the live-model test suite opt-in so bare
pytestno longer spends ~160s and real money per run.What changed
fix(db): rebuild Chainlit 2.11 schema and guard against driftinitdb/init.sql— rebuilt from the installed TypedDicts (StepDict,ElementDict,ThreadDict,FeedbackDict) instead of the stale docs page. Addscommand/modestosteps,path/autoPlay/playerConfigtoelements, and makesfeedbacks.threadIdnullable.bin/check_schema.py(new) — validates each table against the fields the data layer actually writes.--sqlparsesinitdb/init.sqland needs no database (runs on dev machines and inchecks.sh);--dsnqueries a live database (CI runs it against a Postgres service built frominit.sql)..github/workflows/ci.yml— newschema:job that builds the databases frominitdb/init.sqland runs the check.checks.sh— adds the--sqlschema step..gitignore/.dockerignore— ignoresockets/.Raised as issue Chainlit schema drift causes UndefinedColumn on every read/write #319
fix(captcha): declare the Starlette floor the form route needspyproject.toml— declaresstarlette >= 1.3.1. The captcha route callsRequest.form(max_part_size=8192), which was added in 0.44.0 and only enforced inFormParserfrom 1.3.1. The published image shipped 0.41.3, so every visitor got a 500 on the captcha POST with nothing in the dependency set to explain it.bin/chat-fastapi.py— documents why the floor exists (comment-only; the call was already there).poetry.lock— regenerated.feat(llm): support the Responses API and APIM's api-key headersrc/agent/models.py— addsopenai-responsesprovider (use_responses_api=True),api_key(wrapped inSecretStr), andapi_key_header(sends the key in theapi-keyheader that Azure API Management reads; the SDK's defaultAuthorization: Bearergets a 401).src/agent/graph.py— passesLLM_API_KEYandLLM_API_KEY_HEADERthrough toget_llm/get_embedding..config.schema.yaml/env_template— addsopenai-responsesto the provider enum and documentsLLM_API_KEY_HEADER.feat(answer): add answer_text reducer for model outputsrc/util/answer_text.py(new) —answer_text()handles str / list / bare dict / anything with.content.AnswerTextParser(BaseTransformOutputParser[str])overridesparse_result, notparse, becauseinvoke()/transform()callparse_result([ChatGeneration(message=...)]), andChatGenerationflattens content viamessage.text(which keeps onlytype == "text"blocks) before any parser sees it. Aparse-only override is dead code and would silently blank answers onoutput_textpaths.fix(answer): reduce content blocks at every boundary we ownsrc/agent/graph.py—astream_answerroutes chunks throughanswer_text()instead ofstr(chunk.content).src/retrievers/rag_chain.py— passesoutput_parser=AnswerTextParser()tocreate_stuff_documents_chain.src/agent/tasks/**— replacesStrOutputParser()withAnswerTextParser().tests/agent/test_astream_answer.py— two new tests pinning the production shape (reasoning ahead ofoutput_textthroughinvoke; one item per chunk throughtransform).fix(chainlit): drop non-text stream blocks before the reader sees themsrc/util/chainlit_helpers.py— newTextStreamCallbackHandlerthat reduces non-str tokens throughanswer_text()and drops empty results. This is the surface the browser shows (final_streamis the answer, notstate["answer"]), so no output parser could reach it.bin/chat-chainlit.py— usesTextStreamCallbackHandlerinanswer_with_model.fix(chainlit): sanitize persisted llm settingsbin/chat-chainlit.py— monkeypatchesGenerationHelper._build_llm_settingsto runensure_values_serializable()on the settings dict.with_structured_output(method="json_schema")binds the pydantic class asresponse_format, which is not JSON serializable; the INSERT died and the llm step was deleted from thread history, surfacing only as "Task exception was never retrieved". This is a temporary workaround until chainlit upstream sanitizes the settings.-Raised as issue ModelMetaclass is not JSON serializable in persisted llm step settings #320
test: stop live-model tests running by defaultpyproject.toml—addoptsgains-m "not requires_live_model"; barepytestno longer runs live tests.tests/api/test_answer_matches_chat.py— gates onLLM_MODELandLLM_API_KEY(the model the app actually answers with) instead ofOPENAI_API_KEY; unsetsPOSTGRES_LANGGRAPH_DBso the test usesMemorySaverinstead of timing out for 30s on hosts without the Postgres socket.tests/util/test_secrets.py— makes the loading test hermetic by deletingCLOUDFLARE_SECRET_KEYfirst.Verification
./checks.sh— all 5 checks pass:1418 tests pass, 1 skipped, 1 deselected (the live-model test).
Follow-ups
ensure_values_serializablecall in_build_llm_settings(commit 7 workaround can be deleted once fixed), and the 2-vs-4-tuple return path.pytest -m requires_live_modelto confirm both surfaces answer substantively (it costs real tokens; the gate now prevents accidental runs).addoptsrevert decision: the-m "not requires_live_model"means naming a node ID no longer runs it without-m requires_live_model. Revert that half if you'd rather keep spending the 160s.StrOutputParsersource-scan guard: deferred; the three-path story from this PR is the argument for it.Dockerfile.local,docker-compose.override.yml,export_csvs.sh,plantreactome_review.diff,poetry.lock.org,python— decide whether each belongs in the repo or should be removed/gitignored.Upstream issues to file
GenerationHelper._build_llm_settingsshould runensure_values_serializableon its settings dict (removes the commit 7 monkeypatch)._build_llm_settingsreturnsNone, Nonewheninvocation_paramsis absent but callers unpack four values.