feat(gooddata-eval): let evaluations request a reasoning effort - #1709
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThis change adds optional ChangesReasoning effort support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RunConfig
participant AgenticEvaluator
participant ChatClient
participant Langfuse
CLI->>RunConfig: Store reasoning_effort
RunConfig->>AgenticEvaluator: Pass reasoning_effort
AgenticEvaluator->>ChatClient: Construct with reasoning_effort
ChatClient->>ChatClient: Add options.reasoningEffort
AgenticEvaluator->>Langfuse: Record effort metadata and run name
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1709 +/- ##
==========================================
+ Coverage 78.30% 78.34% +0.04%
==========================================
Files 271 271
Lines 18689 18726 +37
==========================================
+ Hits 14634 14671 +37
Misses 4055 4055 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py (1)
451-461: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAppend
reasoning_effortafter the existing evaluator parameters.These public evaluator signatures insert the new parameter before
langfuse. Existing positional callers bind arguments to the wrong parameters. Keep the existing order and appendreasoning_effortafterrun_metadata_extra.
packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py#L451-L461: movereasoning_effortafterrun_metadata_extra.packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py#L394-L402: movereasoning_effortafterrun_metadata_extra.packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py#L143-L152: movereasoning_effortafterrun_metadata_extra.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py` around lines 451 - 461, Public evaluator signatures place reasoning_effort before existing parameters, breaking positional callers. In packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py lines 451-461, packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py lines 394-402, and packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py lines 143-152, move reasoning_effort after run_metadata_extra while preserving all existing parameter order.
🧹 Nitpick comments (1)
packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments for the growing
build_run_contextpositional call.All four call sites invoke
build_run_contextwith the same 8-argument positional list, and this PR addedreasoning_effortas the 8th positional argument at each site. Since most arguments share the typestr | None, a future reordering or insertion inbuild_run_context's signature would silently misalign the callers instead of raising a type error. Convert each call to keyword arguments to remove this risk.
packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py#L185-194: callbuild_run_contextwith keyword arguments (e.g.dataset_name=dataset_name, run_timestamp=run_timestamp, ..., reasoning_effort=reasoning_effort).packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py#L336-344: apply the same keyword-argument conversion.packages/gooddata-eval/src/gooddata_eval/core/agentic/search_tool.py#L179-187: apply the same keyword-argument conversion.packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py#L302-310: apply the same keyword-argument conversion.♻️ Example fix (guardrail.py)
run_name_base, run_metadata = build_run_context( - host, - token, - workspace_id, - dataset_name, - run_timestamp, - model_version_override, - run_metadata_extra, - reasoning_effort, + host=host, + token=token, + workspace_id=workspace_id, + dataset_name=dataset_name, + run_timestamp=run_timestamp, + model_version_override=model_version_override, + run_metadata_extra=run_metadata_extra, + reasoning_effort=reasoning_effort, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py` at line 1, Convert all four build_run_context call sites in guardrail.py, metric_skill.py, search_tool.py, and visualization.py from positional arguments to keyword arguments, including reasoning_effort and every existing parameter. Preserve the current argument values and call behavior while making each parameter explicitly named.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py`:
- Around line 252-266: Establish a single canonical form for reasoning_effort at
initialization time in the __init__ method (anchor site
packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py#L252-L266) by
validating or normalizing the parameter before storing it in
self._reasoning_effort, then reuse that same canonical value in two places: when
building the request options in
packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py#L290-L292 where
it is sent to the server, and in
packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py#L413-L419
where it is used for the Langfuse suffix and metadata. This ensures invalid or
empty values are caught early and all downstream consumers see consistent,
validated reasoning_effort values.
In `@packages/gooddata-eval/src/gooddata_eval/core/config.py`:
- Line 23: Update LangfuseSink to accept and use reasoning_effort by adding a
reasoning_effort parameter to LangfuseSink.__init__(), then pass
config.reasoning_effort when instantiating LangfuseSink in the non-agentic
ChatClient path. Incorporate the reasoning_effort value into both the run name
construction (using the same pattern as the agentic build_run_context path with
a suffix) and the trace metadata dict, ensuring non-agentic runs are
distinguishable by effort level in Langfuse.
---
Outside diff comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py`:
- Around line 451-461: Public evaluator signatures place reasoning_effort before
existing parameters, breaking positional callers. In
packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py lines
451-461, packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py
lines 394-402, and
packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py lines
143-152, move reasoning_effort after run_metadata_extra while preserving all
existing parameter order.
---
Nitpick comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py`:
- Line 1: Convert all four build_run_context call sites in guardrail.py,
metric_skill.py, search_tool.py, and visualization.py from positional arguments
to keyword arguments, including reasoning_effort and every existing parameter.
Preserve the current argument values and call behavior while making each
parameter explicitly named.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2edfc7ab-3e28-4918-bff6-9c3a2e5575c5
📒 Files selected for processing (13)
packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.pypackages/gooddata-eval/src/gooddata_eval/cli/main.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/search_tool.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.pypackages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.pypackages/gooddata-eval/src/gooddata_eval/core/config.pypackages/gooddata-eval/tests/test_sse_client.py
hkad98
force-pushed
the
jkd/eval-reasoning-effort
branch
from
August 4, 2026 16:33
110be83 to
115c616
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gooddata-eval/tests/test_sse_client.py (1)
383-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the
ChatClientconstructor default.
_capture_body_clientalways passesreasoning_effortat Line [388]. The unset-effort test therefore verifies explicitNone, not omission of the constructor argument. If the constructor default changes, this test can still pass.Forward
**kwargsthrough_client_with_handlerso the unset case omits the keyword.Proposed test-helper change
-def _capture_body_client(captured, *, reasoning_effort=None): +def _capture_body_client(captured, **kwargs): def handler(request): captured.append(json.loads(request.content)) return httpx.Response(200, content=_OK_SSE) - client = ChatClient(host="https://example.invalid", token="t", workspace_id="w", reasoning_effort=reasoning_effort) - client._client = httpx.Client(transport=httpx.MockTransport(handler)) - return client + return _client_with_handler(handler, **kwargs)Also applies to: 393-397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/tests/test_sse_client.py` around lines 383 - 390, Update _capture_body_client to forward optional constructor arguments through the existing _client_with_handler helper, ensuring the unset reasoning_effort case omits the keyword entirely rather than passing None. Preserve explicit reasoning_effort forwarding for tests that provide a value, and update the affected call sites accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/gooddata-eval/tests/test_sse_client.py`:
- Around line 383-390: Update _capture_body_client to forward optional
constructor arguments through the existing _client_with_handler helper, ensuring
the unset reasoning_effort case omits the keyword entirely rather than passing
None. Preserve explicit reasoning_effort forwarding for tests that provide a
value, and update the affected call sites accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb8d8e92-eef2-4a2c-aec8-c711f0038590
📒 Files selected for processing (17)
packages/gooddata-eval/README.mdpackages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.pypackages/gooddata-eval/src/gooddata_eval/cli/main.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/search_tool.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.pypackages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.pypackages/gooddata-eval/src/gooddata_eval/core/config.pypackages/gooddata-eval/src/gooddata_eval/core/langfuse/sink.pypackages/gooddata-eval/tests/test_agentic_run_context.pypackages/gooddata-eval/tests/test_cli.pypackages/gooddata-eval/tests/test_sse_client.py
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/gooddata-eval/src/gooddata_eval/core/config.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py
- packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py
- packages/gooddata-eval/src/gooddata_eval/cli/main.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/search_tool.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py
- packages/gooddata-eval/README.md
- packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py
- packages/gooddata-eval/tests/test_agentic_run_context.py
- packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py
- packages/gooddata-eval/tests/test_cli.py
- packages/gooddata-eval/src/gooddata_eval/core/langfuse/sink.py
- packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py
hkad98
enabled auto-merge
August 4, 2026 16:46
Adds an optional reasoning effort to `gooddata-eval`, so an evaluation run can request `LOW`, `MEDIUM` or `HIGH` instead of always using the endpoint default. Sent as `options.reasoningEffort` on each chat message and threaded through ChatClient, all seven `evaluate_agentic_*` entry points, the agentic runner and the CLI (`--reasoning-effort`). Per the AI chat OpenAPI schema the field is `SendMessageOptions.reasoningEffort`, typed as an enum of exactly LOW/MEDIUM/HIGH, nullable, and scoped to a single message rather than persisted. The implementation mirrors all four: uppercase `Literal`, camelCase key, re-sent per message, key omitted when unset. Without this there is no way to evaluate at a non-default effort — the value is read from the request body and the SDK never sent one, so effort was the one request-level option the suite could not vary. Both Langfuse writers record it. `build_run_context` suffixes the dataset-run name and adds run metadata; `LangfuseSink` suffixes its run name and reports the effort via trace `tags`, since metadata alone is not a breakdown dimension — the same reason `model_version` already uses first-class fields there. Two runs differing only by effort would otherwise share a name and merge in the report. The value is normalized once at the boundary. The `Literal` only constrains static callers, so without normalization a lowercase value reached the endpoint as an out-of-enum request, and a blank value was sent yet skipped by the writers' truthiness checks — leaving a run whose recorded identity disagreed with what it requested. Blank now means unset and anything outside the enum raises locally. New parameters are appended to the end of the public `evaluate_agentic_*` signatures rather than inserted mid-list, so existing positional callers of this released package keep binding correctly. Default is None, which omits the key and leaves the request payload byte-identical to before. Also drops an unused local in test_cli.py that was failing lint on master.
hkad98
force-pushed
the
jkd/eval-reasoning-effort
branch
from
August 4, 2026 17:19
115c616 to
49949a5
Compare
zdenekmusil-gd
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an optional reasoning effort to
gooddata-eval, so an evaluation run can requestLOW,MEDIUMorHIGHinstead of always using the endpoint default.It is sent as
options.reasoningEfforton each chat message and threaded throughChatClient, the sevenevaluate_agentic_*entry points, the agentic runner, and the CLI (--reasoning-effort).Contract
Per the AI chat OpenAPI schema (
/api/v1/schemas/gen-ai):SendMessageOptions.reasoningEffort— camelCase, sibling ofitemon the send-message request body.RequestedReasoningEffort, an enum of exactlyLOW,MEDIUM,HIGH.anyOf: [$ref, null]), so omitting the key is valid and is what this change does by default.The implementation mirrors all four: uppercase
Literal, camelCase key, per-message send, key omitted when unset.Why
The SDK had no way to set this, so every evaluation ran at whatever the endpoint defaults to. That makes reasoning effort the one request-level option the suite cannot vary, even though it plausibly affects both latency and answer quality — exactly the kind of thing an evaluation harness exists to measure.
The intent is for effort to become an evaluation dimension alongside model version, so the two can be compared in the same report.
Notes for reviewers
None, which omits the key entirely and leaves the request payload byte-identical to before. There is a test asserting full-body equality for that case, so existing runs cannot be affected.build_run_contextsuffixes the dataset-run name and adds run metadata;LangfuseSinksuffixes its run name and reports the effort via tracetags. Metadata alone is not a breakdown dimension — the same reasonmodel_versionalready uses first-class fields there. Without the run-name suffix, two runs differing only by effort share a name and merge in the report, which defeats the comparison.Literal["LOW", "MEDIUM", "HIGH"]to match the spec enum, with the CLI deriving itschoicesfrom that alias so the two cannot drift. An invalid value then fails locally instead of surfacing as a validation error partway through a run.evaluate_agentic_*signatures rather than inserted mid-list, so existing positional callers of this released package keep binding correctly.send_message— which is what the multi-turn evaluators need.SummaryClient, whose request body has no equivalent option, so the flag applies to chat items only. Also noted in the README.Testing
tests/test_sse_client.py— payload unchanged when unset; the option sent for each ofLOW/MEDIUM/HIGH; applied to every message on a multi-turn client; propagated throughask().tests/test_agentic_run_context.py— run-name suffix and run metadata, that the effort does not displacemodel_version, and that an unset effort leaves both unchanged.tests/test_cli.py— the flag reachesChatClient, and a value outside the enum is rejected by argparse.ruff check,ruff format --checkandty checkare clean.Unrelated to the feature: this also removes an unused local in
tests/test_cli.pythat was already failingruffonmaster.Summary by CodeRabbit
New Features
--reasoning-effortoption with LOW, MEDIUM, and HIGH settings.Documentation
Tests