diff --git a/server/api/views/assistant/agentic_loop.py b/server/api/views/assistant/agentic_loop.py new file mode 100644 index 00000000..a824dbb8 --- /dev/null +++ b/server/api/views/assistant/agentic_loop.py @@ -0,0 +1,137 @@ +import json +import logging + +from api.views.assistant.assistant_types import ( + AssistantResult, + ToolCall, + ToolCallStatus, +) + +logger = logging.getLogger(__name__) + + +def handle_tool_calls_with_reasoning( + response, client, model_defaults: dict, tools: list, user +) -> AssistantResult: + """ + TODO: Read server/api/views/assistant and fill in the docstring + + TODO: Reference the OpenAI Cookbook in the docstring + + # Open AI Cookbook: Handling Function Calls with Reasoning Models + # https://cookbook.openai.com/examples/reasoning_function_calls + """ + + # Every tool call the run made, accumulated across turns: AssistantResult reports + # the whole run, not just the turn that happened to end it. + all_tool_calls: list[ToolCall] = [] + while True: + # user is threaded through so tools that need it get it at dispatch time; tools that don't simply ignore it. + tool_output_messages, turn_tool_calls = invoke_functions_from_response(response, tools, user) + all_tool_calls.extend(turn_tool_calls) + if not tool_output_messages: # Model emitted no tool calls this turn + logger.info("Reasoning completed") + final_response_output_text = response.output_text + final_response_id = response.id + logger.info(f"Final response: {final_response_output_text}") + return AssistantResult( + output_text=final_response_output_text, + response_id=final_response_id, + tool_calls=all_tool_calls, + ) + else: + logger.info("More reasoning required, continuing...") + response = client.responses.create( + input=tool_output_messages, + previous_response_id=response.id, + **model_defaults, + ) + + +def invoke_functions_from_response( + response, tools: list, user +) -> tuple[list[dict], list[ToolCall]]: + """ + TODO: Read server/api/views/assistant and fill in the docstring + + TODO: Reference the OpenAI Cookbook in the docstring + + # Open AI Cookbook: Handling Function Calls with Reasoning Models + # https://cookbook.openai.com/examples/reasoning_function_calls + """ + + # Index the tools by name so a model-supplied call name can be looked up. .get() + # returns None for an unknown name, handled explicitly below. + tools_by_name = {tool.name: tool for tool in tools} + intermediate_messages = [] + tool_calls: list[ToolCall] = [] + + for response_item in response.output: + if response_item.type == "function_call": + tool_output, tool_call = _execute_function_call( + response_item, tools_by_name, user + ) + tool_calls.append(tool_call) + intermediate_messages.append( + { + "type": "function_call_output", + "call_id": response_item.call_id, + "output": tool_output, + } + ) + elif response_item.type == "reasoning": + logger.info(f"Reasoning step: {response_item.summary}") + return intermediate_messages, tool_calls + + +def _execute_function_call( + response_item, tools_by_name: dict, user +) -> tuple[str, ToolCall]: + """Run the one tool the model asked for, on any of its three outcomes. + + Returns a pair because the two results have different destinations: the string is + fed back to the model as the function_call_output, while the ToolCall is kept for + the eval and never reaches the model. + + They are not derivable from each other, which is the trap in collapsing this to a + single return. On OK the two carry the same text, and on UNREGISTERED they do too — + but on FAILED the model gets a message naming the tool it called, while + ToolCall.error holds the bare exception. Deriving one from the other would quietly + change what the model sees after a tool failure. + """ + target_tool = tools_by_name.get(response_item.name) + # Parsed below; stays None if the model's argument JSON can't be parsed, + # so a FAILED record still reports whatever we managed to read. + arguments = None + + if target_tool is None: + msg = f"ERROR - No tool registered for function call: {response_item.name}" + logger.error(msg) + return msg, ToolCall( + name=response_item.name, + status=ToolCallStatus.UNREGISTERED, + error=msg, + ) + + try: + arguments = json.loads(response_item.arguments) + logger.info( + f"Invoking tool: {response_item.name} with arguments: {arguments}" + ) + tool_output = target_tool.run(user=user, **arguments) + logger.info(f"Tool {response_item.name} completed successfully") + return tool_output, ToolCall( + name=response_item.name, + status=ToolCallStatus.OK, + arguments=arguments, + output=tool_output, + ) + except Exception as e: + msg = f"Error executing function call: {response_item.name}: {e}" + logger.error(msg, exc_info=True) + return msg, ToolCall( + name=response_item.name, + status=ToolCallStatus.FAILED, + arguments=arguments, + error=str(e), + ) diff --git a/server/api/views/assistant/assistant_prompts.py b/server/api/views/assistant/assistant_prompts.py index 44bf9b9b..dde89eb8 100644 --- a/server/api/views/assistant/assistant_prompts.py +++ b/server/api/views/assistant/assistant_prompts.py @@ -1,3 +1,23 @@ +# TODO: rewrite the citation template below (RESPONSE FORMAT item 4) so the braces are not +# emitted literally. `[Name {name}, Page {page_number}]` is read by the model as required +# output *syntax* rather than as placeholders: the 20260807 eval returned +# [Pharmacological Treatment of Bipolar Depression: ... Options? {Pharmacological +# Treatment of Bipolar Depression: ... Options?}, Page 2] +# — the name filled in AND the braces kept, duplicating the title. Also observed: +# "Page: 3" (stray colon), "Page 4, Chunk 32" (extra field), "various pages", +# "multiple pages including 1-5". Show a filled-in example instead of a brace template, +# e.g. `[Name advancespharmaco.pdf, Page 9]`, and state that exactly one page number is +# cited per reference. +# +# This is one of two separable citation defects; the other is search_tool.py handing the +# model a UUID alongside the name (see the TODO there). Neither is cosmetic — citations +# are unparseable until both land, which blocks citation accuracy, the "cheapest real +# signal" the scoring TODO in eval_assistant.py is built on. +# +# Note both known importers pass this string through verbatim — assistant_services.py +# hands it to the API as `instructions`, eval_assistant.py imports it for a planned +# sidecar and does not use it — so no .format() reads the braces. They are inert to +# Python; the only thing interpreting them is the model. INSTRUCTIONS = """ You are an AI assistant that helps users find and understand information about bipolar disorder from your internal library of bipolar disorder research sources using semantic search. diff --git a/server/api/views/assistant/assistant_services.py b/server/api/views/assistant/assistant_services.py index ac339b9f..f141887c 100644 --- a/server/api/views/assistant/assistant_services.py +++ b/server/api/views/assistant/assistant_services.py @@ -3,65 +3,37 @@ from openai import OpenAI -from .assistant_prompts import INSTRUCTIONS -from .tool_services import ( - SEARCH_TOOLS_SCHEMA, - make_search_tool_mapping, - handle_tool_calls_with_reasoning, -) +from api.views.assistant.assistant_prompts import INSTRUCTIONS +from api.views.assistant.tool_services import TOOLS +from api.views.assistant.assistant_types import AssistantResult +from api.views.assistant.agentic_loop import handle_tool_calls_with_reasoning logger = logging.getLogger(__name__) +# Module-level so eval_assistant.py can import it and label its CSV with the model that actually ran +MODEL_NAME = "gpt-5-nano" + def run_assistant( - message: str, user, + message: str, previous_response_id: str | None = None, -) -> tuple[str, str]: - """Wire together the OpenAI client, retrieval, and the agentic reasoning loop. - - Parameters - ---------- - message : str - The user's input message. - user : User - The Django user object used for document access control in search_documents. - previous_response_id : str | None - ID of a prior response for multi-turn conversation continuity. - - Returns - ------- - tuple[str, str] - (final_response_output_text, final_response_id) +) -> AssistantResult: + """ + TODO: Read server/api/views/assistant and fill in the docstring """ - # TODO: Track total duration, cost metrics, and tool_calls_made count - # and return them from run_assistant for use in eval_assistant.py CSV output - client = OpenAI(api_key=os.environ.get("OPENAI_API_KEY")) MODEL_DEFAULTS = { "instructions": INSTRUCTIONS, - "model": "gpt-5-nano", # 400,000 token context window - # A summary of the reasoning performed by the model. This can be useful for debugging and understanding the model's reasoning process. + "model": MODEL_NAME, + # TODO: Note how the summary can be used for debugging and understanding the model's reasoning process. "reasoning": {"effort": "low", "summary": None}, - "tools": SEARCH_TOOLS_SCHEMA, + "tools": [tool.schema() for tool in TOOLS], } - # TOOLS_SCHEMA tells the model what tools exist and what arguments to generate. - # tool_mapping wires those tool names to the Python functions that execute them. - # They are separate because the model generates arguments (schema concern) but - # cannot supply request-time values like user (mapping concern). - tool_mapping = make_search_tool_mapping(user) - - if not previous_response_id: - response = client.responses.create( - input=[ - {"type": "message", "role": "user", "content": str(message)} - ], - **MODEL_DEFAULTS, - ) - else: - response = client.responses.create( + if previous_response_id: + initial_response = client.responses.create( input=[ {"type": "message", "role": "user", "content": str(message)} ], @@ -69,4 +41,15 @@ def run_assistant( **MODEL_DEFAULTS, ) - return handle_tool_calls_with_reasoning(response, client, MODEL_DEFAULTS, tool_mapping) + # TODO: Explain the reason user is not part of the schema and is bound into each call at dispatch time + return handle_tool_calls_with_reasoning(initial_response, client, MODEL_DEFAULTS, TOOLS, user) + + initial_response = client.responses.create( + input=[ + {"type": "message", "role": "user", "content": str(message)} + ], + **MODEL_DEFAULTS, + ) + + # TODO: Explain the reason user is not part of the schema and is bound into each call at dispatch time + return handle_tool_calls_with_reasoning(initial_response, client, MODEL_DEFAULTS, TOOLS, user) diff --git a/server/api/views/assistant/assistant_types.py b/server/api/views/assistant/assistant_types.py new file mode 100644 index 00000000..57988011 --- /dev/null +++ b/server/api/views/assistant/assistant_types.py @@ -0,0 +1,146 @@ +"""The assistant's data types: what the model is offered, and what a run produced. + +Gathered here, apart from the code that uses them, so the shapes can be read without +the dispatch and loop logic wrapped around them. Two groups: + + - Tool — the definition side. One assistant tool: the schema the model sees and the + function we run. Instances are registered in tool_services.py's TOOLS list. + - ToolCallStatus / ToolCall / AssistantResult — the record side. Built by the agentic + loop as a run proceeds, and read by eval_assistant.py to fill the result CSV. + +This module imports nothing from the package, so it cannot take part in an import +cycle however many modules come to need a type from it. +""" + +from dataclasses import dataclass +from enum import Enum +from typing import Callable + + +@dataclass(frozen=True) +class Tool: + """One assistant tool: the schema the model sees (data) and the function we run + (behavior), bundled together under a single name. + + Bundling name/description/parameters/run in one object means each tool is + registered in exactly one place — the TOOLS list in tool_services.py — so the + schema sent to the model and the callable actually invoked can never drift + apart. Adding a tool is appending one Tool to TOOLS; nothing else changes. + + Behavior is stored as the `run` field (composition) rather than a method on a + subclass because our tools differ only in *which* function runs — same schema() + machinery, same fields, just a different callable. They are instances of one + concept, not distinct kinds of thing. + + Flip to `Tool(ABC)` + one subclass per tool (with `run` as a method) if a + tool ever needs more than a swapped-in function — specifically when it: + - carries per-type state/setup (a client, connection, cache, validated config); + - overrides more than run (e.g. a custom schema() shape, or extra methods like + validate_arguments / cost_estimate); + - needs a per-type run signature or an @abstractmethod-enforced contract so a + tool with no behavior fails at class-definition time, not at call time. + Until then the callable field is lighter and keeps registration drift-proof. + """ + + name: str + description: str + parameters: dict + # run(user, **arguments) -> str. Every tool takes the request `user` so the dispatch + # loop can call them uniformly; a tool that doesn't need it simply ignores it. + run: Callable + + def schema(self) -> dict: + # Flattened Responses-API shape: name/description/parameters at the top level. + # This is intentionally NOT the nested {"function": {...}} shape that the Chat + # Completions API (and services/tools/tools.py's create_tool_dict) uses. + return { + "type": "function", + "name": self.name, + "description": self.description, + "parameters": self.parameters, + } + + +class ToolCallStatus(str, Enum): + """The outcome of a single tool call. Three distinct states, deliberately not a + bool: FAILED (the tool matched but raised) and UNREGISTERED (the model asked for + a tool name we don't have) are opposite diagnoses — a code/data fault vs. the + model hallucinating a tool — and an eval on tool selection needs to tell them + apart. str-based so it serializes straight into CSV/JSON. + """ + + OK = "ok" + FAILED = "failed" # tool matched but raised + UNREGISTERED = "unregistered" # no tool registered for the model's requested name + + +@dataclass(frozen=True) +class ToolCall: + """A record of one tool call the model made — a complete unit for eval: + which tool, with what query (`arguments`), and what came back (`output`) or + broke (`error`). + + `output` and `error` are disjoint by status: `output` holds the retrieved + content on OK; `error` holds the detail when status is not OK. `arguments` + is the model-generated query (the primary tool-selection signal); it is None + only when the model's argument JSON could not be parsed. + """ + + name: str + status: ToolCallStatus + arguments: dict | None = None # the query the model generated (parsed) + output: str | None = None # the tool's result on success (retrieved content) + error: str | None = None # the failure detail when status is not OK + + +@dataclass(frozen=True) +class AssistantResult: + """What a full agentic run produced: the model's final text, the id of the final + response (for multi-turn continuity), and the ordered ToolCall records for every + tool invocation across all loop iterations. + + TODO: capture token usage and turn count — the other axis, alongside tool + selection, for comparing strategies. The design is settled but unbuilt: + - Six defaulted int fields: input_tokens, cached_tokens, output_tokens, + reasoning_tokens, total_tokens, turn_count. All three existing construction + sites pass keywords, so adding them is inert — this is exactly the property + the dataclass was chosen for over a widened tuple. + - Accumulate at the top of the while body in handle_tool_calls_with_reasoning + (agentic_loop.py), before invoke_functions_from_response. That counts the + initial response (created in run_assistant and passed in) and every + continuation exactly once, including the terminal turn before the return. + turn_count is then simply the number of responses.create calls the run made. + - Read through a helper that walks response.usage defensively rather than + type-checking only the leaf: reasoning_tokens lives at + usage.output_tokens_details.reasoning_tokens and cached_tokens at + usage.input_tokens_details.cached_tokens, so a None usage raises + AttributeError before any leaf check runs. The helper must also reject + non-ints — the loop tests build responses as bare MagicMocks, and MagicMock + implements __add__/__radd__, so mock values would accumulate silently into + the CSV rather than failing loudly. + - cached_tokens is not optional: every turn resends context via + previous_response_id, so a large share of input_tokens bills at the cached + rate. Without the split, a cost figure derived later from input_tokens + overstates spend and cannot be corrected from the CSV afterwards. + - Dollar cost stays out of this dataclass: it needs a price table keyed by + model *and* date, which goes stale and then lies. Derive it in pandas from + the token columns plus the CSV's model column. If a table is ever wanted, the + house pattern is PRICING_DOLLARS_PER_MILLION_TOKENS in + api/services/llm_services.py — which has no reasoning or cached tier yet. + + Known hole that work would widen: if client.responses.create raises mid-loop the + exception propagates out and every ToolCall collected so far is lost with it — + the eval row reads tool_call_count 0 despite real calls having run, and would + likewise read total_tokens 0 despite tokens having been billed. Closing it means + deciding what this dataclass describes: a successful run, or whatever actually + happened (a partial result returned with an error field, or carried on the + exception). + + Answer-quality scoring — ground truth, citation accuracy, LLM-as-judge — is a + separate layer above this one, not a field here; see the scoring TODO in + eval_assistant.py. + """ + + output_text: str + response_id: str + tool_calls: list[ToolCall] diff --git a/server/api/views/assistant/eval_assistant.py b/server/api/views/assistant/eval_assistant.py index 7584ae18..3c3e8c4e 100644 --- a/server/api/views/assistant/eval_assistant.py +++ b/server/api/views/assistant/eval_assistant.py @@ -1,33 +1,35 @@ -#!/usr/bin/env -S uv run --script -# /// script -# requires-python = "==3.11.11" -# dependencies = [ -# "pandas==2.2.3", -# "openai", -# "django", -# ] -# /// - -# uv script (or plain Python) to generate results to CSV, run from the terminal -# Run from inside the container (working dir is /usr/src/server): +# Generates eval results to CSV. Run from inside the container: # docker compose exec backend python api/views/assistant/eval_assistant.py -# - +# +# Needs OPENAI_API_KEY (from config/env/dev.env), a superuser, and embedded +# documents for that user. Writes to results/ next to this file, which the +# ./server bind mount surfaces on the host. +# +# This is NOT a standalone script and cannot be run with `uv run --script` or from +# the host: django.setup() below loads INSTALLED_APPS, pulling in psycopg2, pgvector, +# DRF, djoser and sentence_transformers — the backend image's full requirements.txt. +# A PEP 723 dependency header would have to duplicate that list to stay correct, and +# the host cannot resolve SQL_HOST=db off the docker network anyway. It previously +# carried such a header declaring only pandas/openai/django; that has been removed +# rather than repaired. import os import sys +import csv +import json import logging import datetime +from dataclasses import asdict +from time import perf_counter from concurrent.futures import ThreadPoolExecutor, as_completed -# Django setup must come before any imports that touch the ORM -# NOTE: from api/views/assistant/, "../../../../" resolves four levels up to -# /usr/src (not /usr/src/server, where balancer_backend lives). So this insert -# alone does not put the settings package on sys.path — running the script -# relies on the container already having /usr/src/server on PYTHONPATH. Sanity- -# check this the first time the eval is run for real; the path depth may need -# adjusting (e.g. "../../../"). -sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../../../../"))) +# Django setup must come before any imports that touch the ORM. +# Three levels up from api/views/assistant/ is /usr/src/server, where the +# balancer_backend settings package lives. This insert is doing real work: running +# a script file puts the *script's* directory on sys.path[0], not the working +# directory, and the image sets no PYTHONPATH — so without it django.setup() below +# raises ModuleNotFoundError on balancer_backend.settings. +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../../../"))) os.environ.setdefault("DJANGO_SETTINGS_MODULE", "balancer_backend.settings") import django @@ -35,20 +37,88 @@ from django.contrib.auth import get_user_model # noqa: E402 -from api.views.assistant.assistant_services import run_assistant # noqa: E402 +from api.views.assistant.assistant_services import run_assistant, MODEL_NAME # noqa: E402 +from api.views.assistant.assistant_types import ToolCallStatus +# Imported to warm the embedding model in main() before the worker pool starts — +# see the call site for why this process needs it and the web path does not. +from api.services.sentencetTransformer_model import TransformerModel # noqa: E402 +# Write INSTRUCTIONS to a sidecar file alongside the CSV in main(), named +# results/{branch}-{timestamp}.prompt.txt so the pairing cannot come apart: +# f.write(f"branch: {branch}\nmodel: {MODEL_NAME}\n\n{INSTRUCTIONS}") +# Two alternatives were considered and rejected: a full-text CSV column repeats +# ~2.1KB of multi-line prose in every row and buries a cross-branch CSV diff in +# prompt noise; logging it at run time leaves nothing behind in results/, which is +# exactly the failure this is meant to prevent (a CSV whose prompt is unrecoverable +# months later). Add an instructions_hash column *as well* only if runs are ever +# concatenated into one DataFrame, where a groupby-able key beats diffing sidecars. +# Until that lands, INSTRUCTIONS is imported but deliberately unused. +from api.views.assistant.assistant_prompts import INSTRUCTIONS logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s") logger = logging.getLogger(__name__) -# Read model and INSTRUCTIONS from the source file or add a lightweight config endpoint to the backend +# Model and INSTRUCTIONS both come from their source of truth rather than being +# restated here: MODEL_NAME from assistant_services.py (imported above, and used for +# the CSV's model column), INSTRUCTIONS from assistant_prompts.py (see sidecar TODO). + +# Add a scoring layer. This is the biggest remaining gap, and it needs a design +# pass rather than a patch. As it stands this file is a *generation* harness, not an +# eval: QUESTIONS below carries no ground truth, so the CSV records what the +# assistant said and — since the tool-call columns landed — which tools it chose, but +# nothing about whether the answer was right. Open questions for that pass: +# - Ground truth per question: expected medications/claims, expected source +# documents (which citations the answer should rest on), or both. +# - Grading method: deterministic assertions (does the answer cite doc X, name drug +# Y) vs LLM-as-judge for faithfulness. Likely both — assertions for retrieval +# correctness, judge for answer quality. +# - Citation accuracy is the cheapest real signal available: INSTRUCTIONS mandates +# the [Name {name}, Page {page_number}] format, so citations can be parsed out of +# response_output_text and checked against what search_documents actually +# returned — already captured in the tool_calls_json column. That catches +# fabricated citations, the failure mode that matters most clinically. +# BLOCKED until two citation defects land, both observed in the 20260807 run and +# both queued at their own call sites: the model emits the template's braces +# literally (see the TODO above INSTRUCTIONS in assistant_prompts.py) and sometimes +# cites a UUID as the document name (see the TODO in search_tool.py). Until then +# the format the parser would target does not actually hold, so a parser built now +# would measure prompt drift rather than citation accuracy — fix them first, then +# re-run, then write the parser against what comes out. +# - Where scoring runs: as a separate pass over an already-written CSV, not inside +# run_one, so scoring can be revised and re-run without paying for generation +# again. -# Read model and INSTRUCTIONS from the source file -# INSTRUCTIONS is imported from assistant_prompts.py -# MODEL is read from assistant_services.py MODEL_DEFAULTS -# TODO: import a shared MODEL_NAME constant from assistant_services instead of hardcoding -MODEL = "gpt-5-nano" +# The CSV's columns, in order. This is the single source of truth for column order: +# csv.DictWriter emits keys in this order regardless of the order the row dicts in +# run_one happen to list them, so the two row literals no longer have to be kept in +# lockstep (they used to, because as_completed returns rows nondeterministically and +# a DataFrame took its column order from whichever row landed first). DictWriter also +# raises on a key it doesn't know, so adding a column to a row literal and forgetting +# it here fails loudly instead of silently dropping the column. +FIELDNAMES = [ + "branch", + "model", + "question", + "response_output_text", + "response_id", + "tools_called", + "tool_call_count", + "tool_error_count", + "tool_calls_json", + "duration_s", + "error", +] # Set of representative questions to evaluate the assistant +# +# Two of these came back as corpus-gap disclaimers ("I can't find this in my +# sources") in the clean 20260807 run — lithium/kidney and valproate-vs-lithium — and both +# need confirming before anyone concludes the corpus is missing that content. Lithium/ +# kidney gave the *same* disclaimer on 20260804, when retrieval had in fact crashed +# ('TransformerModel' object has no attribute 'model'), so that bug misattributed itself +# to the data; the disclaimer is not evidence of a gap on its own. Retrieval succeeded +# this time, so read tool_calls_json for those two rows: ~8.8KB of retrieved lithium +# content sitting behind a can't-find answer is an answer-quality finding (the model not +# using what it was handed), not a reason to add documents. QUESTIONS = [ "What medications are recommended for bipolar depression?", "What are the risks of lithium for patients with kidney disease?", @@ -90,22 +160,58 @@ def run_one(question: str, user, branch: str) -> dict: per request — adds overhead to every web request for no benefit - Cleaner call site in eval_assistant.py but wrong trade-off given WSGI """ + # Time the full run_assistant call here rather than inside it: run_one already + # owns the whole call, so wall-clock duration needs no plumbing through the + # production code path (see AssistantResult — duration is not carried). + start = perf_counter() try: - response_text, response_id = run_assistant(message=question, user=user) + result = run_assistant(message=question, user=user) + duration_s = perf_counter() - start + tool_error_count = sum( + 1 for c in result.tool_calls if c.status is not ToolCallStatus.OK + ) return { "branch": branch, - "model": MODEL, + "model": MODEL_NAME, "question": question, - "response_output_text": response_text, + "response_output_text": result.output_text, + "response_id": result.response_id, + # Flat summaries for at-a-glance scanning; the swallowed-failure hole this + # closes shows up as tool_error_count > 0 while error is None. + "tools_called": "|".join(c.name for c in result.tool_calls), + "tool_call_count": len(result.tool_calls), + "tool_error_count": tool_error_count, + # Full per-call detail — status, the model's arguments (query), output/error — + # for analysis that the flat columns can't hold. + # + # TODO: this cell is the CSV's bulk — ToolCall.output holds the tool's entire + # return, so one search_documents call embeds a full retrieved chunk set + # (~8.8KB observed) into a single field, and the 5-row 20260804 file came to + # 79KB. If it needs trimming, truncate `output` *here*, in this serializer, + # not in ToolCall: the loop must keep the full text because it is what gets + # fed back to the model, and truncating upstream would change behavior rather + # than just the artifact. Weigh it against the open questions above, both of + # which are answered by reading this column — a truncation that drops the + # retrieved content also destroys the evidence for citation accuracy and for + # the disclaimer check on QUESTIONS. + "tool_calls_json": json.dumps([asdict(c) for c in result.tool_calls]), + "duration_s": duration_s, "error": None, } except Exception as e: + duration_s = perf_counter() - start logger.error(f"Error evaluating question '{question}': {e}") return { "branch": branch, - "model": MODEL, + "model": MODEL_NAME, "question": question, "response_output_text": None, + "response_id": None, + "tools_called": "", + "tool_call_count": 0, + "tool_error_count": 0, + "tool_calls_json": None, + "duration_s": duration_s, "error": str(e), } @@ -118,11 +224,63 @@ def main(): if not user: raise RuntimeError("No superuser found. Create one with manage.py createsuperuser.") - logger.info(f"Starting evaluation: branch={branch}, model={MODEL}, questions={len(QUESTIONS)}") + logger.info(f"Starting evaluation: branch={branch}, model={MODEL_NAME}, questions={len(QUESTIONS)}") + + # Load the embedding model before starting any workers. This line is load-bearing + # for two separate reasons. + # + # 1. It removes concurrency at the moment of loading, which is the only thing + # keeping this eval off a live race in TransformerModel (see the TODO below). + # get_instance() runs here on the main thread, before any worker exists, so + # the singleton is fully built by the time anything can contend for it. The + # web path is unaffected because api/apps.py preloads the model in ready() — + # but only when sys.argv[1:2] == ['runserver'], which running this file as a + # script does not match. So this process starts cold, and without this line + # all five workers reach a cold TransformerModel at once. That is not + # hypothetical: the first real eval run (results/521-research-agent-tools- + # 20260804T181410.csv) had 3 of 9 document searches fail with + # "'TransformerModel' object has no attribute 'model'". + # + # Those three failures were invisible in that CSV — every row still read + # tool_error_count 0 — because search_documents caught the exception and + # returned it as a string, which is indistinguishable from a retrieval that + # worked. That concealment is fixed separately (search_tool.py now lets it + # raise, so the loop records ToolCallStatus.FAILED), and the two fixes are + # complements rather than alternatives: this warm-up removes the trigger, + # search_tool.py removes the concealment. A future failure here would now be + # loud rather than silent, but it would still be a failure. + # + # 2. It fixes duration_s. Loading costs ~700ms of Hugging Face metadata requests + # plus weight loading. Left to the workers, that cost lands inside whichever + # run_assistant call triggers it, so that question's duration_s is inflated by + # work that has nothing to do with the question — and the column stops being + # comparable across rows. Warming here moves the cost outside every + # measurement, which matters because duration_s exists precisely to compare + # questions and branches. + # + # Fix the TransformerModel singleton itself + # (api/services/sentencetTransformer_model.py) — this warm-up only hides the + # defect at one call site. __new__ assigns cls._instance *before* setting + # .model, so any thread arriving in that ~700ms window gets a non-None but + # half-built object back. Two independent fixes: + # - Publish last: build the object fully, assign cls._instance only afterwards. + # This one is needed even single-threaded. If SentenceTransformer() raises, + # the bare object has already been assigned, leaving a permanently poisoned + # singleton that every later call returns without .model. api/apps.py:37 + # comments that "_instance stays None on failure, so the first actual request + # will attempt to load the model again" — which is not true today. + # - Guard the load with a threading.Lock (double-checked) so two cold callers + # don't both load it. + # Until that lands, anything that reaches get_instance() from a worker thread + # before this warm-up runs — a new concurrent entry point, or a reordering of + # main() — silently re-exposes the race. Note the fix is production code shared + # with the web path and uploadFile/views.py:129, not eval-only, so it may belong + # in its own commit rather than this branch. + TransformerModel.get_instance() # ThreadPoolExecutor runs questions concurrently — see run_one docstring # for trade-off discussion vs asyncio.gather + await run_assistant. - # max_workers=5 stays safely under OpenAI rate limits for gpt-5-nano. + # max_workers=5 stays safely under OpenAI rate limits for MODEL_NAME. results = [] with ThreadPoolExecutor(max_workers=5) as pool: futures = { @@ -132,18 +290,30 @@ def main(): for future in as_completed(futures): results.append(future.result()) - # Import pandas here, not at module top, so that importing this module (e.g. - # run_one from test_eval_assistant.py) does not require pandas. It is only - # needed for the CSV output below, when this script is run directly. - import pandas as pd - - df = pd.DataFrame(results) - + # TODO: decide whether this directory is tracked. It is currently neither committed + # nor in .gitignore, so every run leaves untracked files that show up in git status + # and are one `git clean` from gone. It already holds two CSVs that are worth keeping + # as before/after evidence — 20260804 (the TransformerModel race: 9 tool calls for 5 + # questions, 3 of them 118-char errors) and 20260807 (the clean baseline: 5 for 5, no + # retries) — and the eval's whole value is comparing runs across branches, which + # argues for committing them. Against: they carry full response text and retrieved + # document content, they grow ~80KB per run, and they are regenerable at the cost of + # an API call. Either way the ambiguous state should not persist. results_dir = os.path.join(os.path.dirname(__file__), "results") os.makedirs(results_dir, exist_ok=True) timestamp = datetime.datetime.utcnow().strftime("%Y%m%dT%H%M%S") output_path = os.path.join(results_dir, f"{branch}-{timestamp}.csv") - df.to_csv(output_path, index=False) + + # stdlib csv rather than pandas: this is one write of a handful of dict rows, and + # DictWriter quotes the embedded commas and newlines in response_output_text and + # tool_calls_json correctly. pandas was never in the backend image's + # requirements.txt, so the old pd.DataFrame(...).to_csv() would have raised + # ModuleNotFoundError here — after every question had already been generated and + # billed. newline="" is required on the file object; csv does its own line endings. + with open(output_path, "w", newline="") as f: + writer = csv.DictWriter(f, fieldnames=FIELDNAMES) + writer.writeheader() + writer.writerows(results) logger.info(f"Results saved to {output_path}") diff --git a/server/api/views/assistant/search_tool.py b/server/api/views/assistant/search_tool.py new file mode 100644 index 00000000..970473a3 --- /dev/null +++ b/server/api/views/assistant/search_tool.py @@ -0,0 +1,67 @@ +from api.services.embedding_services import get_closest_embeddings +from api.services.conversions_services import convert_uuids + + +def search_documents(query: str, user) -> str: + """ + Search through user's uploaded documents using semantic similarity. + + This function performs vector similarity search against the user's document corpus + and returns formatted results with context information for the LLM to use. + + Parameters + ---------- + query : str + The search query string + user : User + The authenticated user whose documents to search + + Returns + ------- + str + Formatted search results containing document excerpts with metadata, or a + message saying nothing matched. Matching nothing is a legitimate outcome, + not a failure, so it returns normally and the call is recorded as OK. + + Raises + ------ + Exception + If the embedding search fails. Deliberately not caught here. + invoke_functions_from_response (agentic_loop.py) already catches it, records + the call as ToolCallStatus.FAILED with the error, and still feeds the message + back to the model so it can retry or say it could not retrieve anything — + so letting it propagate loses nothing the model was getting before, and the + failure becomes visible to the eval. + + This used to catch everything and return the error as its result string. + A returned string is indistinguishable from a successful retrieval, so the + call was recorded OK, tool_error_count stayed 0, and ToolCallStatus.FAILED + was unreachable for the only tool the model actually calls. + """ + + embeddings_results = get_closest_embeddings( + user=user, message_data=query.strip() + ) + embeddings_results = convert_uuids(embeddings_results) + + if not embeddings_results: + return "No relevant documents found for your query. Please try different search terms or upload documents first." + + # Format results with clear structure and metadata + # + # Drop `File: {obj['file_id']}` from this line — one of the two citation defects + # blocking any citation-accuracy scoring. This hands the model both a UUID and a human + # document name and does not say which is the citable one, so it sometimes picks the + # UUID: the 20260807 eval produced + # "[Name 4cdd4a7e-0c26-4b80-b685-e731e8670725], Page 3, Chunk 12". + # The model never needs file_id — nothing downstream resolves it and INSTRUCTIONS asks + # for {name} — so removing the field removes the ambiguity outright. The sibling defect + # is in the citation template itself; see the TODO above INSTRUCTIONS in + # assistant_prompts.py. Both must land before citation accuracy is parseable, which is + # what the scoring TODO in eval_assistant.py rests on. + prompt_texts = [ + f"[Document {i + 1} - File: {obj['file_id']}, Name: {obj['name']}, Page: {obj['page_number']}, Chunk: {obj['chunk_number']}, Similarity: {1 - obj['distance']:.3f}]\n{obj['text']}\n[End Document {i + 1}]" + for i, obj in enumerate(embeddings_results) + ] + + return "\n\n".join(prompt_texts) diff --git a/server/api/views/assistant/test_assistant_services.py b/server/api/views/assistant/test_assistant_services.py index 9d911920..feae114d 100644 --- a/server/api/views/assistant/test_assistant_services.py +++ b/server/api/views/assistant/test_assistant_services.py @@ -1,13 +1,38 @@ # Tests for run_assistant (assistant_services.py): the orchestrator that wires the -# OpenAI client, the search tool mapping, and the agentic loop together. +# OpenAI client, the tool schemas, and the agentic loop together. # -# The OpenAI client and handle_tool_calls_with_reasoning are mocked, so these -# tests cover only logic run_assistant owns: how it builds the user input message, -# its decision to include vs. omit previous_response_id, and that it binds the -# request user into the search tool. No live OpenAI calls and no database. +# The OpenAI client and handle_tool_calls_with_reasoning are mocked, so what remains +# to test is the one decision run_assistant actually makes: whether to include +# previous_response_id in the call at all. Everything else it does is forwarding — a +# hardcoded message dict, TOOLS and user passed straight through to the loop — and +# the tests that asserted those forwards were removed as glue. The only bugs they +# could catch were renames and reorderings, and one of them (`args[3] is TOOLS`) was +# coupled to positional argument order, so it would have gone red on a harmless +# switch to keyword arguments. +# +# Coverage that leaves open, deliberately noted rather than silently dropped: +# - The user -> run_assistant -> loop leg is no longer asserted. It is a bare +# positional forward with no decision in it, and the legs on either side are +# still covered (test_invoke_calls_tool_and_returns_output asserts the loop +# dispatches run(user=user, ...); test_search_tool_run_forwards_query_and_user +# asserts the handoff into retrieval). +# - Nothing asserts that MODEL_DEFAULTS["tools"] == [tool.schema() for tool in +# TOOLS] reaches the model. That comprehension is a real transformation and is +# genuinely untested — but it is not what the deleted test checked either. from unittest.mock import MagicMock, patch +import pytest + +from api.views.assistant.assistant_types import AssistantResult + +# Distinguishes "the kwarg was omitted" from "the kwarg was passed as None", which is +# the entire point of the test below. It cannot use dict.get()'s usual None default: +# a regression that sent previous_response_id=None explicitly would then be +# indistinguishable from correctly omitting the key, which is exactly the bug the +# omit-branch exists to prevent. +ABSENT = object() + def _make_terminal_response(output_text="Final answer.", response_id="resp-1"): response = MagicMock() @@ -16,76 +41,44 @@ def _make_terminal_response(output_text="Final answer.", response_id="resp-1"): response.id = response_id return response -@patch("api.views.assistant.assistant_services.handle_tool_calls_with_reasoning") -@patch("api.views.assistant.assistant_services.OpenAI") -def test_run_assistant_sends_message_as_user_input(mock_openai_cls, mock_handle): - mock_client = MagicMock() - mock_openai_cls.return_value = mock_client - mock_client.responses.create.return_value = _make_terminal_response() - mock_handle.return_value = ("answer", "resp-1") - - from api.views.assistant.assistant_services import run_assistant - - run_assistant(message="Tell me about valproate.", user=MagicMock()) - - call_kwargs = mock_client.responses.create.call_args - input_messages = call_kwargs.kwargs.get("input") or call_kwargs.args[0] - assert any( - item.get("role") == "user" and "valproate" in item.get("content", "") - for item in input_messages - ) - - -@patch("api.views.assistant.assistant_services.handle_tool_calls_with_reasoning") -@patch("api.views.assistant.assistant_services.OpenAI") -def test_run_assistant_passes_previous_response_id(mock_openai_cls, mock_handle): - mock_client = MagicMock() - mock_openai_cls.return_value = mock_client - mock_client.responses.create.return_value = _make_terminal_response() - mock_handle.return_value = ("answer", "resp-2") - - from api.views.assistant.assistant_services import run_assistant - - run_assistant(message="More info.", user=MagicMock(), previous_response_id="resp-1") - call_kwargs = mock_client.responses.create.call_args.kwargs - assert call_kwargs.get("previous_response_id") == "resp-1" +def _make_result(output_text="answer", response_id="resp-1"): + return AssistantResult(output_text=output_text, response_id=response_id, tool_calls=[]) +@pytest.mark.parametrize( + "previous_response_id, expected", + [ + pytest.param("resp-1", "resp-1", id="forwarded-when-provided"), + pytest.param(None, ABSENT, id="omitted-entirely-when-none"), + ], +) @patch("api.views.assistant.assistant_services.handle_tool_calls_with_reasoning") @patch("api.views.assistant.assistant_services.OpenAI") -def test_run_assistant_omits_previous_response_id_when_none(mock_openai_cls, mock_handle): +def test_run_assistant_includes_previous_response_id_only_when_set( + mock_openai_cls, mock_handle, previous_response_id, expected +): + """run_assistant's `if not previous_response_id` branch, both ways. + + Parametrized rather than written twice: the two cases are the same call with one + input changed, and previously duplicated four lines of client/loop mock setup to + assert two halves of one decision. + + Asserting on call_args is the only way to see this decision — omitting a kwarg + has no return-value footprint, since both branches return the same loop result. + """ mock_client = MagicMock() mock_openai_cls.return_value = mock_client mock_client.responses.create.return_value = _make_terminal_response() - mock_handle.return_value = ("answer", "resp-1") + mock_handle.return_value = _make_result() from api.views.assistant.assistant_services import run_assistant - run_assistant(message="First message.", user=MagicMock(), previous_response_id=None) + run_assistant( + message="Tell me about valproate.", + user=MagicMock(), + previous_response_id=previous_response_id, + ) call_kwargs = mock_client.responses.create.call_args.kwargs - assert "previous_response_id" not in call_kwargs - - -@patch("api.views.assistant.tool_services.search_documents") -@patch("api.views.assistant.assistant_services.handle_tool_calls_with_reasoning") -@patch("api.views.assistant.assistant_services.OpenAI") -def test_run_assistant_binds_user_to_search_documents(mock_openai_cls, mock_handle, mock_search): - mock_client = MagicMock() - mock_openai_cls.return_value = mock_client - mock_client.responses.create.return_value = _make_terminal_response() - mock_handle.return_value = ("answer", "resp-1") - - from api.views.assistant.assistant_services import run_assistant - - user = MagicMock() - run_assistant(message="query", user=user) - - # Extract the tool_mapping passed to handle_tool_calls_with_reasoning - tool_mapping = mock_handle.call_args.kwargs.get("tool_mapping") or mock_handle.call_args.args[3] - bound_search = tool_mapping["search_documents"] - - # Calling the bound function should forward user to search_documents - bound_search(query="test query") - mock_search.assert_called_once_with("test query", user) + assert call_kwargs.get("previous_response_id", ABSENT) == expected diff --git a/server/api/views/assistant/test_eval_assistant.py b/server/api/views/assistant/test_eval_assistant.py index 5853d340..dd8fe64a 100644 --- a/server/api/views/assistant/test_eval_assistant.py +++ b/server/api/views/assistant/test_eval_assistant.py @@ -1,20 +1,93 @@ # Tests for run_one (eval_assistant.py): the helper that runs the assistant for a # single eval question and shapes the outcome into a result row. # -# run_assistant is mocked, so this covers the logic run_one owns — specifically -# that a raising question is captured as an error row (error text recorded, -# response left None) instead of aborting the whole eval batch. +# run_assistant is mocked, so this covers the logic run_one owns — the try/except +# that turns a raising question into an error row instead of aborting the batch, the +# tool columns derived from AssistantResult.tool_calls, and the invariant that both +# paths emit every CSV column. from unittest.mock import MagicMock, patch -from api.views.assistant.eval_assistant import run_one +import pytest + +from api.views.assistant.assistant_types import AssistantResult, ToolCall, ToolCallStatus +from api.views.assistant.eval_assistant import FIELDNAMES, run_one # TODO: add coverage for main()'s CSV output. -@patch("api.views.assistant.eval_assistant.run_assistant", side_effect=Exception("boom")) +# The two run_assistant outcomes, as patch() kwargs so the same pair can drive both +# the per-path tests and the shared column invariant without restating either setup. +_SUCCEEDS = { + "return_value": AssistantResult( + output_text="answer", + response_id="resp-1", + tool_calls=[ + ToolCall( + name="search_documents", + status=ToolCallStatus.OK, + arguments={"query": "lithium"}, + output="docs", + ), + ToolCall( + name="ask_database", + status=ToolCallStatus.FAILED, + arguments={"query": "SELECT"}, + error="bad sql", + ), + ], + ) +} +_RAISES = {"side_effect": Exception("boom")} + + +@pytest.mark.parametrize( + "run_assistant_behavior", + [pytest.param(_SUCCEEDS, id="success-row"), pytest.param(_RAISES, id="error-row")], +) +def test_run_one_row_carries_every_csv_column(run_assistant_behavior): + """One invariant over both code paths, so it is parametrized rather than restated. + + This is the only guard on it. csv.DictWriter raises on an *extra* key + (extrasaction="raise"), which is the direction the FIELDNAMES comment describes — + but a *missing* key is silently filled with restval (""). So a column added to + one row literal in run_one and forgotten in the other reaches the CSV as an empty + cell rather than an error, which is precisely the ragged-row failure FIELDNAMES + was introduced to prevent. + """ + with patch( + "api.views.assistant.eval_assistant.run_assistant", **run_assistant_behavior + ): + row = run_one("query", user=MagicMock(), branch="feature") + + assert set(row) == set(FIELDNAMES) + + +@patch("api.views.assistant.eval_assistant.run_assistant", **_RAISES) def test_run_one_captures_error(mock_run_assistant): row = run_one("query", user=MagicMock(), branch="feature") assert row["branch"] == "feature" assert row["response_output_text"] is None assert "boom" in row["error"] + # The error row *defaults* the tool columns rather than omitting them, and still + # records time-to-failure. That the columns are present at all is asserted above; + # these are their values. + assert row["tools_called"] == "" + assert row["tool_call_count"] == 0 + assert row["tool_error_count"] == 0 + assert row["tool_calls_json"] is None + assert row["duration_s"] > 0 + + +@patch("api.views.assistant.eval_assistant.run_assistant", **_SUCCEEDS) +def test_run_one_records_tool_calls(mock_run_assistant): + row = run_one("query", user=MagicMock(), branch="feature") + + assert row["tools_called"] == "search_documents|ask_database" + assert row["tool_call_count"] == 2 + # tool_error_count counts every non-OK status, so one FAILED call stays visible + # even though the run itself did not raise and `error` is None. That combination + # is the swallowed-failure hole this column exists to close — a run that reads + # clean at the row level while a retrieval underneath it broke. + assert row["tool_error_count"] == 1 + assert row["error"] is None diff --git a/server/api/views/assistant/test_tool_services.py b/server/api/views/assistant/test_tool_services.py index 86e57eed..559c58e9 100644 --- a/server/api/views/assistant/test_tool_services.py +++ b/server/api/views/assistant/test_tool_services.py @@ -1,67 +1,65 @@ -# Tests for tool_services.py: the retrieval tooling and the agentic reasoning loop. +# Tests for the assistant's tools and the agentic reasoning loop. # -# Covers the logic this module owns, with mocked tools (no DB, no OpenAI): -# - make_search_tool_mapping: the closure that binds the request user to -# search_documents, including per-call user independence. -# - invoke_functions_from_response: dispatching the model's function calls — -# the call/no-call branch, output shaping, and the unregistered-tool and -# tool-raises error paths. -# - handle_tool_calls_with_reasoning: the while-loop that keeps calling the -# model until it stops emitting tool calls, including loop continuity via +# Covers the logic these modules own, with mocked collaborators (no DB, no OpenAI): +# - Tool instances: SEARCH_TOOL.run adapts the loop's uniform (user, **arguments) +# call into search_documents' own (query, user) signature; schema() emits the +# flattened Responses-API shape rather than the nested Chat-Completions one. +# - search_documents' error/empty contract: failing raises, matching nothing does not. +# - invoke_functions_from_response: dispatching the model's function calls — the +# call/no-call branch, output shaping, and both error outcomes. Tools are indexed +# by name and invoked as tool.run(user, **arguments). +# - handle_tool_calls_with_reasoning: the while-loop that keeps calling the model +# until it stops emitting tool calls, including loop continuity via # previous_response_id. +# +# Two tests were removed as glue. test_ask_database_tool_run_ignores_user asserted a +# single-argument forward whose wrong version raises TypeError on first call, and +# test_tools_registry_contains_both_tools restated the TOOLS list literal — a +# change-detector that made "adding a tool is appending one Tool to TOOLS; nothing +# else changes" (tool_services.py) false, since the intended way to extend the code +# was also the way to break the test. +# +# Where two tests were the same test with one input changed, they are now one +# pytest.mark.parametrize case table. Tests whose assertions differ in kind are left +# separate on purpose: folding those together needs a column per optional assertion +# and a body full of conditionals, which costs more clarity than the duplication did. import json -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch -# TODO: add coverage for search_documents itself (formatting of embeddings -# results, the empty-results message, and the exception path). No DB needed: -# search_documents only calls get_closest_embeddings and convert_uuids, so -# mocking those two (like the rest of the suite mocks collaborators) covers all -# three paths as fast, DB-free unit tests. +import pytest -from api.views.assistant.tool_services import ( +# TODO: add coverage for search_documents' formatting of embeddings results — the +# [Document N - File: ..., Similarity: ...] shape and the multi-result join. No DB +# needed: search_documents only calls get_closest_embeddings and convert_uuids, so +# mocking those two (like the rest of the suite mocks collaborators) is enough. The +# empty-results and exception paths are covered below. +# +# Sequence this after the file_id removal queued in search_tool.py, not before: that +# format string is about to lose its `File: {file_id}` field, so a test written against +# today's shape would be red on arrival. Pinning the format is worth doing either way — +# the field is there because the model reads it, and a change-detector objection doesn't +# apply to output whose exact text is the contract with the model. + +from api.views.assistant.assistant_types import ( + AssistantResult, + Tool, + ToolCall, + ToolCallStatus, +) +from api.views.assistant.agentic_loop import ( invoke_functions_from_response, handle_tool_calls_with_reasoning, - make_search_tool_mapping, ) +from api.views.assistant.search_tool import search_documents +from api.views.assistant.tool_services import SEARCH_TOOL # --------------------------------------------------------------------------- -# make_search_tool_mapping tests -# --------------------------------------------------------------------------- - -@patch("api.views.assistant.tool_services.search_documents") -def test_make_search_tool_mapping_bound_fn_forwards_user(mock_search): - mock_search.return_value = "results" - user = MagicMock() - mapping = make_search_tool_mapping(user) - - mapping["search_documents"](query="lithium") - - mock_search.assert_called_once_with("lithium", user) - - -@patch("api.views.assistant.tool_services.search_documents") -def test_make_search_tool_mapping_different_users_are_independent(mock_search): - # Each call to make_search_tool_mapping should capture its own user, - # so two mappings created with different users do not share state. - user_a = MagicMock() - user_b = MagicMock() - mapping_a = make_search_tool_mapping(user_a) - mapping_b = make_search_tool_mapping(user_b) - - mapping_a["search_documents"](query="q") - mapping_b["search_documents"](query="q") - - # bound_search calls search_documents(query, user) positionally, so each - # recorded call is (args, kwargs) == (("q", user), {}). - calls = mock_search.call_args_list - assert calls[0] == (("q", user_a), {}) - assert calls[1] == (("q", user_b), {}) - - -# --------------------------------------------------------------------------- -# invoke_functions_from_response tests +# Response / tool builders +# +# Defined before the tests because pytest.mark.parametrize case tables are built at +# import time, so anything they construct must already exist. # --------------------------------------------------------------------------- def _make_function_call_item(name, arguments, call_id): @@ -86,133 +84,301 @@ def _make_response(output_items): return response -def test_invoke_returns_empty_list_when_no_function_calls(): - response = _make_response([_make_reasoning_item()]) - result = invoke_functions_from_response(response, tool_mapping={}) - assert result == [] +def _make_terminal_response(output_text, response_id): + """A response with no function calls — terminates the loop.""" + response = MagicMock() + response.output = [] + response.output_text = output_text + response.id = response_id + return response -def test_invoke_calls_tool_and_returns_output(): - mock_tool = MagicMock(return_value="search result") - item = _make_function_call_item("search_documents", {"query": "lithium"}, "call-1") - response = _make_response([item]) +def _make_tool_call_response(response_id, query="lithium"): + """A response with one function call — continues the loop.""" + response = MagicMock() + response.output = [_make_function_call_item("search_documents", {"query": query}, "call-loop")] + response.id = response_id + return response - result = invoke_functions_from_response( - response, tool_mapping={"search_documents": mock_tool} - ) - mock_tool.assert_called_once_with(query="lithium") - assert result == [ - {"type": "function_call_output", "call_id": "call-1", "output": "search result"} - ] +def _make_client(*responses): + """A client whose successive responses.create calls return `responses` in order. + side_effect rather than return_value on purpose: return_value would hand the same + terminal response back forever, so a loop that failed to terminate would hang or + silently pass. A list runs out, and the extra call raises StopIteration. + """ + client = MagicMock() + client.responses.create.side_effect = list(responses) + return client + + +def _fake_tool(name, run): + """A Tool whose run is a mock; description/parameters are irrelevant to dispatch.""" + return Tool(name=name, description="", parameters={}, run=run) + + +# --------------------------------------------------------------------------- +# Tool instances +# --------------------------------------------------------------------------- + +@patch("api.views.assistant.tool_services.search_documents") +def test_search_tool_run_forwards_query_and_user(mock_search): + """The adapter inverts the argument order, which is why this is worth asserting. + + The loop calls run(user=..., query=...); search_documents takes (query, user). + Getting the swap wrong searches with a User object as the query string and scopes + access control to a string — silent in both directions, and this is the leg where + document access control is actually enforced. + """ + mock_search.return_value = "results" + user = MagicMock() + + SEARCH_TOOL.run(user=user, query="lithium") + + mock_search.assert_called_once_with("lithium", user) + + +def test_tool_schema_is_flattened_shape(): + schema = SEARCH_TOOL.schema() + assert schema["type"] == "function" + assert schema["name"] == "search_documents" + assert "parameters" in schema + # The load-bearing assertion: this repo contains both tool-schema shapes, and + # services/tools/tools.py's create_tool_dict builds the nested Chat-Completions + # one. The Responses API needs the flattened form, so a copy-paste from there + # would be accepted by every other assertion here. + assert "function" not in schema -def test_invoke_returns_error_message_when_tool_not_registered(): - item = _make_function_call_item("unknown_tool", {"query": "x"}, "call-2") - response = _make_response([item]) - result = invoke_functions_from_response(response, tool_mapping={}) +# --------------------------------------------------------------------------- +# search_documents error/empty contract +# +# These two lock in the distinction the tool's status reporting depends on: a +# retrieval that *fails* must raise (so the loop records FAILED), while a retrieval +# that legitimately *matches nothing* must return normally (so it stays OK). Both +# used to return a string, which made the two indistinguishable downstream. +# --------------------------------------------------------------------------- + +@patch("api.views.assistant.search_tool.get_closest_embeddings") +def test_search_documents_raises_instead_of_returning_the_error(mock_get): + mock_get.side_effect = RuntimeError("embedding backend down") + + # Must propagate. Swallowing it here would report a failed retrieval as a + # successful tool call and leave ToolCallStatus.FAILED unreachable for this tool. + with pytest.raises(RuntimeError, match="embedding backend down"): + search_documents("lithium", user=MagicMock()) + + +@patch("api.views.assistant.search_tool.convert_uuids", return_value=[]) +@patch("api.views.assistant.search_tool.get_closest_embeddings", return_value=[]) +def test_search_documents_returns_message_when_nothing_matches(mock_get, mock_convert): + result = search_documents("lithium", user=MagicMock()) + + # No match is an outcome, not an error — returns normally so the call records OK. + assert "No relevant documents found" in result + + +@patch( + "api.views.assistant.search_tool.get_closest_embeddings", + side_effect=RuntimeError("embedding backend down"), +) +def test_failed_status_is_reachable_through_the_real_search_tool(mock_get): + """The two fixes composed: a real retrieval failure arrives at the eval as FAILED. + + Deliberately dispatches the *real* SEARCH_TOOL — only its embedding dependency is + mocked — rather than a fake tool that raises. A fake would exercise the identical + loop branch as test_invoke_records_the_two_error_outcomes below and prove nothing + extra; what is worth testing is that search_documents' decision not to swallow the + exception and the loop's decision to record FAILED actually meet, with the real + adapter between them. + """ + item = _make_function_call_item("search_documents", {"query": "lithium"}, "call-e2e") + + _, calls = invoke_functions_from_response( + _make_response([item]), tools=[SEARCH_TOOL], user=MagicMock() + ) + + assert calls[0].status is ToolCallStatus.FAILED + assert "embedding backend down" in calls[0].error + + +# --------------------------------------------------------------------------- +# invoke_functions_from_response tests +# --------------------------------------------------------------------------- - assert result[0]["call_id"] == "call-2" - assert "ERROR" in result[0]["output"] +def test_invoke_returns_empty_lists_when_no_function_calls(): + response = _make_response([_make_reasoning_item()]) + messages, calls = invoke_functions_from_response(response, tools=[], user=MagicMock()) + assert messages == [] + assert calls == [] -def test_invoke_returns_error_message_when_tool_raises(): - mock_tool = MagicMock(side_effect=Exception("tool exploded")) - item = _make_function_call_item("search_documents", {"query": "x"}, "call-3") +def test_invoke_calls_tool_and_returns_output(): + mock_run = MagicMock(return_value="search result") + tool = _fake_tool("search_documents", mock_run) + user = MagicMock() + item = _make_function_call_item("search_documents", {"query": "lithium"}, "call-1") response = _make_response([item]) - result = invoke_functions_from_response( - response, tool_mapping={"search_documents": mock_tool} + messages, calls = invoke_functions_from_response(response, tools=[tool], user=user) + + # The loop binds user at dispatch and forwards the model's arguments. + mock_run.assert_called_once_with(user=user, query="lithium") + # The OpenAI payload (unchanged shape) is the first return value. + assert messages == [ + {"type": "function_call_output", "call_id": "call-1", "output": "search result"} + ] + # The ToolCall record captures the outcome, the model's query, and the output. + assert calls == [ + ToolCall( + name="search_documents", + status=ToolCallStatus.OK, + arguments={"query": "lithium"}, + output="search result", + ) + ] + + +@pytest.mark.parametrize( + "tools, expected_output_fragment, expected_status, expected_error_fragment, expected_arguments", + [ + pytest.param( + [], + "ERROR - No tool registered", + ToolCallStatus.UNREGISTERED, + "No tool registered", + None, + id="model-named-a-tool-we-do-not-have", + ), + pytest.param( + [_fake_tool("search_documents", MagicMock(side_effect=Exception("tool exploded")))], + "Error executing function call", + ToolCallStatus.FAILED, + "tool exploded", + {"query": "x"}, + id="registered-tool-raised", + ), + ], +) +def test_invoke_records_the_two_error_outcomes( + tools, + expected_output_fragment, + expected_status, + expected_error_fragment, + expected_arguments, +): + """FAILED vs UNREGISTERED, parametrized to keep the contrast readable. + + These are opposite diagnoses — a code or data fault on our side vs. the model + hallucinating a tool name — which is why ToolCallStatus is a three-state enum and + not a bool, and why a tool-selection eval has to tell them apart. + + Reading them as one table also surfaces a difference neither test stated when they + were separate: `arguments` is parsed inside the registered branch, so an + unregistered call records None while a raising tool still reports the query the + model generated. + """ + item = _make_function_call_item("search_documents", {"query": "x"}, "call-err") + + messages, calls = invoke_functions_from_response( + _make_response([item]), tools=tools, user=MagicMock() ) - assert "Error executing function call" in result[0]["output"] + # Either way the model still gets a message back, so it can retry or say it could + # not retrieve anything — the loop does not abandon the turn. + assert messages[0]["call_id"] == "call-err" + assert expected_output_fragment in messages[0]["output"] + + assert calls[0].name == "search_documents" + assert calls[0].status is expected_status + assert expected_error_fragment in calls[0].error + assert calls[0].arguments == expected_arguments def test_invoke_handles_multiple_function_calls(): - mock_tool = MagicMock(return_value="result") + mock_run = MagicMock(return_value="result") + tool = _fake_tool("search_documents", mock_run) items = [ _make_function_call_item("search_documents", {"query": "q1"}, "call-4"), _make_function_call_item("search_documents", {"query": "q2"}, "call-5"), ] response = _make_response(items) - result = invoke_functions_from_response( - response, tool_mapping={"search_documents": mock_tool} - ) + messages, calls = invoke_functions_from_response(response, tools=[tool], user=MagicMock()) - assert len(result) == 2 - assert mock_tool.call_count == 2 + # Two calls in one response accumulate rather than overwrite — distinct from the + # cross-iteration accumulation covered in the loop test below. + assert [m["call_id"] for m in messages] == ["call-4", "call-5"] + assert [c.arguments for c in calls] == [{"query": "q1"}, {"query": "q2"}] + assert mock_run.call_count == 2 # --------------------------------------------------------------------------- # handle_tool_calls_with_reasoning tests # --------------------------------------------------------------------------- -def _make_terminal_response(output_text, response_id): - """A response with no function calls — terminates the loop.""" - response = MagicMock() - response.output = [] - response.output_text = output_text - response.id = response_id - return response - - -def _make_tool_call_response(response_id, query="lithium"): - """A response with one function call — continues the loop.""" - response = MagicMock() - response.output = [_make_function_call_item("search_documents", {"query": query}, "call-loop")] - response.id = response_id - return response - - def test_handle_terminates_immediately_when_no_tool_calls(): response = _make_terminal_response("Final answer.", "resp-1") - client = MagicMock() + client = _make_client() - text, resp_id = handle_tool_calls_with_reasoning( - response, client, model_defaults={}, tool_mapping={} + result = handle_tool_calls_with_reasoning( + response, client, model_defaults={}, tools=[], user=MagicMock() ) - assert text == "Final answer." - assert resp_id == "resp-1" + assert isinstance(result, AssistantResult) + assert result.output_text == "Final answer." + assert result.response_id == "resp-1" + assert result.tool_calls == [] client.responses.create.assert_not_called() -def test_handle_calls_tool_then_terminates(): - mock_search = MagicMock(return_value="doc content") - first_response = _make_tool_call_response("resp-1") - second_response = _make_terminal_response("Final answer.", "resp-2") - - client = MagicMock() - client.responses.create.return_value = second_response +@pytest.mark.parametrize( + "queries", + [ + pytest.param(["lithium"], id="one-tool-turn"), + pytest.param(["q1", "q2"], id="two-tool-turns"), + ], +) +def test_handle_loops_until_the_model_stops_calling_tools(queries): + """The loop at one and two tool-calling turns. + + Three tests collapsed into this table — they were the same scenario at different + turn counts, asserting one facet each (that a tool runs then the loop terminates, + that ToolCall records accumulate across iterations, that the follow-up call chains + off previous_response_id). Asserting all three at every turn count is strictly + more coverage than the originals: continuity was previously only checked on the + first follow-up, so a loop that re-sent resp-1 forever would have passed. + """ + mock_run = MagicMock(return_value="doc content") + tool = _fake_tool("search_documents", mock_run) + user = MagicMock() - text, resp_id = handle_tool_calls_with_reasoning( - first_response, - client, - model_defaults={}, - tool_mapping={"search_documents": mock_search}, + # One tool-calling response per query, then a terminal one that ends the loop. + tool_turns = [ + _make_tool_call_response(f"resp-{i + 1}", query=q) for i, q in enumerate(queries) + ] + terminal_id = f"resp-{len(queries) + 1}" + # The first response is the one run_assistant creates and passes in; only the rest + # come back from the client. + client = _make_client( + *tool_turns[1:], _make_terminal_response("Final answer.", terminal_id) ) - mock_search.assert_called_once_with(query="lithium") - assert text == "Final answer." - assert resp_id == "resp-2" - - -def test_handle_passes_previous_response_id_on_followup(): - mock_search = MagicMock(return_value="doc content") - first_response = _make_tool_call_response("resp-1") - second_response = _make_terminal_response("Done.", "resp-2") - - client = MagicMock() - client.responses.create.return_value = second_response - - handle_tool_calls_with_reasoning( - first_response, - client, - model_defaults={}, - tool_mapping={"search_documents": mock_search}, + result = handle_tool_calls_with_reasoning( + tool_turns[0], client, model_defaults={}, tools=[tool], user=user ) - call_kwargs = client.responses.create.call_args.kwargs - assert call_kwargs["previous_response_id"] == "resp-1" + # The tool ran once per turn, with user bound at each dispatch. + assert mock_run.call_args_list == [call(user=user, query=q) for q in queries] + # ToolCall records from every iteration accumulate into one flat list. + assert [c.arguments for c in result.tool_calls] == [{"query": q} for q in queries] + assert all(c.status is ToolCallStatus.OK for c in result.tool_calls) + # Loop continuity: each follow-up chains off the id of the response it answers, + # so the chain advances resp-1 -> resp-2 -> ... rather than repeating resp-1. + assert [ + c.kwargs["previous_response_id"] for c in client.responses.create.call_args_list + ] == [turn.id for turn in tool_turns] + # Terminating returns the *last* response's text and id, not the first. + assert result.output_text == "Final answer." + assert result.response_id == terminal_id diff --git a/server/api/views/assistant/tool_services.py b/server/api/views/assistant/tool_services.py index 0fb96cef..a4af2767 100644 --- a/server/api/views/assistant/tool_services.py +++ b/server/api/views/assistant/tool_services.py @@ -1,214 +1,109 @@ -import json -import logging -from typing import Callable - -from ...services.embedding_services import get_closest_embeddings -from ...services.conversions_services import convert_uuids - -logger = logging.getLogger(__name__) - -TOOL_DESCRIPTION = """ +from api.views.assistant.assistant_types import Tool +# Keep this as a bare-name import: rewriting SEARCH_TOOL.run to call +# search_tool.search_documents(...) would move the patch target and break the tests. +from api.views.assistant.search_tool import search_documents +# Reuse the existing ask_database implementation from services/tools rather than +# reimplementing it here — it already enforces the SELECT-only and ALLOWED_TABLES +# guards, and does no DB work at import time. +from api.services.tools.database import ask_database + + +SEARCH_TOOL = Tool( + name="search_documents", + description=""" Search the user's uploaded documents for information relevant to answering their question. Call this function when you need to find specific information from the user's documents to provide an accurate, citation-backed response. Always search before answering questions about document content. -""" - -TOOL_PROPERTY_DESCRIPTION = """ +""", + parameters={ + "type": "object", + "properties": { + "query": { + "type": "string", + "description": """ A specific search query to find relevant information in the user's documents. Use keywords, phrases, or questions related to what the user is asking about. Be specific rather than generic - use terms that would appear in the relevant documents. -""" - -# SEARCH_TOOLS_SCHEMA defines the search_documents tool for the OpenAI API. -# The model reads this schema to know what tools are available and what -# arguments to generate — it can only generate arguments declared here. -SEARCH_TOOLS_SCHEMA = [ - { - "type": "function", - "name": "search_documents", - "description": TOOL_DESCRIPTION, - "parameters": { - "type": "object", - "properties": { - "query": { - "type": "string", - "description": TOOL_PROPERTY_DESCRIPTION, - } - }, - "required": ["query"], +""", + } }, - } -] - - -# TODO: Add get_tools_schema() and make_tool_mapping(user) aggregation functions -# that combine all tool schemas and mappings so assistant_services.py never needs -# to change when a new tool is added — only tool_services.py does. - -def make_search_tool_mapping(user) -> dict[str, Callable]: - # make_search_tool_mapping binds user to search_documents at call time. - # user is a request-time value the model cannot generate, so it must be - # captured here and kept out of the schema. - """Return a tool mapping with search_documents bound to the given user. - - Parameters - ---------- - user : User - The Django user object used for document access control. - - Returns - ------- - dict[str, Callable] - Tool mapping ready to pass to invoke_functions_from_response. - """ - def bound_search(query: str) -> str: - return search_documents(query, user) - - return {"search_documents": bound_search} - - -def search_documents(query: str, user) -> str: - """ - Search through user's uploaded documents using semantic similarity. - - This function performs vector similarity search against the user's document corpus - and returns formatted results with context information for the LLM to use. - - Parameters - ---------- - query : str - The search query string - user : User - The authenticated user whose documents to search - - Returns - ------- - str - Formatted search results containing document excerpts with metadata - - Raises - ------ - Exception - If embedding search fails - """ - - try: - embeddings_results = get_closest_embeddings( - user=user, message_data=query.strip() - ) - embeddings_results = convert_uuids(embeddings_results) - - if not embeddings_results: - return "No relevant documents found for your query. Please try different search terms or upload documents first." - - # Format results with clear structure and metadata - prompt_texts = [ - f"[Document {i + 1} - File: {obj['file_id']}, Name: {obj['name']}, Page: {obj['page_number']}, Chunk: {obj['chunk_number']}, Similarity: {1 - obj['distance']:.3f}]\n{obj['text']}\n[End Document {i + 1}]" - for i, obj in enumerate(embeddings_results) - ] - - return "\n\n".join(prompt_texts) - - except Exception as e: - return f"Error searching documents: {str(e)}. Please try again if the issue persists." - - -def invoke_functions_from_response( - response, tool_mapping: dict[str, Callable] -) -> list[dict]: - """Extract all function calls from the response, look up the corresponding tool function(s) and execute them. - (This would be a good place to handle asynchroneous tool calls, or ones that take a while to execute.) - This returns a list of messages to be added to the conversation history. - - Parameters - ---------- - response : OpenAI Response - The response object from OpenAI containing output items that may include function calls - tool_mapping : dict[str, Callable] - A dictionary mapping function names (as strings) to their corresponding Python functions. - Keys should match the function names defined in the tools schema. - - Returns - ------- - list[dict] - List of function call output messages formatted for the OpenAI conversation. - Each message contains: - - type: "function_call_output" - - call_id: The unique identifier for the function call - - output: The result returned by the executed function (string or error message) - """ - - # Open AI Cookbook: Handling Function Calls with Reasoning Models - # https://cookbook.openai.com/examples/reasoning_function_calls - - intermediate_messages = [] - for response_item in response.output: - if response_item.type == "function_call": - target_tool = tool_mapping.get(response_item.name) - if target_tool: - try: - arguments = json.loads(response_item.arguments) - logger.info( - f"Invoking tool: {response_item.name} with arguments: {arguments}" - ) - tool_output = target_tool(**arguments) - logger.info(f"Tool {response_item.name} completed successfully") - except Exception as e: - msg = f"Error executing function call: {response_item.name}: {e}" - tool_output = msg - logger.error(msg, exc_info=True) - else: - msg = f"ERROR - No tool registered for function call: {response_item.name}" - tool_output = msg - logger.error(msg) - intermediate_messages.append( - { - "type": "function_call_output", - "call_id": response_item.call_id, - "output": tool_output, - } - ) - elif response_item.type == "reasoning": - logger.info(f"Reasoning step: {response_item.summary}") - return intermediate_messages - -def handle_tool_calls_with_reasoning( - response, client, model_defaults: dict, tool_mapping: dict[str, Callable] -) -> tuple[str, str]: - """Run the agentic loop until the model stops emitting function calls. - - Parameters - ---------- - response : OpenAI Response - The initial response from the model. - client : OpenAI - The OpenAI client instance. - model_defaults : dict - Keyword arguments forwarded to every client.responses.create call. - tool_mapping : dict[str, Callable] - Maps function names to their implementations. - - Returns - ------- - tuple[str, str] - (final_response_output_text, final_response_id) - """ - # Open AI Cookbook: Handling Function Calls with Reasoning Models - # https://cookbook.openai.com/examples/reasoning_function_calls - while True: - # Mapping of the tool names we tell the model about and the functions that implement them - function_responses = invoke_functions_from_response(response, tool_mapping) - if len(function_responses) == 0: # We're done reasoning - logger.info("Reasoning completed") - final_response_output_text = response.output_text - final_response_id = response.id - logger.info(f"Final response: {final_response_output_text}") - return final_response_output_text, final_response_id - else: - logger.info("More reasoning required, continuing...") - response = client.responses.create( - input=function_responses, - previous_response_id=response.id, - **model_defaults, - ) + "required": ["query"], + }, + # search_documents needs the request user for document access control. + run=lambda user, query: search_documents(query, user), +) + + +# The schema string describing the queryable medication table for ask_database's prompt. +# +# Kept in sync by hand with api.views.listMeds.models.Medication: if you add/rename a +# column there, update this string so ask_database's prompt matches the real table. +# +# Hand-writing the column list (rather than deriving it from Django's Model._meta) is a +# deliberate trade-off. _meta.concrete_fields would auto-sync with the model and needs no +# DB connection, but it dumps *every* column indiscriminately. A hand-written list lets us +# curate what the LLM sees — e.g. omit `id`, which the model never needs to filter on — and +# it drops the app-registry dependency (_meta requires the app registry loaded, so importing +# this module during app startup could raise AppRegistryNotReady). The cost is the manual +# update above, cheap for a table this small and stable. +_MEDICATION_SCHEMA_STRING = "Table: api_medication\nColumns: name, benefits, risks" + +# Sharpen this description — it is the highest-value change on this branch, and +# there are now two eval runs of evidence behind it. Across 14 tool calls over those two +# runs the model selected ask_database exactly 0 times. Decisively, for "What medications +# are recommended for bipolar depression?" it generated a semantic search query rather +# than SELECT name, benefits, risks FROM api_medication, despite that being a literal +# match for this table. +# +# The descriptions explain the split. SEARCH_TOOL's ends with a standing order ("Always +# search before answering questions about document content"); this one opens with a scope +# sentence and then spends its remaining length on SQL mechanics (brand→generic, +# LOWER() matching). One commands, the other documents syntax — so the model reads only +# the first as an instruction about *when* to call. +# +# The fix is ordering, not length: lead with when to prefer this over semantic search +# (exact medication attributes, enumerating the catalog, any question answerable from +# name/benefits/risks) and move the SQL mechanics down into the `query` parameter +# description, where they belong — that text is read when writing the argument, not when +# choosing the tool. Consider a matching "prefer ask_database for ..." clause in +# SEARCH_TOOL so the carve-out is stated from both sides. Re-run the eval afterwards: +# selection count is the measurement, and it is already baselined at 0. +ASK_DATABASE_TOOL = Tool( + name="ask_database", + description=""" +Use this tool to answer questions about the medications in the Balancer database. +Medications are stored by their official generic names, not brand names, so convert +brand names to generic names first and match case-insensitively +(e.g. LOWER(name) = LOWER('lurasidone')). The input must be a single, fully-formed +SQL SELECT query. +""", + parameters={ + "type": "object", + "properties": { + "query": { + "type": "string", + "description": ( + "A plain-text SQL SELECT query answering the user's question, " + "written against this schema:\n" + f"{_MEDICATION_SCHEMA_STRING}" + ), + } + }, + "required": ["query"], + }, + # ask_database queries the shared medication table, so it ignores the request user. + run=lambda user, query: ask_database(query), +) + + +# Single source of truth for the assistant's tools. assistant_services builds the +# schema list the model sees with [tool.schema() for tool in TOOLS]; the agentic loop +# indexes this by name to dispatch calls. Register a new tool by appending it here. +# +# OVERLAP RISK — no longer hypothetical: this exposes a semantic document-search tool AND +# a SQL medication-lookup tool at once, and for a question both could answer the model has +# resolved it entirely in favour of search_documents (0 ask_database selections in 14 tool +# calls across two eval runs). The lever is sharper descriptions carving out when to prefer +# which — see the TODO above ASK_DATABASE_TOOL — not a third overlapping tool. +TOOLS = [SEARCH_TOOL, ASK_DATABASE_TOOL] diff --git a/server/api/views/assistant/urls.py b/server/api/views/assistant/urls.py index 4c68f952..53467803 100644 --- a/server/api/views/assistant/urls.py +++ b/server/api/views/assistant/urls.py @@ -1,5 +1,5 @@ from django.urls import path -from .views import Assistant +from api.views.assistant.views import Assistant urlpatterns = [path("v1/api/assistant", Assistant.as_view(), name="assistant")] diff --git a/server/api/views/assistant/views.py b/server/api/views/assistant/views.py index 74bee8f6..9173078c 100644 --- a/server/api/views/assistant/views.py +++ b/server/api/views/assistant/views.py @@ -9,7 +9,7 @@ from drf_spectacular.utils import extend_schema, inline_serializer from rest_framework import serializers as drf_serializers -from .assistant_services import run_assistant +from api.views.assistant.assistant_services import run_assistant logger = logging.getLogger(__name__) @@ -46,16 +46,16 @@ def post(self, request): message = request.data.get("message", None) previous_response_id = request.data.get("previous_response_id", None) - final_response_output_text, final_response_id = run_assistant( - message=message, + result = run_assistant( user=user, + message=message, previous_response_id=previous_response_id, ) return Response( { - "response_output_text": final_response_output_text, - "final_response_id": final_response_id, + "response_output_text": result.output_text, + "final_response_id": result.response_id, }, status=status.HTTP_200_OK, )