Repository navigation
feat(agents): allow optimization agents to configure models - #1159
Merged
Merged
Conversation
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
BrianNewsom
marked this pull request as ready for review
August 7, 2026 05:45
BrianNewsom
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR moves Insights, Experimentalist, and Eval Author to Platform-managed default and fast model pairs. Setup persists and resolves both models. Runtime integrations share routed clients with scoped cleanup. Insights stores model pairs for scheduled analysis. Legacy credential and tier configuration is removed. ChangesPlatform-managed model routing
Insights persistence and execution
Supporting configuration and documentation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-insights/examples/research-agent/README.md (1)
75-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not combine
--autowith prompt instructions.
--autouses automatic setup behavior. It does not support “press Enter at the fast prompt.” Remove--autofor interactive selection, or revise the text to describe automatic selection.🤖 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 `@plugins/nemo-insights/examples/research-agent/README.md` around lines 75 - 80, Update the setup instructions around the `nemo setup` command so they no longer combine `--auto` with interactive prompt guidance. Either remove `--auto` to preserve the documented Enter-to-reuse behavior, or revise the surrounding text to describe automatic model selection.
🧹 Nitpick comments (2)
packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModel resolution logic is duplicated.
resolve()and_create_default_config()now contain the same three-step precedence chain. Extract a small helper so a future precedence change updates one place.Also applies to: 489-492, 501-501, 551-559
🤖 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/nemo_platform_ext/src/nemo_platform_ext/config/config.py` around lines 75 - 78, Extract the duplicated three-step model precedence chain from resolve() and _create_default_config() into a shared helper, then reuse that helper at all affected call sites. Preserve the existing precedence and ensure future model-resolution changes only require updating the helper.packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py (1)
2251-2262: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
NEMO_FAST_MODELis dropped when no default model resolves.If discovery returns nothing and
NEMO_DEFAULT_MODELis unset, theelsebranch runs and neither model is persisted, even when the user exportedNEMO_FAST_MODEL. Persist the fast model in that case, or state in the warning that the fast override is ignored without a default.🤖 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/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py` around lines 2251 - 2262, Update the model persistence flow around ModelPair and the default_model fallback so an explicitly configured NEMO_FAST_MODEL is not discarded when default_model is unavailable. Persist the fast override in the no-default branch using the existing model configuration mechanism, or explicitly warn that it is ignored; preserve the current behavior when a default model resolves.
🤖 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 @.github/workflows/insights-testbed.yml:
- Around line 117-121: Update the NEMO_DEFAULT_MODEL fallback to the
provider-discovered ID default/openai-gpt-5-5, and remove NEMO_FAST_MODEL when
it matches the default so _run_auto_mode supplies the fallback.
In `@docs/get-started/example-agent.mdx`:
- Around line 168-169: Add the clone-mode warning immediately before the
Git-backed command containing NEMO_DEFAULT_MODEL and NEMO_FAST_MODEL, explaining
that clone mode is an integrity boundary rather than a confidentiality boundary.
Keep the existing later local-only warning unchanged.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py`:
- Around line 390-414: Update _configure_local_connection so setup never
overwrites an existing context named by _LOCAL_CONTEXT_NAME. Before
Config.write, detect whether that context already exists and reject the
operation or use a distinct dedicated context, preserving the existing context’s
endpoint and credentials while retaining the local no-auth setup behavior.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/nooa_model_client.py`:
- Around line 47-51: Update NooaModelClient.aclose to always attempt closing
fast when it is distinct from default, even if default.aclose fails. In the
model-resolution failure path around resolved, close every already-created
client before re-raising the original resolution error. Add failure-path tests
covering both cleanup scenarios in
packages/nemo_platform_plugin/src/nemo_platform_plugin/nooa_model_client.py:47-51
and :159-167.
In `@plugins/nemo-experimentalist/benchmarks/run.py`:
- Around line 345-348: Update the summary aggregation logic around the job field
validation to validate raw payload or stats values before calling int(). Reject
booleans explicitly, along with floats and numeric strings, and preserve the
field name in the TypeError; only convert values after they pass the strict
integer check.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py`:
- Around line 106-136: Preserve the selected default and fast model references
returned by resolve_model_clients in the run flow, and expose or pass them to
the benchmark caller alongside the result summary. Update the benchmark
persistence path, including benchmarks/run.py and its summary.json generation,
to record both resolved references in addition to the existing ModelSpec fields.
In `@plugins/nemo-insights/examples/research-agent/tests/test_analyst_e2e.py`:
- Around line 152-154: Update the credential check in the E2E fixture to require
TAVILY_API_KEY and accept either NVIDIA_API_KEY or NEMO_DEFAULT_INFERENCE_KEY as
the model-provider credential. Ensure the missing-key message reports the model
credential requirement without skipping when only the alternate credential is
present.
---
Outside diff comments:
In `@plugins/nemo-insights/examples/research-agent/README.md`:
- Around line 75-80: Update the setup instructions around the `nemo setup`
command so they no longer combine `--auto` with interactive prompt guidance.
Either remove `--auto` to preserve the documented Enter-to-reuse behavior, or
revise the surrounding text to describe automatic model selection.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py`:
- Around line 2251-2262: Update the model persistence flow around ModelPair and
the default_model fallback so an explicitly configured NEMO_FAST_MODEL is not
discarded when default_model is unavailable. Persist the fast override in the
no-default branch using the existing model configuration mechanism, or
explicitly warn that it is ignored; preserve the current behavior when a default
model resolves.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py`:
- Around line 75-78: Extract the duplicated three-step model precedence chain
from resolve() and _create_default_config() into a shared helper, then reuse
that helper at all affected call sites. Preserve the existing precedence and
ensure future model-resolution changes only require updating the helper.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5ee37ee-7c42-46bd-9f55-4a7d4633abbd
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/manifest_registry.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/config/config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/config/models.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.pyis excluded by!sdk/**
📒 Files selected for processing (88)
.github/actions/insights-intake-stack/action.yml.github/workflows/insights-testbed.ymlREADME.mdSETUP.mddocs/agents/insight-driven-optimization.mdxdocs/get-started/example-agent.mdxdocs/get-started/setup.mdxdocs/set-up/config-reference.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/manifest_registry.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/src/nemo_platform_ext/config/config.pypackages/nemo_platform_ext/src/nemo_platform_ext/config/models.pypackages/nemo_platform_ext/tests/cli/commands/test_setup.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/nooa_model_client.pypackages/nemo_platform_plugin/tests/test_nooa_model_client.pyplugins/nemo-eval-author/.env.exampleplugins/nemo-eval-author/README.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/model_config.pyplugins/nemo-eval-author/tests/conftest.pyplugins/nemo-eval-author/tests/test_eval_author_repair_e2e.pyplugins/nemo-eval-author/tests/test_eval_author_run.pyplugins/nemo-eval-author/tests/test_model_config.pyplugins/nemo-eval-author/tests/test_plugin_boundary.pyplugins/nemo-experimentalist/AGENTS.mdplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/benchmarks/README.mdplugins/nemo-experimentalist/benchmarks/configs/tau3-quality.yamlplugins/nemo-experimentalist/benchmarks/configs/tau3-smoke.yamlplugins/nemo-experimentalist/benchmarks/configs/terminal-bench-quality.yamlplugins/nemo-experimentalist/benchmarks/configs/terminal-bench-smoke.yamlplugins/nemo-experimentalist/benchmarks/run.pyplugins/nemo-experimentalist/examples/tau3-nooa-agent/.env.exampleplugins/nemo-experimentalist/examples/tau3-nooa-agent/record_tau_airline_traces.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cards.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/coder.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/goal_tree.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/rationalizer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/terminator.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_scorer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/settings.pyplugins/nemo-experimentalist/tests/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_model_config.pyplugins/nemo-experimentalist/tests/experimentalist/test_terminator.pyplugins/nemo-experimentalist/tests/experimentalist/test_tools.pyplugins/nemo-experimentalist/tests/test_cli_profile.pyplugins/nemo-experimentalist/tests/test_experiment_cli.pyplugins/nemo-experimentalist/tests/test_experimentalist_benchmark.pyplugins/nemo-experimentalist/tests/test_experimentalist_run.pyplugins/nemo-experimentalist/tests/test_preflight.pyplugins/nemo-experimentalist/tests/test_resolve.pyplugins/nemo-insights/README.mdplugins/nemo-insights/examples/research-agent/README.mdplugins/nemo-insights/examples/research-agent/tests/test_analyst_e2e.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/agent.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/model_config.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/config.pyplugins/nemo-insights/src/nemo_insights_plugin/controller.pyplugins/nemo-insights/src/nemo_insights_plugin/entities.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/src/nemo_insights_plugin/preflight.pyplugins/nemo-insights/src/nemo_insights_plugin/schema.pyplugins/nemo-insights/src/nemo_insights_plugin/sdk_resources/analysis_configs.pyplugins/nemo-insights/src/nemo_insights_plugin/service.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/eval/stack.pyplugins/nemo-insights/tests/test_analysis_config_contract.pyplugins/nemo-insights/tests/test_analyst_agent.pyplugins/nemo-insights/tests/test_analyst_run.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_preflight.pyplugins/nemo-insights/tests/testbed/test_cli.pyscript/generate_config_docs.py
💤 Files with no reviewable changes (16)
- docs/set-up/config-reference.mdx
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/model_config.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/model_config.py
- plugins/nemo-experimentalist/tests/test_experimentalist_benchmark.py
- plugins/nemo-experimentalist/benchmarks/configs/terminal-bench-quality.yaml
- plugins/nemo-experimentalist/benchmarks/configs/tau3-smoke.yaml
- plugins/nemo-insights/src/nemo_insights_plugin/config.py
- plugins/nemo-insights/testbed/cli.py
- plugins/nemo-experimentalist/tests/experimentalist/test_model_config.py
- plugins/nemo-insights/tests/test_analyst_agent.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.py
- plugins/nemo-experimentalist/benchmarks/configs/tau3-quality.yaml
- plugins/nemo-eval-author/tests/test_model_config.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/settings.py
- script/generate_config_docs.py
- plugins/nemo-experimentalist/benchmarks/configs/terminal-bench-smoke.yaml
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
…newsom Signed-off-by: Brian Newsom <brnewsom@nvidia.com> # Conflicts: # docs/get-started/example-agent.mdx
Contributor
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Contributor
|
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/cli/reference.mdx`:
- Around line 50-51: Move the setup-text changes from the generated
docs/cli/reference.mdx output into its source document, then run the designated
documentation generation command to regenerate the reference page and commit the
updated generated output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8770a6e6-6dbb-4903-8374-3d69eec5cc62
📒 Files selected for processing (1)
docs/cli/reference.mdx
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
sklinglernv
reviewed
Aug 7, 2026
sklinglernv
reviewed
Aug 7, 2026
sklinglernv
reviewed
Aug 7, 2026
gaiadilorenzo
left a comment
gaiadilorenzo
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me but I'm missing a lot of nemo platform context
sklinglernv
reviewed
Aug 7, 2026
schuellc-nvidia
added a commit
that referenced
this pull request
Aug 7, 2026
The Configuration section documented `NEMO_EXPERIMENTALIST_API_BASE`, `_API_KEY`, and the smart/mid/fast tiers. #1159 deletes `settings.py` outright, so every one of those variables goes away: the optimizer now resolves a default/fast pair of Platform Model Entities selected by `nemo setup`, and reads no endpoint, provider key, or provider model name of its own. Rewrites the section against that PR's code rather than its prose, so the tier mapping is accurate -- the trajectory scorer moves from mid to fast, and the architecture doc from mid to default. Updates the sandbox `sbx exec --env` block to the two overrides that replace the six, and the secret-handling caution that assumed keys were among them. Also names the bounded sub-problem optimization loop in the round walkthrough. It was missing, and `implement` runs it before the integration check rather than inside it (coder.py:680-686). Review feedback from @gaiadilorenzo on #1165. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian Schüller <cschueller@nvidia.com>
8 of 15 tasks
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
…newsom Signed-off-by: Brian Newsom <brnewsom@nvidia.com> # Conflicts: # plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/agent.py # plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py # plugins/nemo-eval-author/tests/test_eval_author_run.py # plugins/nemo-experimentalist/README.md # plugins/nemo-experimentalist/examples/tau3-nooa-agent/.env.example # plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py # plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/terminator.py # plugins/nemo-experimentalist/tests/experimentalist/test_model_config.py
…newsom Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
gaiadilorenzo
approved these changes
Aug 7, 2026
…newsom Signed-off-by: Brian Newsom <brnewsom@nvidia.com> # Conflicts: # README.md
svvarom
reviewed
Aug 7, 2026
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
svvarom
approved these changes
Aug 7, 2026
callingmedic911
added a commit
that referenced
this pull request
Aug 7, 2026
Review feedback on #1116. #1159 moves the optimization agents onto the models chosen in `nemo setup` and drops `INFERENCE_API_KEY` as an agent credential, so naming that variable, the gateway, and Claude Opus 4.8 would go stale the day it lands. State the requirement instead — the Analyst needs a model to run on — and let `analyst doctor` be the authority on whether it is satisfied, which is true before and after that change. Also from review: - Frame the skill as analyzing behavior and producing Insights rather than "finding what an agent keeps getting wrong". - Describe `--agent-spec` by what it gives the Analyst: the intent behind the agent, which is not recoverable from code or traces. Per the AGENT-SPEC design doc, that is its whole job. (The proposed ETHOS.md rename has not landed, so the filename stays.) - Drop `--base-url` from the run example; it defaults to `NMP_BASE_URL`. The wider `NMP_*` rename is ASE-815 — 414 files reference those variables, so it is not a skill-PR change. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
callingmedic911
added a commit
that referenced
this pull request
Aug 7, 2026
The previous commit wrote the credential story as #1159 will leave it, but that PR has not landed: the analyst still reads INFERENCE_API_KEY directly today, so "rather than any credential of its own" was false in the world this ships into. Say only that the model configuration is in flux and that pre-flight names what is missing — accurate before and after. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
callingmedic911
added a commit
that referenced
this pull request
Aug 7, 2026
Review feedback on #1116. #1159 moves the optimization agents onto the models chosen in `nemo setup` and drops `INFERENCE_API_KEY` as an agent credential, so naming that variable, the gateway, and Claude Opus 4.8 would go stale the day it lands. State the requirement instead — the Analyst needs a model to run on — and let `analyst doctor` be the authority on whether it is satisfied, which is true before and after that change. Also from review: - Frame the skill as analyzing behavior and producing Insights rather than "finding what an agent keeps getting wrong". - Describe `--agent-spec` by what it gives the Analyst: the intent behind the agent, which is not recoverable from code or traces. Per the AGENT-SPEC design doc, that is its whole job. (The proposed ETHOS.md rename has not landed, so the filename stays.) - Drop `--base-url` from the run example; it defaults to `NMP_BASE_URL`. The wider `NMP_*` rename is ASE-815 — 414 files reference those variables, so it is not a skill-PR change. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
callingmedic911
added a commit
that referenced
this pull request
Aug 7, 2026
The previous commit wrote the credential story as #1159 will leave it, but that PR has not landed: the analyst still reads INFERENCE_API_KEY directly today, so "rather than any credential of its own" was false in the world this ships into. Say only that the model configuration is in flux and that pre-flight names what is missing — accurate before and after. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
schuellc-nvidia
added a commit
that referenced
this pull request
Aug 8, 2026
The README had grown into an operator runbook that duplicated the Experimentalist skill and the docs site while never explaining what the Experimentalist does. Refocus it on the plugin's function and link out for the rest. - Explain the optimization loop, the levers it can change, and the train-diagnoses / validation-selects split, matching the round order the implementation actually runs. - Add a parameter table built from the CLI options, plus smoke-first guidance, run cost and duration, and resume-on-rerun. - Document what ships in the box: the nooa and langchain-framework skills, the two example agents, and the canonical benchmark suites. - Lead with the example-agent walkthrough so readers start from a worked run. - Correct claims that did not hold against the source: the command is `nemo agents experimentalist doctor`; the evaluation container receives whatever each Harbor task's `task.toml` declares rather than a fixed forwarded set, with `AUT_MODEL_NAME` a convention of the example agents; NeMo Experiments receives a structure-only mirror; and the model table read as exhaustive when the fast model also drives sub-steps across most components. Keep the "Recommended laptop isolation" heading, which the Experimentalist skill links to by anchor, and carry forward the objective_function and regression_metrics reference from #1159 under a task-oriented heading. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
ryana
pushed a commit
to ryana/nemo-platform
that referenced
this pull request
Aug 12, 2026
* feat(skills): add nemo-analyst workflow Add the platform-bundled nemo-analyst skill so coding agents have a reliable route to `nemo agents analyst run` and the minimum correct operating knowledge to use it. The body stays lean and defers depth to docs/agents/insight-driven-optimization.mdx rather than restating it. It covers the Insight shape, the credential distinction from the Experimentalist (the Analyst reads only INFERENCE_API_KEY), the doctor pre-flight, the run, where Insights are stored, and a verification step. Persistence is documented as it behaves after NVIDIA-NeMo#1054: Insights always go to the platform, and --insights-file-output mirrors what was stored. Routing tests follow the explicit/implicit/negative-control convention already used by nemo-explore. Verified that all six analyst-intent prompts resolve to nemo-analyst, that the skill introduces no new failures elsewhere in the catalog, and that it wins no ties by sort order. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Aditya Pandey <aditya@autospace.co> Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * feat(skills): tighten nemo-analyst from journey findings Drove the skill end to end in a container against a real LangGraph agent's telemetry. It carried the workflow, so this sharpens what it left the driver to work out rather than adding to it — net zero lines, paid for by cutting redundancy from the storage and verify sections. - `analyst doctor` takes no `--agent` flag, so it always reports a red profile line. Say that passing `--agent` makes that line noise instead of leaving the reader to reason past a red mark, or to write an `optimizer.yaml` it does not need. - `agent_name` is carried on agent-level spans, not on their model and tool children, so the old "matches the `agent_name` on the spans" pointed at a query that undercounts. - Every span came back `status=success` while the agent was badly broken, so state that the Analyst judges behavior rather than status or scores. - Telemetry that captures only the shape of a run is a third cause of an empty result, alongside scoping and volume. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * feat(skills): decouple nemo-analyst from a specific model and credential Review feedback on NVIDIA-NeMo#1116. NVIDIA-NeMo#1159 moves the optimization agents onto the models chosen in `nemo setup` and drops `INFERENCE_API_KEY` as an agent credential, so naming that variable, the gateway, and Claude Opus 4.8 would go stale the day it lands. State the requirement instead — the Analyst needs a model to run on — and let `analyst doctor` be the authority on whether it is satisfied, which is true before and after that change. Also from review: - Frame the skill as analyzing behavior and producing Insights rather than "finding what an agent keeps getting wrong". - Describe `--agent-spec` by what it gives the Analyst: the intent behind the agent, which is not recoverable from code or traces. Per the AGENT-SPEC design doc, that is its whole job. (The proposed ETHOS.md rename has not landed, so the filename stays.) - Drop `--base-url` from the run example; it defaults to `NMP_BASE_URL`. The wider `NMP_*` rename is ASE-815 — 414 files reference those variables, so it is not a skill-PR change. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * fix(skills): don't claim nemo-analyst has no credential of its own The previous commit wrote the credential story as NVIDIA-NeMo#1159 will leave it, but that PR has not landed: the analyst still reads INFERENCE_API_KEY directly today, so "rather than any credential of its own" was false in the world this ships into. Say only that the model configuration is in flux and that pre-flight names what is missing — accurate before and after. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * fix(skills): vendor nemo-analyst into the SDK `lint-sdk-vendored` and `lint-cli` both fail on this branch because every skill is mirrored into `sdk/python/nemo-platform` and this one never was — it was the only skill missing from the vendored tree. Generated with `make vendor`; as with every other skill, `tests.json` is not vendored. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * refactor(skills): ship nemo-analyst with the Insights plugin Follow the pattern NVIDIA-NeMo#1088 settled on. That PR started with the Experimentalist skill in the platform package and deliberately moved it into the plugin; this one was written against the earlier shape. The skill documents `nemo agents analyst`, which only exists when nemo-insights is installed, so the platform package should not ship it. Moving it also removes it from the vendored SDK tree — plugin-owned skills are discovered through the `nemo.skills` entry point instead, which is what made lint-sdk-vendored care about it in the first place. Mirrors the Experimentalist layout: skills.py exposing skills_dir, a skills README, and an entry-point test. `nemo skills list` now reports the skill with source nemo-insights-plugin. Also adds the nemo-analyst row to nemo-skill-selection, as NVIDIA-NeMo#1088 did for the Experimentalist. The catalog already told agents to use the Experimentalist "after `agents analyst` has created an Insight" while having no row that routed there. The description said "Produces the Insight that nemo-experimentalist consumes". Now that both skills are in the catalog together, that literal token made this skill win the Experimentalist's own explicit routing test — scoring treats a hyphenated name as one token. Reworded to "the Experimentalist", which keeps the loop context without the collision. Catalog-wide: 47 failures both before and after, 12 more passes, and nemo-analyst involved in no failure. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * docs(skills): note Analyst telemetry analysis as plugin-owned Completes the NVIDIA-NeMo#1088 parallel: that PR appended the Experimentalist to the plugin-owned skills sentence in both files once its skill moved into the plugin. The Analyst now lives there too. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * fix(skills): verify the analyst run, not the stored Insight set CodeRabbit review on NVIDIA-NeMo#1116. The Verify section contradicted the skill's own premise: it said a successful run leaves at least one Insight, while "What it produces" says filing nothing is a valid outcome. It also verified the wrong thing — listing by `?agent=` returns every Insight for the agent, including earlier runs', so it attests the store rather than this run. Verify against what the run reports instead. `analyst_backend` emits a line per operation (`- created: <title> [<id>]`, `- updated: <id>`, or `- no insights created or updated`), so read those ids back individually via `GET .../insights/<insight-id>`, and treat "no insights created or updated" as the success it is. Two smaller fixes from the same review: - Keep the bearer token out of `curl`'s argv, where any process on the host can read it, by passing it through curl's config on stdin. Verified the header still arrives. - The trigger `my agent keeps getting wrong` was missing its object; now `my agent keeps getting things wrong`. Routing is unchanged against a clean main baseline — 47 failures both sides, and nemo-analyst still fails none and steals none. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * refactor(skills): drop the Insights skills README and entry-point test Review feedback on NVIDIA-NeMo#1116. Neither file was pulling its weight. skills/README.md is never read — the registry only looks at subdirectories containing a SKILL.md — and with a single skill in the directory it restated that skill's own frontmatter. The Experimentalist's equivalent earns its place by disambiguating a user-facing skill from an internal one; this one had nothing to disambiguate. test_skills_entry_point.py guarded the `nemo.skills` declaration in pyproject.toml. Without that entry the skill simply never appears in `nemo skills list`, so the failure announces itself the first time anyone uses the feature and does not need a regression test of its own. Verified by hand instead: the entry point resolves and `nemo skills list` reports nemo-analyst with source nemo-insights-plugin. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> --------- Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
ryana
pushed a commit
to ryana/nemo-platform
that referenced
this pull request
Aug 12, 2026
…IA-NeMo#1165) The README had grown into an operator runbook that duplicated the Experimentalist skill and the docs site while never explaining what the Experimentalist does. Refocus it on the plugin's function and link out for the rest. - Explain the optimization loop, the levers it can change, and the train-diagnoses / validation-selects split, matching the round order the implementation actually runs. - Add a parameter table built from the CLI options, plus smoke-first guidance, run cost and duration, and resume-on-rerun. - Document what ships in the box: the nooa and langchain-framework skills, the two example agents, and the canonical benchmark suites. - Lead with the example-agent walkthrough so readers start from a worked run. - Correct claims that did not hold against the source: the command is `nemo agents experimentalist doctor`; the evaluation container receives whatever each Harbor task's `task.toml` declares rather than a fixed forwarded set, with `AUT_MODEL_NAME` a convention of the example agents; NeMo Experiments receives a structure-only mirror; and the model table read as exhaustive when the fast model also drives sub-steps across most components. Keep the "Recommended laptop isolation" heading, which the Experimentalist skill links to by anchor, and carry forward the objective_function and regression_metrics reference from NVIDIA-NeMo#1159 under a task-oriented heading. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
AnuradhaKaruppiah
pushed a commit
to AnuradhaKaruppiah/nemo-platform
that referenced
this pull request
Aug 27, 2026
…nt (NVIDIA-NeMo#1313) * refactor(experimentalist): a runner and a context, so the loop is just a strategy `_run` held everything: input resolution, all backend I/O, the entity lifecycle, the round loop, and PR publishing. There was no seam a strategy could be swapped at, because the runner and the sole strategy were the same code. Split them. `runner.py` is the composition root — it prepares inputs, runs one strategy, then persists and publishes. It is the only code that holds a Backend. `context.py` is everything a strategy may reach: its datasets, its candidates, and the verbs for measuring and recording them. `deps.py` is gone; config is a constructor argument. Four things move out of the loop and into the host, because they were never the strategy's to decide: - The insight/no-insight branch, with Eval Author now behind a lazy import: a run with no Insight never authors a suite and must not fail to import without the package. - Run resume. The runner re-opens the ExperimentRun; the strategy rebuilds itself from `ctx.candidates()`. A strategy that declares `supports_resume = False` is refused loudly rather than silently restarted — these runs cost hours, so the silent restart is the expensive failure. - The winner's copy-out. Its skip list mixed three owners (backend metadata, strategy documentation, evaluator scaffolding), so it is now composed from three named sets instead of one literal that would strip a third-party strategy's real output. - The Insight-suite report sections, which are a reward channel's epilogue. `report_progress(completed, total, unit, note)` replaces `rounds_completed` end to end. Not every strategy has rounds — DSPy's compile() is one opaque call — and the ones that most need reporting are exactly the ones that cannot produce a fraction. A counter is always producible, so `ExperimentRun` now carries one plus its unit, and a consumer renders a bar only when a total is actually known. Tests get the shared doubles the suite conspicuously lacked: an in-memory backend, a fake evaluator, and a context factory, in tests/experimentalist/. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): a Candidate is metadata and an artifact reference A Candidate was four jobs in one object: a proposed change, an identity, a set of measurements, and the optimized artifact. The artifact was always a directory, the identity was derived from that directory's name, and storage of the first three was implemented by walking the fourth — `list_candidates` globbed `agents/*/metadata.json`. A strategy that does not produce one directory per candidate could not store, list, or resume anything. Separate them. `Proposal` carries the build request; the runner stores identity and measurements at `eval-and-optimize/candidates/<id>.json`; and the finished work is *addressed* through `artifact: ResourceRef` rather than contained. That is the same pattern `persist_evaluation` already uses for traces, applied to the one entity that lacked it. The lifecycle inverts with it. A candidate used to be created and then mutated as the Coder filled its directory in, which is why a failed build needed a killed marker so resume would not resurrect it. Now `ctx.fork()` reserves and populates a directory, the Builder writes it, and `ctx.commit_candidate()` validates the result and creates the Candidate — so `artifact` can be required, a failed build leaves no record at all, and the killed marker for it goes away. The fields that went with the old shape: - `round` → `ancestor is None` for "this is the baseline", which `entities.py` already documented as the same thing, plus a strategy-supplied `generation` for grouping. DSPy leaves it 0; our loop sets the round. - `optimization`/`optimization_type`/`task_ids` → `description` plus the embedded Proposal, whose payload is owned by the Proposer/Builder pair. The Coder gets its own `BuildRequest` view and never sees the entity. - `ancestor` is a candidate id, not a directory name, so `benchmarks/run.py` reads the winner's location from its artifact instead of building `agents/{label}`. Candidate validation rejects a record whose `ancestor` or `description` disagrees with the Proposal they were derived from — two accounts of one candidate's origin must not be able to drift. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): stop stripping metadata.json from candidate artifacts Candidate metadata lives in the run's own store now, so ``metadata.json`` inside an artifact is no longer the host's file. Leaving it on the copy-out and publish skip lists is exactly the failure mode §9 warns about: a third-party strategy that writes one has produced real output, and the host would silently drop it. The remaining entries keep the owner they came from — ``architecture.md`` is the strategy's, the Harbor wrappers are the evaluator's — so a later reader can tell why each one is there. Also drops the Coder prompts' instructions about a file it can no longer see, and silences two nooa-stub return diagnostics that only surfaced because these files entered the changed set. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the loop's own prompts still read dropped Candidate fields ``select_diverse_survivors`` and ``merge_analysis`` are CodeAct methods: their docstrings are executed as Python against real Candidate objects. Both still told the model to read ``candidate.round`` and index ``get_metadata(...)["round"]``, which no longer exist — so the first cell of every round's analysis would raise and burn iterations recovering. Points them at ``generation``, ``is_baseline`` and ``generated_from.payload``, and uses ``label`` rather than ``name``/``id`` for the workspace lookups, since the display handle is what that tool is keyed by now. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the evolution tree is keyed by id, so finalize must be too An end-to-end run reached the last statement of the loop and died with ``KeyError: 'agent-0'``. `EvolutionTree.add` files nodes under the candidate id now that identity is no longer the directory name, but `_finalize` still took the winner's *label* and used it for both `mark_best` and the final node lookup. `mark_best` swallowed the wrong key silently, so the mistake surfaced one line later as an unrelated error — it now raises on an unknown key, which is what would have located this in seconds. `_finalize` keeps the node it selected rather than round-tripping through a string: `mark_best` gets the id, and `write_final_report` gets the label, because it reads the label-keyed workspace. Neither substitutes for the other. The test doubles hid this: `make_candidate` set `_id = label`, so every test saw identity and display handle as the same string — the one thing this milestone separated. They are now distinct by default, and a regression test covers the finalize path. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the mirror's ancestor lookup used an id as a label Studio lineage links were silently dropped. `_experiment_ids` was keyed by display label but looked up with `Candidate.ancestor`, which is a candidate id now — so the cache always missed, and the fallback then composed an Experiment name from that id, which can never exist. `NotFoundError` is caught by design, so the projection degraded quietly and an end-to-end run was the only way to see it. Experiment names are built from labels because they are meant to be greppable, and `ancestor` is an id because identity is no longer the directory name; neither should change to suit the other. So the mirror keeps both: the id keys the cache, and an id → label map lets it name an ancestor's Experiment. Within a run the ancestor is always projected first, so the map is warm; across a resume where a child is projected first the link is omitted rather than guessed. `experiment_metadata` now carries `candidate_id` (the real id) alongside `candidate_label`, so lineage is reconstructible from the projection alone — `candidate_id` previously held the label, which is what made this easy to write. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): a fork must not inherit the ancestor's architecture doc `architecture.md` is not documentation. It is the Proposer's only view of an agent — its prompt says to reason exclusively from that graph and not to read source — so whatever is in that file is what the next round proposes changes against. The pre-M1 fork excluded it and let the Coder seed it from the ancestor at the end of a build, right before rewriting it against the finished source. `_ignore_forked` copied it at fork time instead. Two consequences: - The Coder's seed step became unreachable: `ancestor_arch.exists() and not candidate_arch.exists()` can never hold once the fork provides the file. Dead code that still reads as live. - Absence stopped being a signal. `create_architecture_doc` is an LLM call and can fail; the Proposer renders a missing file as "(architecture.md missing for agent-N)" and knows it is blind. With the file inherited, a failed regeneration instead leaves the *ancestor's* graph in place and the Proposer reads it as the candidate's — a stale but plausible input, which is worse than a missing one because nothing surfaces it. Only reachable at max_rounds > 1, so the smoke config could not have caught it. Restores the exclusion and names why it sits alongside the run layout, the hygiene set and the evaluator's scratch. No claim is made that this caused the Coder to no-op in the M1 end-to-end run; that is still unexplained. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): resolve the model tiers once and hand them down Every component reached for a module-level getter to build its own clients, so nothing could say which models a run used, two runs in one process could not target different endpoints, and a test had to go through the environment to substitute a fake. `ModelTiers` resolves a tier on first use from one settings object. The runner builds one per run, the context carries it, and the strategy passes it to each of the nine components it constructs. The parameter defaults to this install's settings so a standalone caller still works, matching how every other config slice in this plugin is handed over. The run record follows from that. `config_snapshot` dumped only `EvolutionaryOptimizerConfig`, which since M0 split deployment settings out no longer mentions the endpoint or the tiers at all — so the one durable record of how a run was configured could not answer which models it used. It now carries a `deployment` block with the resolved endpoint and tier names, and deliberately not the credential: that record is written to run.json and mirrored to the platform. The gap predates M1 — the snapshot has dumped declared rather than resolved values since the plugin was migrated, and a live run.json shows it recording three nulls for a run that used three real models. Tests pin both halves; the snapshot one fails against the previous line. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): keep a candidate's record and its artifact in step Review of the M1 branch found ten defects, most of them one root cause: the record/artifact split left two halves of a candidate that nothing kept together. Deleting a directory used to delete the candidate, because the population was derived from directories. It is derived from records now, so rollback left ghost records addressing nothing — offered to the Proposer as branchable survivors, and selectable as the Pareto winner. `candidate_dir()` then resolved a vanished artifact to the shared `agents/` root instead of raising, so copying the winner out could `rmtree` the user's workspace entries and archival could push every candidate's code as one candidate's. Backends gain `delete_candidate`, `ctx.discard_candidate` removes both halves, and a missing artifact is refused. The rest: - `_reserve` gave the baseline's directory to every ancestor-less Proposal, so the HPO case the `generation` field exists for would have had N candidates overwrite one file while reporting distinct rewards. Only the baseline owns it. - Resuming before the first round analysis committed a second baseline: ids are uuids now, so a re-commit no longer overwrites a label-keyed record. Guarded. - `ancestor` became an id but was never validated, and the fork that resolves it ran outside the per-proposal error handling — so one hallucinated `"agent-2"` failed a multi-hour run. Validated in the Proposer, and an unforkable proposal is now dropped rather than fatal. - The build Coder was the one component not given the run's ModelTiers, so it wrote every candidate against the ambient endpoint while config_snapshot named the injected one. A test now asserts over the source that no construction omits it, since the failure is invisible per-component. - The winner was matched by value against candidates re-read from the store, so any field that did not round-trip dropped the Insight section silently. Matched by id. - `api_base` was copied verbatim into the run record, which is written to disk and mirrored to the platform — but a URL is a normal way to carry a key, which is why the log banner already strips userinfo. Sanitized. The previous guard test only checked the API-key value. - FakeBackend equated candidate id with label, which is what let the id/label confusions through in the first place. It now assigns uuids and keys by id. - The example's uv.lock kept the old nooa revision the repin replaced. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): carry M0's later fixes into the refactored loop Rebasing onto the current M0 branch brought three changes this milestone was written before, all in code it had since rewritten, so the conflict resolution that kept the new structure dropped them: - `Candidate.set_reward` is `record_reward` now. Every caller follows, including `ctx.record_reward`, which writes through it. - Cancellation is not a build failure. `CancelledError` derives from BaseException, so `_build_candidates`' filter would have let a cancelled build through as if it had succeeded — committed, evaluated and ranked. It re-raises instead, so the round unwinds. - A failed build logs its exception and traceback, not just the candidate name. `return_exceptions=True` turns a failure into a value, so the cause is only ever visible if it is logged, and a build that never became a candidate is the one thing a run cannot cheaply reproduce. Two tests M0 added for the reward accessor construct a Candidate directly; they now build one through the shared double, since the entity requires a description and an artifact. The evolution-tree table keeps M0's full channel names and this milestone's `description` column: M0 deleted the abbreviation map that the renamed column header was still reaching for. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): prove a method-level model tier reaches its model M0 made components take their tiers by injection, and M1 said the point of that was a canary: nothing in the suite proved an `@strategy(llm=...)` override actually resolves. `test_experimentalist_analyzer.py` builds its analyzer with `object.__new__` and assigns over `select_trials`, so it stubs past the decorator entirely — the override could stop working and every test would still pass while the run quietly moved to the class-level smart tier. Resolve it through nooa's own `resolve_method_llm`, which is what its dispatch calls, so the assertion is about our wiring and needs no LLM. Verified in both directions: deleting the `llm=lambda self: self._fast_model` line makes it fail. Set the tiers through the environment rather than passing them to `ExperimentalistConfig`. For a `NemoConfig` the environment wins over constructor arguments, so passing them reads back whatever the conftest exported and all three tiers collapse onto one client — which also makes `ModelTiers`' claim that a test can inject fakes without touching the environment false. Corrected that docstring rather than leaving the trap for the next person. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): the Builder owns the candidate span, and components register M1 said a Builder consumes one Proposal and returns a committed Candidate, and that a strategy is resolved by name. Neither was true: the strategy forked and committed, the Coder wrote into a directory handed to it through a `BuildRequest`, and `EvolutionaryOptimizer` was constructed by a factory that named the class. `Coder.build(ctx, proposal, *, generation) -> Candidate` now owns the whole span — it asks the context for a working copy, edits, verifies, and hands back the committed Candidate. No filesystem path crosses into it from outside, which is what lets candidate storage move without changing a Builder's signature. The seven places that rebuilt `eval-and-optimize/agents/<name>` from run layout, including inside prompt text, now descend from the Fork the context returned. `BuildRequest` is deleted. It re-read the code-change payload with `payload.get()` alongside the typed `CodeChange` the Proposer already writes, and dropped `root_cause` on the way — so the Coder was told to validate its fix against tasks chosen by a diagnosis it never saw. `CodeChange.model_validate(proposal.payload)` is now the single reading, and the diagnosis reaches the build. `fork` returns a Fork — a working copy plus the upstream it came from — because a fork inherently knows what it forked. Asking the context separately for "the ancestor directory of a proposal" was a confused question. `import_baseline` takes the baseline out of `fork`/`commit_candidate`, so both lose their "unless this is the baseline" branches and `proposal` is never None; it is also idempotent, since a resumed run re-enters it and the invariant is the host's to keep, not something each strategy reimplements. `save_candidate` could persist a Candidate that had never been validated, so the create path is private and `update_candidate` cannot reach it. Components are found by `(role, name)`. Ours ship entry points in the `nemo.experimentalist.components` group exactly like a third party's — there are no privileged built-ins, so if the mechanism a plugin developer depends on breaks, our own loop stops resolving too. Resolution never falls back; enumeration degrades, so one broken package cannot take down a run that does not name it. Roles are typed against Protocols in `seam.py` rather than the concrete context. That is structural, not stylistic: naming `ExperimentContext` in `roles` closes an import cycle through the backend, the config tree, and every component config slice. It also lets an out-of-tree strategy type its own `run` without importing our internals. `Candidate.rewards` becomes a mapping whose `__missing__` returns an empty record without storing it, replacing the `reward()`/`rewards` pair. Not a defaultdict: that one's `__missing__` inserts, so reading a channel would mark it measured, skip its evaluation and persist a phantom. A channel is written once, so the merge goes with it. Verified end to end: a smoke run completes with the baseline imported, a candidate built through the Builder, and records whose ancestor is the parent's id rather than its label. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): read the benchmark's winner through the store, not the layout The plan predicted this consumer would break: `benchmarks/run.py` reads the local backend's on-disk layout directly, including `ExperimentRun.winner_agent` — documented as a candidate id but used as a label to build `agents/{label}`, which only worked because the local backend set the store id equal to the label. Half of that was already fixed by resolving the winner through its own artifact reference. This does the other half: `load_winner` lives next to `load_candidate`, so the benchmark stops hardcoding `candidates/<id>.json` and the knowledge of where a run keeps its records has one home. Also fixes a defect introduced while making the file type-check. Harbor's task ids are a union and only `PackageTaskId` carries a `name`, so `str(getattr(task, "name", task))` returned a *pydantic repr* — "git_url='...' path=PosixPath('...')" — for the other two variants, silently substituting a garbage task id for a real one. There is no accessor that means the same thing across the union (`get_name()` gives "hello-world" for a git task and "org/hello-world" for a package one), and benchmark suites are pinned to published packages, so anything else is a suite authored wrong: say so instead of guessing. `metadata.version is None` now raises too, rather than validating "" against the suite's pinned revision. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): make the resume path survive being interrupted A workflow review of the branch found ten defects, five of them on resume — the one path no test drove, and which the MR description admitted had never been run live. Three of the five are regressions this branch introduced. **Resume.** A run interrupted after the baseline was scored but before round-0's analysis landed took the loop's *fresh* branch on restart, where the idempotent `_ensure_baseline` keeps the existing baseline and `_evaluate_validation_candidates` returns nothing pending — so indexing that map by label raised `KeyError` and failed the run at exactly the moment it must not. Recording is now its own named step that skips what was already measured. `run.json` is the only record of which run owns the candidates on disk, and it was rewritten in place on every progress report; a kill mid-write left a file resume could not parse, whereupon the runner minted a *new* run id and `ctx.candidates()` — which filters by it — returned nothing. Hours of candidates stayed on disk and became invisible. It is now written through a temp file and renamed, and starting over a populated candidate store refuses loudly instead of silently orphaning it. `_finalize` ran *outside* the handler that marks a run failed, so a run whose work had finished but whose winner artifact had gone stayed `running` forever with no result written. **Elsewhere.** An unknown ancestor from the Proposer aborted the whole run: that check runs after the CodeAct loop, so raising bought no retry — it unwound through the strategy and killed a multi-hour run over one bad string. Bad improvements are dropped now, with the batch still failing loudly if *every* one is unusable. `_ignore_forked` stripped `architecture.md` at every depth, deleting an agent's own `docs/architecture.md` from every candidate and from the winner copied back over the user's workspace; it is top-level only again, as the three paths it replaced were. Subclassing a registered component raised "duplicate" at import, because `name` was inherited — the most obvious way to customise a builder was the one way that could not work. The draft-PR title interpolated the Proposer's multi-sentence prose where a short handle belongs. `WorkspaceTool.get_metadata` re-parsed the whole candidate store per lookup, quadratic in a loop the prompts drive per agent. And the deleted `test_loop_failure.py` took the cancellation guard's only test with it. **Tests.** `test_resume_e2e.py` drives a real runner, context and local backend over a real directory, with only the model and evaluator faked, and cuts the run at the points a crash actually lands. Every fix here was checked by reverting it and confirming a test fails — including the atomic write, whose first test passed either way because it asserted the outcome rather than the mechanism. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): discard candidates by marking them, not deleting them Rolling back an incomplete round is the only thing that discards a candidate — a failed build never commits one, and a candidate that loses selection is marked `killed_generation` and kept. So the only work ever destroyed was work that is about to be redone, and destroying it made a wrong rollback unrecoverable. `discarded` is now a field. `discard_candidate` marks the record and leaves the artifact in place; `list_candidates` filters discarded candidates out unless asked for them, so a consumer cannot forget to. `delete_candidate` is gone from the backend interface — nothing needs it. Both halves survive together deliberately. Keeping the record while deleting the directory would let `_reserve` hand out a label a discarded record still claims, giving one run two candidates with the same handle — the id/label conflation that has already caused four defects here. Evaluator scratch is still removed on rollback: it is keyed by label, so leaving it would let the re-run read a previous round's results. `_delete_all_artifacts` is now `_roll_back_to`, which is what it does. Also sweeps the historical comments this branch introduced — "this used to be X", "no longer overwrites the way it did" — which read as noise to anyone who has not followed the refactor. State what the code does; the reason for a rule is worth keeping, the shape of what it replaced is not. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): review renames and clarifications Review feedback on names and comments that read as unclear: - `ctx.client` is `ctx.platform_client`, and `registry.get` is `get_component` — both were generic enough that a call site said nothing about what it returned. - `context.py`'s docstring names both `ExperimentContext` and `StrategyContext` and says which is which: the class and the Protocol components are typed against. - `_reserve` states the rule about fresh directories rather than illustrating it with HPO, which does not exist yet. - The dataset lookup asserts what it relies on — train and validation are guaranteed by the runner, insight exists only with an Insight — instead of leaving `.get` to imply it. - The run's own directories are named once in `_roll_back_to` rather than four times, and the tree/report keying comment sits on the line it explains. - `pyproject.toml` says why the entry points list modules rather than components. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): the baseline is just the candidate with no parent The baseline had its own creation verb, its own hardcoded `agent-0` handle, and its own `generated_from = None` case, so "is this the baseline" had to be answered in three ways and every creation path carried an "unless" branch. It is now an ordinary build of an ordinary Proposal: `kind="import"`, `ancestor=None`, built by an `Importer`. `commit_candidate` is the only way a Candidate is born, `generated_from` is never empty, and `ancestor is None` is the single place the distinction lives — as data, not a branch. Two things fall out. The baseline lands on `agent-0` because the first fork takes the first free handle, so nothing has to name it. And a strategy that wants several roots — importing three agents to compare — gets that for free, where the old verb assumed exactly one. Host-side idempotence goes with it, deliberately. "Exactly one baseline" is the evolutionary strategy's invariant, not the host's, and `_ensure_baseline` already enforces it; a strategy comparing three imported agents would have been wrong to stop. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): selection becomes a component Selection was spread over three places in the loop: a Pareto sort, an LLM method choosing diverse survivors, and a separate Pareto front picking the winner from evolution-tree nodes. Nothing named the policy, so nothing could replace it. `ParetoDiversitySelector` owns all three as `rank` / `survivors` / `winner`, resolved as `selector: pareto-llm-diversity`. It reads reward channels and nothing else — no Proposal, no artifact — which is what lets one selector serve a code-optimizing run and a numeric one. Which channels it ranks on is config (`objectives`, defaulting to validation) rather than a flag on the reward channel: "insight rewards must never feed Pareto selection" is a selector's business, and a `selecting: bool` on the channel would be wrong for anyone who wants to rank on train. Ranking and diversity are separate methods on purpose. Ranking is arithmetic over channels; choosing among incomparable candidates inside one front is a judgement that needs a model. A numeric strategy can reuse the first and skip the second. `_finalize` now takes the selector rather than reaching through `self`, so the winner comes from the same component that chose the survivors. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): every seam the strategy delegates to resolves by name The registry existed but held two roles; the other five were constructed directly by the loop, so a new optimization paradigm would have had to edit the strategy to reach them. All seven now resolve through it: strategy, builder, proposer, selector, terminator, root-cause-analyzer, trajectory-scorer. The two `disable_*` booleans are gone. Turning a step off is the degenerate case of choosing a different implementation, so `terminator: null` stops only on the round budget and `trajectory_scorer: null` skips step scoring and the goal tree it needs — the same spelling as `analyzer: null`. Each role's config moved to `<role>_config`, leaving the role key to name the component. `EvolutionaryOptimizer` becomes `EvolutionaryStrategy` in `strategies/evolutionary.py`: it fills the `strategy` role, and the file held one strategy rather than a loop shared by several. `EvolutionaryOptimizerConfig` keeps its name — it configures the algorithm, not the role. `nemo agents experimentalist components` lists what an install can resolve, which is how a developer checks their own package was picked up. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): M1's acceptance bar, as an assertion The milestone claims the core is done, and the test of that claim is that a new optimization paradigm needs no edit to the strategy or to any existing component. This registers a stand-in for all seven roles the way a `pip install`ed package would, names each in config, and checks the registry hands back what config asked for. If a seam is not swappable, this says so now rather than when HPO is written. Also covers the two failure modes that are easy to ship: a default naming a component that does not resolve, which kills a run after the user has waited for it, and the optional roles accepting null while the load-bearing ones do not. The fixture restores the registry's *contents* rather than swapping the mapping. Entry-point discovery writes into whichever dict is bound when it runs, and it runs once per process, so replacing the object sends the real components into a copy that is then discarded — leaving every later lookup empty. That is the same hazard as the `_loaded` flag, and it bit here first. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): evaluation resolves by name, and its type stops being a Literal `EvaluatorType = Literal["harbor"]` meant adding an evaluator required editing this package — the closed set that §2 names as the fourth reason the code could not host a plugin system. It is now the name of a registered `evaluation` component, so the set is open and a package can add to it. `HarborEvaluator` registers as `evaluation: harbor`, and the runner resolves it from `config.evaluation` rather than a hardcoded factory key. That makes eight roles swappable from config, which is the whole set the strategy delegates to. Deferred deliberately: lifting the ~130 lines of generic machinery out of `harbor.py` — the scoped Python import, the syntax preflight, the trial-dir walk — and the Harbor-specific fields that reach the generic loop through `EvaluatorConfig`'s `extra="allow"`. That is a quality problem inside one component, not a seam: it changes nothing about whether an evaluation component can be replaced, and it touches the path every run's numbers come from. It wants its own change with its own e2e, not a rider on this one. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): prove the plugin mechanism from outside the repository `examples/acme-strategies` is a separate package with its own pyproject, depending on the plugin rather than the monorepo, shipping one entry point. Installed beside us, its strategy resolves as `strategy: random-search` with no change here. That is the only evidence both discovery levels of §1 work for someone who never checked this repository out. Our own components register through the same group, but they live in this tree and this venv, so they prove the mechanism compiles rather than that it works for a stranger. Writing it surfaced a missing verb: `ctx.component(role, name)`, which §3.2 specifies and nothing had implemented. Without it a third-party strategy has to import our registry to reach a builder — reaching around the seam the context exists to be. It supplies the run's model tiers by default, since that is the one argument every component needs and the one a strategy should not have to know about. The strategy itself is deliberately trivial. What it demonstrates is the wiring: it imports only entities, the role, and the context Protocol, and reaches the platform only through the context. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the round budget is the loop's, and a bad proposal is not a bad run An end-to-end run failed at round two and exposed two defects, one of them a regression from promoting the terminator. **The loop had no bound of its own.** It was `while True`, and the only thing enforcing `max_rounds` was the budget check *inside* the default terminator. Turning the convergence check off used to keep that terminator, so the budget survived; selecting a different terminator — or none — removed it, and the run kept proposing past its configured budget. A component's opinion must not be the only thing between a config and an unbounded run, so the loop bounds itself and the terminator decides whether to stop *early*. **A near-miss optimization_type ended the run.** The Proposer returned `edit_method` where `edit_concrete_method` was valid; that check runs after the CodeAct loop, so raising bought no retry and unwound through the strategy after hours of work. Unusable improvements are now dropped with a warning, the same policy already applied to an unknown ancestor and to a failed build, and the batch still fails loudly when nothing in it is usable. Both were found by the end-to-end run, not the suite: the first needs a config that selects a non-default terminator, the second needs a model to make a plausible mistake. Regression tests for both. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): make the seams swappable in fact, not only in lookup A review found the acceptance test proved the wrong thing. Every assertion in it was `resolve(role, name) is not None`, so a role could resolve by name and then die on its constructor — and two did. `evaluation` was the worst: the runner built the evaluator *and every dataset* through a factory keyed on a hardcoded `{"harbor": (...)}`, so `evaluation: acme` passed the swap test and raised "Unsupported evaluator type" at run start. The component now declares its own `dataset_type` and `config_type`, and the factory resolves rather than looks up — which is what plan item 8 asked for. `ctx.component` did not supply the run-scoped arguments a component cannot know for itself, so `ctx.component("builder", "coder")` raised TypeError. It now passes the model tiers and the run root to whichever of them the constructor names. Named parameters only: a `**kwargs` on a component almost always means "forward to my base class", so treating it as permission hands an nooa Agent a `workspace` it rejects. Two more the same run turned up. `analyzer: null` skipped writing `round-N.md`, and that file is the strategy's only resume marker — a restart could not tell which rounds had finished, started from zero, and left every earlier cohort alive in the store to be re-selected. The train evaluation now goes with the analyzer that consumes it, which is what made `analyzer: null` claim to be cheaper. The acceptance test now constructs every default through the context and drives a swapped Builder to a committed Candidate. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): stop discarded candidates leaking, and removed config keys passing silently Three from the review. A rolled-back candidate was still projected to the platform as `survived`, because `update_candidate` mirrors on every write including the one that discards, and the status derivation had no branch for it. It also still reached the report and analysis prompts through `WorkspaceTool`, which globs the candidate store directly. Both now respect the same rule `list_candidates` does. `disable_convergence_check` and `disable_trajectory_scoring` were removed fields on a model with pydantic's default `extra="ignore"`, so an existing config carrying them ran full goal-tree scoring and early termination while reading as if it had turned both off. They now raise and name their replacement, like the `curator` and `models` renames before them. Same for `analyzer:`/`proposer:` when they hold a config block. And the fourth registry bug: a `monkeypatch.setattr` handed the object it was already holding records that object as "old" and restores it unchanged, so the test terminator stayed registered for every later test. Every one of these four has been invisible to the full suite and visible only in isolation or reverse order, so the no-leak invariant is now asserted rather than assumed. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): candidate identity survives serialization, and comments say less A Candidate round-tripped through JSON came back with an empty `id` while its label survived, because `id` is a computed field backed by a private attribute and `model_validate` ignores computed fields. Selection crosses exactly that boundary — survivors are the return value of an LLM method — so a model that returned reconstructed candidates would produce `survived = {""}`, and the round would mark every candidate killed, survivors included. `ExperimentRun` already had this validator; `Candidate` did not. The rest is comment economy, from a review that read every comment the branch adds. Two were worse than verbose: `roles.py` opened by saying five of the seven roles "are still constructed directly by the evolutionary strategy and become registry citizens in M2" — all five are declared and resolved in that same file. And `Builder.accepts` spent ten lines describing a resolution-time check that does not exist, on the premise that the Proposer is not a resolved component, which it is. Also: `EvolutionaryStrategy`'s class docstring is its system prompt, and it was spending it on host provenance and the runner's orchestration. The prompt guard that should have caught this only checked four literal strings, so it now bounds length and covers all seven Agent components. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the seams a review found still half-connected A verification pass over the whole branch. The finding that mattered: the out-of-tree example package — the milestone's own evidence that a third party can do this — could not build a candidate. `ctx.component` supplied the model tiers, the run root and the workspace, but not the evaluator or the dataset, so the Coder it returned raised on its first build. It now supplies every run-scoped argument a component cannot know for itself, and a test pins each one: deleting any single line from that mapping fails. Two roles were resolved by name and then ignored: `_generate_architecture_doc` instantiated the Coder directly, so `builder: acme-overlay` still ran our Coder's LLM against every baseline and wrote an architecture doc for a Builder that never asked for one. Documenting an artifact is now a Builder verb with a do-nothing default. And the goal tree was built and updated whenever *any* trajectory scorer was configured, though it is the built-in scorer's own data structure — a replacement paid two LLM passes per round for something it cannot read. Scorers declare whether they want one. `Proposer.produces` was documented as the Proposer/Builder compatibility declaration and read by nothing. A pairing that can never build now fails before the run starts rather than after `max_rounds` empty rounds. `slim()` empties trials so a candidate can go into a prompt, and the loop carried the selector's slim copies back into the population. Recording any later reward persisted one, wiping every channel's trials in the store — including the validation traces trajectory scoring reads, which then reported 0.0 for candidates that had them. The loop maps survivors back to the real records, and persisting a slim copy is now refused outright. This one predates the branch. Four tests could not fail, including two added by the earlier review round: `test_a_role_can_be_turned_off` asserted that pydantic returns what it was handed; the round-budget test asserted substrings of `inspect.getsource`. Both now drive the loop. Every fix here was checked by reverting it. Also: the proposer derived an artifact path from a display label, which the Candidate contract forbids; the strategy resolved its own model tiers rather than the run's, so `config_snapshot["deployment"]` described models it had not necessarily used; a re-scan kept stale import failures; ~35 lines of dead code; and one docstring where an in-place edit had left half of the old sentence behind. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): two of the previous commit's fixes were wrong Teaching `ctx.component` to supply the run's dataset made it supply `datasets[PRIMARY_SPLIT]`, which is `validation` — the split `holdout_utils` hides from the agent because the winner is chosen on it. A Builder runs bounded LLM repair loops against whatever dataset it is handed, so every Builder resolved through the seam was repairing against the held-out set, while the strategy's own `_new_builder` passed train. The seam now passes train too, and the test that asserted the old behaviour was pinning the leak rather than catching it. And gating the goal tree on a new `TrajectoryScorer.needs_goal_tree` did stop a replacement scorer paying for two LLM passes it cannot read — by also gating the only call site that constructs a scorer at all. `trajectory_scorer: acme-steps` became silently equivalent to `trajectory_scorer: null`: the role resolved and was never invoked. Reverted. The underlying complaint stands and is not fixed here: the goal-tree pipeline lives in the strategy, so a scorer is handed `GoalNode` objects whether or not it models goals. That is now stated on the role instead of papered over with a flag. It is the narrowest of the eight seams and moving the pipeline into the component is what would widen it. Also: `Coder.build` called `create_architecture_doc` directly, so a subclass overriding `describe` was honoured for the baseline and bypassed for every candidate; a selector choice outside the population was dropped in silence; the proposer read the architecture doc without the file-vs-directory normalization the context applies. Two more tests could not fail — the analyzer one asserted a substring of `inspect.getsource`, the same anti-pattern this branch already replaced once, and the terminator's round-budget test asserted pydantic defaults. Both now drive the loop, and each fix above was checked by reverting it. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * docs(experimentalist): say what the slim guard is worth, and pin describe The marker `update_candidate` refuses on is a private attribute, so it is lost through JSON — and the path it exists for, an LLM method returning candidates, is exactly the one that crosses that boundary. The docstring claimed a stronger invariant than the code has. It now says the guard is in-process only, and that this is why the loop looks survivors back up by id rather than relying on it. `Builder.describe` did not say who calls it when. The strategy calls it for the baseline; a Builder calls it for the candidates it builds, and one that never does produces candidates the Proposer reads as "(architecture.md missing)". `Coder.build` delegating to `self.describe` had no test, which for a hook whose only purpose is being overridden is the wrong way round. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one naming rule for config, one for roles Review found three inconsistencies that make the plugin contract harder to read than it needs to be. `evaluation:` selected a component while `evaluator:` configured it — two near-identical words for different things. Every role now follows one rule: the bare key names the component, `<role>_config` configures it. So `evaluator` becomes `evaluation_config`, `coder` becomes `builder_config` (it was a config named like a component, which is backwards), and `goal_config` becomes `trajectory_scorer_config`. All three are rejected with a migration error naming the replacement, like the keys removed earlier. Components declared their role four different ways; they now all use `roles.<Name>`, which distinguishes the role from `Agent` and from the same-named component class — there is both a `Proposer` component and a `roles.Proposer`. And `_evaluation()` returned `Any`, so callers read `dataset_type` off it unchecked. Making the role inherit `Evaluator` gives it one type that is both constructible and carries the ClassVars, rather than an implicit pairing a plugin author has to infer. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): every role declares its entry point, in terms only entities name Three of the eight roles declared a method; the other five declared nothing, so their real contracts lived implicitly in the evolutionary strategy's call sites. A plugin author had no way to learn the signature except by reading our loop — which is the difference between "swappable" and "swappable if you already know how it works". All eight now declare one, and the test for what each should say is M2's: would adding HPO force a change to a core file? Four of the five failed it. - `Terminator.run` took `EvolutionaryOptimizerConfig` and an `EvolutionTree`, so any strategy had to build ours to consult a terminator. It now takes the population and reads its own thresholds from `terminator_config`. `assess_round_budget` is gone: the budget is the loop's, and a component's opinion must not be the only thing bounding a run. - `Analyzer.run` took a display label — which the Candidate contract forbids deriving storage from — plus a platform client and workspace name, pushing the `ctx` escape hatch through a public role. It takes a Candidate; the handles arrive at construction. - `Proposer.run` took an `EvolutionTree` *and* a pre-rendered markdown table of the same data, plus an evolutionary-only `phase`. It takes candidates, renders its own table, and `hint` is a free-form steer any strategy can ignore. - `TrajectoryScorer.run` took `GoalNode` — the built-in's own data structure — so a replacement was handed goal-tree leaves whether or not it models goals. The goal tree is now the scorer's: ~190 lines of generation, refinement, loading and scoring moved out of the strategy, which went from three touchpoints to one call that hands over candidates and takes back reward records. `Evaluation` is the exception: it inherits `Evaluator`, so `run` is already declared and working. Restating it as a stub shadowed the real implementation in the MRO and broke every evaluator — caught by the suite, and the reason the role documents the inheritance rather than repeating it. No signature now names `EvolutionTree`, `GoalNode`, `EvolutionaryOptimizerConfig` or a platform client. `PRIMARY_SPLIT` moved to `seam.py`, where the components that need it can reach it without importing the implementation. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the analyzer was built without the run's platform handles Moving `client` and `nmp_workspace` off `Analyzer.run` is what got platform types out of the role's signature, but the construction site was never given them. The analyzer ran blind for a whole round: it could not load `intake://` traces, and it reported that as the *agent's* root cause — "the analyzer lacks the Intake client/workspace configuration required to load the count..." The Proposer acted on it and the Coder, with nothing in the agent to fix, committed a candidate identical to its ancestor. The whole suite passed while this was broken, because every analyzer test constructs the component itself and passes a client. Nothing asserted the strategy supplies one. The new test drives `_analyze_round` with a recording analyzer and pins what it is built with; re-introducing the omission fails it. Found by an end-to-end run, not by the suite. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit 1a81761db4809293e82d76c7fb3ed6ecc6ba186f) * fix(experimentalist): a Candidate's identity and its measurements stop being writable The reward record the trajectory scorer returns is keyed by candidate id, not by the Candidate: a Candidate is a pydantic model and unhashable, so `dict[Candidate, ...]` raised at the moment of return — after the round had paid for the scoring. Keying by id also matches the rule the rest of this branch follows. The suite passed while that was broken because the stub scorer returned `{}`, and an empty dict never exercises the key type. It returns a real record now, and there is a test on the built-in scorer, because the first fix was not enough: reverting the real line still passed, since the swap test only ever drove the stub. Which raised the larger question — twelve public mutable fields, and nothing but convention keeping them straight. Two are now enforced: Identity and provenance are frozen. `run_id`, `label`, `ancestor`, `generation`, `generated_from`, `description` and `artifact` are set once by `commit_candidate` and never again — verified: nothing in src wrote them post-construction, and `update_candidate` only ever receives `discarded` and `killed_generation`. Rewriting a candidate's lineage is now a ValidationError rather than a convention, which is the class of mistake behind five defects here. And `RewardMap` refuses `__setitem__`. A direct write mutates memory and is never persisted, so the candidate looks measured until the next reload — the same silent-loss shape as the `trajectory_detail` mutation this commit's first paragraph is about. `record_reward` remains the one writer and bypasses the guard on a single visible line. Fixtures that need a pre-scored candidate use `doubles.seed_reward`, which says in its name that nothing was persisted. Found by an end-to-end run with `trajectory_scorer: goal-tree`, which is the only configuration that reaches the scorer at all. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): tell the scorer which round its analysis describes The loop's counter has already advanced when the trajectory scorer is called, so passing it raw made the scorer name its state after the wrong round — a round-1 goal tree was written as `round-2-goal.json`. Nothing broke, because the scorer reads back the highest number it finds, but resume reads those files and the numbering was a lie. Caught by the first end-to-end run with `trajectory_scorer: goal-tree`, which is the only configuration that reaches the goal-tree pipeline at all — both smoke scenarios and the tau3 config set it to null. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): smoke-check the tasks a change claims to fix `integration_check` drew one task at random from the whole train split and never looked at `CodeChange.task_ids`, so it routinely verified a task the change does not touch. That matters because the check is not just a gate — it drives the Coder's repair loop: for attempt in range(max_fix_attempts + 1): evaluation = await self.run_smoke_eval(...) if healthy: return True await self._fix_runtime_issues(workdir, evaluation) Passing on an unrelated task returns True on attempt 0, so the repair never runs. Seen end to end: a candidate added an aggregation handler whose regex matched nothing, was smoke-checked against a lookup control that already passed, and shipped with both targeted tasks at 0. The Coder had two repair attempts it never used. It checks the targeted tasks now, falling back to the old single random task when a Proposal names none. Not a correctness fix — the loop still evaluated that candidate on validation and the selector correctly kept the baseline — but a wasted round and a wasted self-repair. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the run's metric contract must reach the components `objective_function` and `regression_metrics` were defined and validated on the run config and then read by nothing. Every component fell back to its own default of "maximize reward", so a run that configured an objective was silently optimizing a different one -- and it still completed, reported, and picked a winner, which is why the merge did not surface it. Threads both into the selector, proposer and terminator at construction, keeping role signatures free of strategy config, and gives the selector the finalization contract main added in #1195 -- ported rather than adopted, because M1 had already split the question in two. Survivor selection is an LLM judgement told to favour new blood; `winner` is arithmetic that must be able to answer "nothing here beat doing nothing". Main routed both through the former, which is the bug #1195 fixed; here they were already separate methods, so `winner` only had to gain the rules: - the generation-0 baseline is re-admitted even if survivor selection killed it, and anchors the regression comparison even when it does not win - a candidate that worsens a protected metric is dropped before ranking, so a gain on the objectives cannot pay for a regression - ranking projects through `pareto_objectives`, so a minimized target is not ranked backwards and a protected metric does not sway the front - ties go to the oldest candidate: equal score means the diff bought nothing `rank` gets the same projection; it had the same direction bug one level up, where it decides which candidates breed. Main's `test_winner_selection.py` is ported onto `Selector.winner` and kept whole -- 26 cases, each verified to fail when the behaviour it names is reverted -- plus a new test naming each construction site, since the wiring was exactly what nothing covered. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): stop stubbing a method that no longer exists `test_role_swap` silenced `_generate_initial_goal_tree` in four places. That method moved into the trajectory-scorer during this refactor and no longer exists on the strategy, so those lines set an attribute nothing reads -- silencing nothing while reading as though a model call were being suppressed. The remaining stubs assigned one `-> dict[str, Any]` function to five methods returning `None`, `str` and `dict[str, EvaluationResult]`. Split into one stub per return type, and set through `monkeypatch.setattr` so the type checker sees the assignment and pytest undoes it. Type diagnostics for the file: 31 to 1. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): say what each removed config key became, and cover them all The rejection for a removed key reused one sentence for two different migrations, so a rename was explained with "turning a step off is how you choose no implementation of it" -- advice that does not apply to it. Split into the three cases that actually exist: a `disable_*` flag becomes `<role>: null`, a config block moves under `<role>_config` while the bare key starts naming the component, and a role renamed outright takes both its name and its config key with it. The guard also only fired on a dict, so `evaluator: harbor` -- a plausible thing to write, and no longer a field -- was accepted and ignored. Keys that are not fields at all are now rejected whatever the value. Only `curator` had a test. The config tolerates unknown keys, so every other removal was silently accepted and the run then did something other than what the file said, with `config_snapshot` recording it as correct. All nine are covered, including the half that is easy to break while fixing the other: a *string* under `analyzer` or `proposer` still selects the component. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): a repeated optimization_type is not a malformed batch #1163 removed this raise on main -- "repeated `optimization_type` is fine" -- and the merge kept the pre-#1163 lines while also taking #1163's duplicate-text warning, so the validator ended up a mixture of both versions. The raise contradicts the reasoning two methods above it, which argues that these checks run after the CodeAct loop, that raising buys no retry, and that one bad proposal is not a bad round. There are twenty optimization types and two genuinely different edits to the same method share one, so it is not evidence of a malformed batch; duplicate optimization *text* is, and is already dropped. Found by an end-to-end run: the generalization scenario died at `Proposer returned duplicate optimization_type 'edit_concrete_method'` after paying for a baseline evaluation and an analysis. Nothing covered the raise -- removing it broke no test -- which is how the mixture survived the merge, so both halves are pinned now. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit aa4afa721c26d88c29e6100e97f877243dc1ee67) * refactor(experimentalist)!: name each component for its implementation, and the evaluator for what it measures Two naming inconsistencies from review, both in the same rule. **`coder` -> `llm-code-edit`.** Eight of nine components name an implementation -- by method (`evolutionary`, `pareto-llm-diversity`), evidence (`agent-trace`), structure (`goal-tree`), backend (`harbor`), output (`code-change`) or operation (`import`). `coder` named an *agent* instead, which is what the role already does, and it sat next to `import` under the same role spelled the other way. The class stays `Coder`; only the registered name changes. **`evaluation` -> `outcome-evaluator`.** The role was named for what the step produces rather than the thing that does it, leaving it the one role that was not an agent-noun, and "Evaluation" said nothing about *which* evaluator it is. What makes it specific is already in its own docstring: it measures the outcome, where the trajectory-scorer measures the process of the same run. `evaluator` alone was not available -- the role inherits `Evaluator`, and one word for both is the confusion the `<role>` / `<role>_config` rule exists to remove; `outcome-evaluator` keeps them apart and matches the `root-cause-analyzer` precedent. So `evaluation:` becomes `outcome_evaluator:`, `evaluation_config:` becomes `outcome_evaluator_config:`, and `ctx.evaluation` becomes `ctx.outcome_evaluator`. Both old keys are rejected with a migration error naming the replacement, as with every key removed before, and both are covered. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): restore three main-side changes the merge had reverted Found by auditing every main commit against our tree rather than by another dead run. The merge combined two change sets in 42 files, and in three places it kept ours and dropped theirs: **#1163's allowlist fix.** `allowed_types = set(available_types) or all_types` makes a type forbidden the moment it is used, because `available_types` is `all_types - tried_types`. Christian removed exactly this line five days ago and we brought it back. It killed a real multi-round run: rounds 1 and 2 took the agent from 0.333 to 1.000 with `edit_concrete_method`, round 3 proposed refining it, the only proposal was rejected as unusable, and the run raised and died -- precisely when the loop was succeeding. Now `all_types`, with novelty left as the prompt-side preference it was always documented to be. **#1159's litellm removal.** `_LiteLLMModule`, `_enable_litellm_drop_params` and its test came back. Models resolve through the platform now, so the shim was dead code that still ran an import on every run. Main carries no reference to it. **#1107's insight-channel reads.** Four survived: a summary line, two prompt code examples, and an "Insight Suite Reward" table with instructions on when to render it. Nothing has recorded that channel since insight promotion was deleted -- zero writers, four readers -- so the table could never have data, and the instruction was paying tokens in an Agent docstring to describe it. The audit script is /tmp/merge_audit2.py: for each main commit, check whether the lines it added are present and the lines it deleted are absent, per file, restricted to the files both sides touched. The four remaining hits are accounted for -- `_validate_improvements` is this branch's deliberate split of `_filter_improvements`, and the rest are docstring wording. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit 651d45d3fe28fdbb0e028025737abfec4ef14851) * fix(experimentalist): restore the run-config values the refactor disconnected Review found five places where a value the run config declares reached nobody. Each fails the same silent way: the run completes, reports, and picks a winner, against settings other than the ones the file states, while `config_snapshot` records it as correct. **Insight suites reached nothing (P1).** In Mode 1 the Eval Author writes verifiers that emit their own metric keys, plus a suite of production-trace tasks. Neither was used: `with_insight_objective` had no caller outside a test, so selection kept ranking on `reward` — which those verifiers never emit, leaving no candidate eligible to win — and the suite was parked in `datasets["insight"]`, a key nothing read, so its tasks were never evaluated. The host now applies the authored keys as the objective, demoting the configured targets to guardrails, and distributes the suite into the splits the loop evaluates, bracketed by the held-out restore/hide pair. Because the strategy is constructed before the Eval Author runs, it cannot read this off its own config. The effective contract now travels on the context, which is where the host settles it, and `StrategyContext` declares it. **The terminator judged convergence from a partial history (P1).** It received the round's working set — survivors plus the previous round's builds — while convergence counts distinct generations and compares an older Pareto front against the current one. Eliminated candidates were invisible, so with `max_survivors=1` the generation window could never fill and a converged run paid for every remaining round. It now receives the whole evolution tree. **Two values had two homes (P2).** `min_rounds_before_stopping` and `max_trajectory_tasks` existed on the run config *and* on the component config, and the components read their own copy — so the shipped benchmark configs set values that did nothing. Each now has one home, on the run config beside `max_rounds`, and reaches its component the way the objective and regression targets already do. **Reporting fell back to `reward` (P2).** `ExperimentContext` was built without the objective list, so progress lines showed configured metrics as unavailable and `run_finished` omitted the winner's actual scores. Every fix is pinned by a test verified to fail when the fix is reverted. The first attempt at those tests caught only the P2s, which is the same gap that let these through: asserting a component stores what it was handed says nothing about whether the loop hands it over. The terminator and Insight tests now drive the real call site. Signed-off-by: Severin Klingler <sk…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes the Analyst, Eval Author, and Experimentalist use the models selected through
nemo setup. It replaces plugin-specific endpoints, credentials, hardcoded model names, and Experimentalist's three-tier model configuration with one Platform-native pair:default_modelfor quality-oriented work andfast_modelfor latency-oriented work, with fast falling back to default.Before this change, each optimization plugin configured inference independently. Setup-selected models and credentials in Platform Secrets were not reused, so OpenAI-compatible providers required manual plugin configuration and Anthropic could not be selected in setup and consumed through its native Messages contract.
Changes
Platform-native model selection
nemo setupnow stores two workspace-qualified Model Entity references in the active CLI context:Shared logic in
nemo_platform_pluginreads the pair, resolves each entity through the existing Platform SDK, uses its actual served model name/provider route/backend format/Platform headers, constructs the corresponding Nooa/LiteLLM client, and activates those clients for the agent run.Backend selection comes from the Model Entity rather than model-name heuristics:
OPENAI_CHATuses the OpenAI-compatible Chat Completions contract.ANTHROPIC_MESSAGESuses Anthropic's native Messages contract.The plugins do not translate request or response bodies. Provider credentials stay in Platform Secrets and are never passed into the optimization agents.
Consistent agent roles
default_modelis the existing quality-oriented default and replaces the previous smart role.fast_modelis used only for deliberately latency-sensitive or high-volume work, such as summarization and control steps.Experimentalist's checked-in
models.yamlremains separate: it controls model mutations for the agent under test, not the models running the optimizer itself. Application and benchmark credentials such asAUT_MODEL_NAME,OPENAI_API_KEY, andOPENAI_BASE_URLmay therefore still be required by the evaluated workload.Setup and compatibility
Interactive setup asks for a default and fast model. The fast prompt defaults to the chosen default, so users who want one model can press Enter twice without understanding the internal role mapping.
nemo setup --autopreserves main's behavior:NEMO_DEFAULT_MODELwhen set; otherwise select the alphabetically first discovered Model Entity.NEMO_FAST_MODELwhen set; otherwise reuse the effective default.Existing contexts that only contain
default_modelremain compatible because fast automatically falls back to default.For CI, containers, and other environments that cannot read the host CLI context, the pair can be overridden with Model Entity references:
The referenced entities and providers must already exist on the target Platform.
Removed optimizer-specific configuration
The optimization agents no longer consume:
AUTHOR_API_BASE,AUTHOR_API_KEY, orAUTHOR_*_MODEL_NAMENEMO_EXPERIMENTALIST_API_BASEorNEMO_EXPERIMENTALIST_API_KEYNEMO_EXPERIMENTALIST_MODELS_SMART,NEMO_EXPERIMENTALIST_MODELS_MID, orNEMO_EXPERIMENTALIST_MODELS_FASTINFERENCE_API_KEYas an optimization-agent credentialScheduled Analyst runs
nemo insights analysis enablenow persists the effective pair in the server-side analysis config. The controller does not need access to the operator's local~/.config/nmp/config.yaml. Re-runanalysis enableafter changing the pair; previously enabled records must also be re-enabled once to capture it.Anthropic setup's
auth_header_formatfix is intentionally isolated in #1158.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run --frozen pytest packages/nemo_platform_ext/tests/cli/commands/test_setup.py -q— 229 passed.uv run --frozen pytest sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.py -q— 229 passed.ty: passed.ff047d2a57) — preserved the new SDK/bootstrap/mise documentation and the model-pair setup text; relevant file-level pre-commit hooks passed.uv run pre-commit run -aafter the latest main sync — Ruff, formatting,ty, config docs, Helm docs, copyright, UI, import-boundary, and merge-conflict checks passed.uv-lock-checkis blocked becauseuv lockadds newly published wheel metadata despite no dependency-input change; that generated drift is excluded, and the committed lock resolves successfully withuv run --frozen.make docs-check— passed; 216 MDX files parsed cleanly and no published-to-gated links found.make docs-broken-links— command exited successfully but reported five existing broken links in unchanged pages (telemetry-and-privacy.mdx,using-authentication.mdx, andstudio/index.mdx). None are in this pull request's changed documentation.Live provider validation:
default/openai-openai-gpt-5-5) and created two Insights from 19 observed sessions. Separate Analyst runs completed with native Anthropic models and with an OpenAI default + Anthropic fast pair.default/gpt-5-minifast path.Summary by CodeRabbit
New Features
Documentation
Bug Fixes