-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(qa): prevent grader truncation #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ | |
| from api.services.workflow.dto import QANodeData | ||
|
|
||
| QA_USAGE_CONTEXT = "qa_analysis" | ||
| # Explicit output ceiling for QA grader inference. Google/Anthropic default to | ||
| # 4096 output tokens (shared with dynamic thinking on Gemini), truncating long | ||
| # grading JSON. A generous ceiling + bounded thinking lets a full grade complete. | ||
| QA_MAX_OUTPUT_TOKENS = 16384 | ||
|
|
||
|
|
||
| async def create_qa_llm_service( | ||
|
|
@@ -47,6 +51,7 @@ async def create_qa_llm_service( | |
| api_key, | ||
| correlation_id=correlation_id, | ||
| usage_context=QA_USAGE_CONTEXT, | ||
| max_tokens=QA_MAX_OUTPUT_TOKENS, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: max_tokens is threaded unconditionally from the QA path for every provider, but create_llm_service_from_provider only honors it in the GOOGLE branch (service_factory.py ~1005). The GOOGLE_VERTEX and Anthropic branches accept the new parameter and silently ignore it. The comment added next to QA_MAX_OUTPUT_TOKENS explicitly notes that Anthropic also defaults to 4096 output tokens, so an Anthropic (or Vertex) QA grader will still hit the same mid-object truncation that this PR is fixing, while the caller believes a generous ceiling was requested. Consider applying the output ceiling on the Anthropic branch (and Vertex if it shares the same cap) or scoping the QA flag to the provider actually covered, so the fix is not silently ineffective for other providers. Prompt for AI agents |
||
| **kwargs, | ||
| ) | ||
| return llm, model | ||
|
|
@@ -75,6 +80,7 @@ async def create_qa_llm_service( | |
| model_override, | ||
| correlation_id=correlation_id, | ||
| usage_context=QA_USAGE_CONTEXT, | ||
| max_tokens=QA_MAX_OUTPUT_TOKENS, | ||
| ) | ||
| return llm, model | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| """Regression test: QA-grader thinking config must match the Google model family. | ||
|
|
||
| ``thinking_budget`` is only valid for Gemini 2.5; Gemini 3 uses ``thinking_level``, | ||
| and the two must not be mixed. Any id that isn't a recognized 2.5/3 family (custom, | ||
| legacy, future, or one that merely embeds the family name) must get no thinking | ||
| config, so an unsupported setting can't make the grader reject the request — the | ||
| raised ``max_tokens`` ceiling alone still guards against truncation. | ||
|
|
||
| The last test exercises the real wiring: the QA ``max_tokens`` plus the guarded | ||
| thinking config must build valid Google *and* Vertex settings. | ||
| """ | ||
|
|
||
| from api.services.pipecat.service_factory import ( | ||
| GoogleLLMSettings, | ||
| GoogleVertexLLMSettings, | ||
| _google_thinking_for_model, | ||
| ) | ||
|
|
||
| QA_MAX_TOKENS = 16384 | ||
|
|
||
|
|
||
| def test_gemini_25_uses_thinking_budget(): | ||
| tc = _google_thinking_for_model("gemini-2.5-flash") | ||
| assert tc is not None | ||
| assert tc.thinking_budget == 4096 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: These tests only call the isolated helper Prompt for AI agents |
||
| assert getattr(tc, "thinking_level", None) is None | ||
|
|
||
|
|
||
| def test_gemini_25_revision_still_matches(): | ||
| # dated/preview revisions still start with the family prefix | ||
| assert _google_thinking_for_model("gemini-2.5-flash-002").thinking_budget == 4096 | ||
|
|
||
|
|
||
| def test_gemini_3_uses_thinking_level(): | ||
| tc = _google_thinking_for_model("gemini-3-pro-preview") | ||
| assert tc is not None | ||
| assert tc.thinking_level == "low" | ||
| assert tc.thinking_budget is None | ||
|
|
||
|
|
||
| def test_unknown_legacy_or_embedded_substring_omits_thinking(): | ||
| # legacy family, a custom id, ids that only *contain* the family name, empty | ||
| for m in ( | ||
| "gemini-1.5-pro", | ||
| "some-custom-model", | ||
| "acme-gemini-2.5-wrapper", | ||
| "x-gemini-3-y", | ||
| "", | ||
| ): | ||
| assert _google_thinking_for_model(m) is None, m | ||
|
|
||
|
|
||
| def test_qa_settings_accept_max_tokens_and_family_thinking(): | ||
| """The real wiring: QA max_tokens + guarded thinking build valid settings.""" | ||
| for Settings in (GoogleLLMSettings, GoogleVertexLLMSettings): | ||
| tc = _google_thinking_for_model("gemini-2.5-flash") | ||
| s = Settings( | ||
| model="gemini-2.5-flash", | ||
| temperature=0.1, | ||
| max_tokens=QA_MAX_TOKENS, | ||
| thinking=tc, | ||
| ) | ||
| assert s.max_tokens == QA_MAX_TOKENS | ||
| assert s.thinking is not None and s.thinking.thinking_budget == 4096 | ||
|
|
||
| # legacy model: max_tokens still applied, no thinking config attached | ||
| legacy = Settings( | ||
| model="gemini-1.5-pro", | ||
| temperature=0.1, | ||
| max_tokens=QA_MAX_TOKENS, | ||
| thinking=_google_thinking_for_model("gemini-1.5-pro"), | ||
| ) | ||
| assert legacy.max_tokens == QA_MAX_TOKENS | ||
| assert legacy.thinking is None | ||
Uh oh!
There was an error while loading. Please reload this page.