-
Notifications
You must be signed in to change notification settings - Fork 20
feat(eval-author): default Author clients to medium reasoning effort #1347
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
Changes from 1 commit
a64d51e
c4f15a9
49eb435
f464765
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 |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Eval Author completion options Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Add `CompletionClientOptions` (`reasoning_effort` + `completion_params`) to `nooa_model_client`, and wire Eval Author with default `reasoning_effort="medium"`. | ||
|
|
||
| **Architecture:** Options are resolved once at `resolve_model_clients` and baked into each `CompletionClient`. Eval Author builds options from `EvalAuthorConfig`. Other consumers unchanged when options are omitted. | ||
|
|
||
| **Tech Stack:** Python, dataclasses, Pydantic, pytest, existing Nooa `CompletionClient` kwargs. | ||
|
|
||
| **Spec:** `docs/superpowers/specs/2026-08-17-eval-author-completion-options-design.md` | ||
|
|
||
| --- | ||
|
|
||
| ### Task 1: `CompletionClientOptions` + resolve wiring | ||
|
|
||
| **Files:** | ||
| - Modify: `packages/nemo_platform_plugin/src/nemo_platform_plugin/nooa_model_client.py` | ||
| - Modify/Create: `packages/nemo_platform_plugin/tests/test_nooa_model_client.py` | ||
|
|
||
| - [ ] Add frozen `CompletionClientOptions` | ||
| - [ ] Thread `options` through `_completion_client` / `resolve_model_clients` | ||
| - [ ] Merge order: base → `completion_params` → explicit `reasoning_effort` if not None | ||
| - [ ] Unit tests for omit / medium / params override / effort wins | ||
|
|
||
| ### Task 2: Eval Author config + run | ||
|
|
||
| **Files:** | ||
| - Modify: `plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/models.py` | ||
| - Modify: `plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py` | ||
| - Modify: `plugins/nemo-eval-author/tests/test_eval_author_run.py` | ||
|
|
||
| - [ ] `reasoning_effort: str | None = "medium"` | ||
| - [ ] `completion_params: dict[str, Any] = Field(default_factory=dict)` | ||
| - [ ] Pass `CompletionClientOptions` into `resolve_model_clients` | ||
| - [ ] Test default medium + explicit None + params forwarded (mock resolve) | ||
|
|
||
| ### Task 3: Verify | ||
|
|
||
| - [ ] `uv run --frozen pytest packages/nemo_platform_plugin/tests/test_nooa_model_client.py plugins/nemo-eval-author/tests/test_eval_author_run.py -q` | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| - [ ] Carry design doc onto the branch if missing | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| # Eval Author completion options | ||
|
|
||
| Date: 2026-08-17 | ||
| Status: implemented on branch `eval-author-completion-options/akhoury` | ||
|
|
||
| ## Problem | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| Eval Author (and other Nooa-backed agents) build `CompletionClient`s through | ||
| `nemo_platform_plugin.nooa_model_client` with no way to set `reasoning_effort` or | ||
| other LiteLLM/CompletionClient kwargs. On `main`, the field is omitted and the | ||
| provider default applies. Smoke Mode 1 evidence shows Author quality is sensitive | ||
| to effort; we want Author to default to `medium` without hardcoding it inside | ||
| `_completion_client` for every consumer. | ||
|
|
||
| ## Goals | ||
|
|
||
| - Let Eval Author specify OpenAI-style `reasoning_effort` (default **`medium`**). | ||
| - Let callers pass arbitrary `completion_params` for non-OpenAI backends (or extra | ||
| OpenAI knobs) without a second one-off API. | ||
| - Keep Analyst and Experimentalist behavior unchanged until they opt in. | ||
| - `None` / empty params must match today’s omit-the-field behavior for consumers | ||
| that do not set options. | ||
|
|
||
| ## Non-goals (v1) | ||
|
|
||
| - Per-component Experimentalist override map. | ||
| - Analyst wiring. | ||
| - CLI flags (follow-up). | ||
| - Provider-specific translation (e.g. Anthropic `thinking` helpers). Unsupported | ||
| keys continue to rely on existing `drop_params=True`. | ||
|
|
||
| ## Design | ||
|
|
||
| ### Shared: `CompletionClientOptions` | ||
|
|
||
| In `packages/nemo_platform_plugin/.../nooa_model_client.py`: | ||
|
|
||
| ```python | ||
| @dataclass(frozen=True) | ||
| class CompletionClientOptions: | ||
| reasoning_effort: str | None = None | ||
| completion_params: Mapping[str, Any] = field(default_factory=dict) | ||
| ``` | ||
|
|
||
| `resolve_model_clients(client, refs=None, options: CompletionClientOptions | None = None)` | ||
| threads options into `_completion_client`. | ||
|
|
||
| Merge order when constructing `CompletionClient`: | ||
|
|
||
| 1. Existing base kwargs (`api_base`, `drop_params`, `_skip_responses_api_bridge`, …). | ||
| 2. Spread `completion_params`. | ||
| 3. If `reasoning_effort is not None`, set `reasoning_effort=...` **last** so the | ||
| explicit field wins over the same key inside `completion_params`. | ||
|
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Protect internal
🤖 Prompt for AI Agents |
||
|
|
||
| If `options` is `None` or both fields are unset, wire behavior matches `main` | ||
| today (no `reasoning_effort` kwarg). | ||
|
|
||
|
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Make If Also applies to: 75-76 🤖 Prompt for AI Agents |
||
| Apply the same merge for OpenAI- and Anthropic-shaped clients. No format-specific | ||
| branching in v1. | ||
|
|
||
| ### Eval Author | ||
|
|
||
| `EvalAuthorConfig` gains: | ||
|
|
||
| ```python | ||
| reasoning_effort: str | None = "medium" | ||
| completion_params: dict[str, Any] = Field(default_factory=dict) | ||
| ``` | ||
|
|
||
| `run_eval_author` builds `CompletionClientOptions` from config and passes it to | ||
| `resolve_model_clients`. Mode 1 inherits this when Experimentalist invokes Author: | ||
| the runner nested-resolves Author-scoped clients from `config.eval_author` (default | ||
| `medium`) so Experimentalist's outer default/fast pair stays unchanged. | ||
|
|
||
| Setting `reasoning_effort=None` in config restores provider-default omit for a | ||
| given run. | ||
|
|
||
| ### Defaults summary | ||
|
|
||
| | Consumer | v1 default | | ||
| | --- | --- | | ||
| | `resolve_model_clients` (no options) | omit effort (unchanged) | | ||
| | Eval Author config | `reasoning_effort="medium"` | | ||
| | Analyst / Experimentalist agents | unchanged (no options passed) | | ||
|
|
||
| ## Tests | ||
|
|
||
| - Unit (`test_nooa_model_client`): merge order; `None` omits effort; params | ||
| forwarded; explicit effort overrides duplicate key in params. | ||
| - Eval Author: default config resolves with `reasoning_effort="medium"`; explicit | ||
| `None` omits; `completion_params` reach `resolve_model_clients` (mock). | ||
|
|
||
| ## Follow-ups | ||
|
|
||
| - Optional Experimentalist YAML under `eval_author:` for these fields. | ||
| - CLI flags for standalone Author runs. | ||
| - Run-level + per-role map (design C) for Experimentalist components. | ||
| - Anthropic-oriented helpers if silent `drop_params` proves insufficient. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,10 +9,11 @@ | |||||||||||||||||||||||||||||||||||||
| public plugin contract. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| from collections.abc import Iterator | ||||||||||||||||||||||||||||||||||||||
| from collections.abc import Iterator, Mapping | ||||||||||||||||||||||||||||||||||||||
| from contextlib import contextmanager | ||||||||||||||||||||||||||||||||||||||
| from contextvars import ContextVar | ||||||||||||||||||||||||||||||||||||||
| from dataclasses import dataclass | ||||||||||||||||||||||||||||||||||||||
| from dataclasses import dataclass, field | ||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| from nemo_platform import AsyncNeMoPlatform | ||||||||||||||||||||||||||||||||||||||
| from nemo_platform.config import get_context | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -35,6 +36,20 @@ class ConfiguredModelRefs: | |||||||||||||||||||||||||||||||||||||
| fast: str | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| @dataclass(frozen=True) | ||||||||||||||||||||||||||||||||||||||
| class CompletionClientOptions: | ||||||||||||||||||||||||||||||||||||||
| """Optional kwargs applied when building each run-scoped CompletionClient. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ``reasoning_effort`` is the OpenAI-shaped shortcut. When ``None``, the field | ||||||||||||||||||||||||||||||||||||||
| is omitted so the provider default applies. ``completion_params`` are spread | ||||||||||||||||||||||||||||||||||||||
| into ``CompletionClient`` for backend-specific knobs; an explicit | ||||||||||||||||||||||||||||||||||||||
| ``reasoning_effort`` wins over the same key inside ``completion_params``. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| reasoning_effort: str | None = None | ||||||||||||||||||||||||||||||||||||||
| completion_params: Mapping[str, Any] = field(default_factory=dict) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| @dataclass(frozen=True) | ||||||||||||||||||||||||||||||||||||||
| class ConfiguredModelClients: | ||||||||||||||||||||||||||||||||||||||
| """Resolved Nooa clients for default and low-latency agent work.""" | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -84,10 +99,21 @@ def _parse_model_ref(model_ref: str) -> tuple[str, str]: | |||||||||||||||||||||||||||||||||||||
| return workspace, name | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def _client_option_kwargs(options: CompletionClientOptions | None) -> dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||
| """Merge completion options: params first, then explicit reasoning_effort.""" | ||||||||||||||||||||||||||||||||||||||
| if options is None: | ||||||||||||||||||||||||||||||||||||||
| return {} | ||||||||||||||||||||||||||||||||||||||
| kwargs: dict[str, Any] = dict(options.completion_params) | ||||||||||||||||||||||||||||||||||||||
| if options.reasoning_effort is not None: | ||||||||||||||||||||||||||||||||||||||
| kwargs["reasoning_effort"] = options.reasoning_effort | ||||||||||||||||||||||||||||||||||||||
| return kwargs | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+109
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Honor
Proposed fix kwargs: dict[str, Any] = dict(options.completion_params)
- if options.reasoning_effort is not None:
+ if options.reasoning_effort is None:
+ kwargs.pop("reasoning_effort", None)
+ else:
kwargs["reasoning_effort"] = options.reasoning_effort📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def _completion_client( | ||||||||||||||||||||||||||||||||||||||
| client: AsyncNeMoPlatform, | ||||||||||||||||||||||||||||||||||||||
| model_entity: ModelEntity, | ||||||||||||||||||||||||||||||||||||||
| served_model_name: str, | ||||||||||||||||||||||||||||||||||||||
| options: CompletionClientOptions | None = None, | ||||||||||||||||||||||||||||||||||||||
| ) -> CompletionClient: | ||||||||||||||||||||||||||||||||||||||
| """Build a Nooa client that routes through the Model Entity's Platform URL.""" | ||||||||||||||||||||||||||||||||||||||
| api_base = client.models.get_model_entity_route_openai_url(model_entity) | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -96,6 +122,7 @@ def _completion_client( | |||||||||||||||||||||||||||||||||||||
| # response bytes. Nooa needs decoded JSON/SSE, so make that requirement | ||||||||||||||||||||||||||||||||||||||
| # explicit at this adapter boundary for every configured agent client. | ||||||||||||||||||||||||||||||||||||||
| extra_headers[_ACCEPT_ENCODING_HEADER] = _IDENTITY_ENCODING | ||||||||||||||||||||||||||||||||||||||
| option_kwargs = _client_option_kwargs(options) | ||||||||||||||||||||||||||||||||||||||
| # Backend format is the Platform-facing wire contract, not the upstream | ||||||||||||||||||||||||||||||||||||||
| # provider identity. The LiteLLM prefix selects the adapter for that shape. | ||||||||||||||||||||||||||||||||||||||
| if model_entity.backend_format == _OPENAI_FORMAT: | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -114,6 +141,7 @@ def _completion_client( | |||||||||||||||||||||||||||||||||||||
| # LiteLLM versions otherwise bridge GPT-5.4+ tool calls with any | ||||||||||||||||||||||||||||||||||||||
| # reasoning_effort value (including "none") to /responses. | ||||||||||||||||||||||||||||||||||||||
| _skip_responses_api_bridge=True, | ||||||||||||||||||||||||||||||||||||||
| **option_kwargs, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| elif model_entity.backend_format == _ANTHROPIC_FORMAT: | ||||||||||||||||||||||||||||||||||||||
| api_base = api_base.removesuffix("/v1") | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -131,6 +159,7 @@ def _completion_client( | |||||||||||||||||||||||||||||||||||||
| base_model=litellm_model, | ||||||||||||||||||||||||||||||||||||||
| extra_headers=extra_headers, | ||||||||||||||||||||||||||||||||||||||
| drop_params=True, | ||||||||||||||||||||||||||||||||||||||
| **option_kwargs, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -159,6 +188,7 @@ async def _served_model_name( | |||||||||||||||||||||||||||||||||||||
| async def resolve_model_clients( | ||||||||||||||||||||||||||||||||||||||
| client: AsyncNeMoPlatform, | ||||||||||||||||||||||||||||||||||||||
| refs: ConfiguredModelRefs | None = None, | ||||||||||||||||||||||||||||||||||||||
| options: CompletionClientOptions | None = None, | ||||||||||||||||||||||||||||||||||||||
| ) -> ConfiguredModelClients: | ||||||||||||||||||||||||||||||||||||||
| """Resolve configured Model Entities and construct each distinct client once.""" | ||||||||||||||||||||||||||||||||||||||
| selected = refs or configured_model_refs() | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -171,7 +201,7 @@ async def resolve_model_clients( | |||||||||||||||||||||||||||||||||||||
| workspace, name = _parse_model_ref(model_ref) | ||||||||||||||||||||||||||||||||||||||
| entity = await client.models.retrieve(name, workspace=workspace) | ||||||||||||||||||||||||||||||||||||||
| served_model_name = await _served_model_name(client, entity, provider_cache) | ||||||||||||||||||||||||||||||||||||||
| resolved[model_ref] = _completion_client(client, entity, served_model_name) | ||||||||||||||||||||||||||||||||||||||
| resolved[model_ref] = _completion_client(client, entity, served_model_name, options) | ||||||||||||||||||||||||||||||||||||||
| except Exception as resolution_error: | ||||||||||||||||||||||||||||||||||||||
| for model_client in resolved.values(): | ||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the plugin-skill instruction.
This line tells workers to invoke plugin-based skills. That conflicts with the repository policy. Replace it with the repository-approved workflow.
As per coding guidelines: “DO NOT invoke any plugin-based skill,
/skill-nameslash command, or globally-installed assistant for these requests.”🤖 Prompt for AI Agents
Source: Coding guidelines