Repository navigation
refactor(eval-author): simplify dataset augmentation - #1107
Conversation
|
|
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:
📝 WalkthroughWalkthroughEval Author now stages and validates train, validation, and Insight inputs, authors shared metrics, and returns metric metadata. Experimentalist now applies objective and regression contracts throughout optimization instead of scalar Insight evaluation and promotion. ChangesEval Author metric handoff
Experimentalist multi-objective optimization
Sequence Diagram(s)sequenceDiagram
participant Caller
participant run_eval_author
participant DatasetFactory
participant EvalAuthor
participant EvolutionaryOptimizer
participant RunReporter
Caller->>run_eval_author: provide Eval Author inputs
run_eval_author->>DatasetFactory: parse staged datasets
run_eval_author->>EvalAuthor: pass train, validation, and Insight datasets
EvalAuthor-->>Caller: return authored metrics and staged datasets
EvolutionaryOptimizer->>EvolutionaryOptimizer: build objective and regression contracts
EvolutionaryOptimizer->>RunReporter: report metric mappings and objective scores
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.py (1)
84-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude URI resolution in the accumulated-failure path.
local_path_from_uriruns outside thetry. A non-local task URI raises immediately and discards all failures collected so far. Move it inside the guarded block so the caller sees the full report.♻️ Proposed change
- task_dir = local_path_from_uri( - task.uri, - context=f"{dataset_name} dataset task {task.id!r}", - ).resolve() try: + task_dir = local_path_from_uri( + task.uri, + context=f"{dataset_name} dataset task {task.id!r}", + ).resolve() verifier_dir = _verifier_dir(task_dir) except (FileNotFoundError, ValueError, tomllib.TOMLDecodeError) as exc: failures.append(f"{dataset_name}/{task.id}: {exc}") continue🤖 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-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.py` around lines 84 - 92, Move the task.uri resolution and .resolve() logic into the existing try block surrounding _verifier_dir(task_dir), so local_path_from_uri failures are caught by the same exception handler. Preserve the current failures.append and continue behavior to retain accumulated failures and continue processing later tasks.plugins/nemo-eval-author/tests/test_eval_author_agent.py (1)
263-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
raising=Falseforvalidate_metric_contracts.
agent.pyimports the symbol at module scope, so it exists. Withraising=False, a future rename or removal silently leaves the real validator unpatched and the test still passes.♻️ Proposed change
- monkeypatch.setattr(eval_author_module, "validate_metric_contracts", validate_metric_contracts, raising=False) + monkeypatch.setattr(eval_author_module, "validate_metric_contracts", validate_metric_contracts)🤖 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-eval-author/tests/test_eval_author_agent.py` at line 263, Update the monkeypatch call for validate_metric_contracts in the test to remove raising=False, ensuring the patch fails if the module-level symbol is renamed or removed.
🤖 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 `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/models.py`:
- Around line 93-96: Update the validation in the model’s identity-checking path
around insight_suite_identity to require the exact sha256: prefix followed by 64
hexadecimal characters, while retaining the existing required/non-empty
behavior. Reject identities with an invalid prefix, incorrect length, or
non-hexadecimal characters, and add tests covering each case.
---
Nitpick comments:
In
`@plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.py`:
- Around line 84-92: Move the task.uri resolution and .resolve() logic into the
existing try block surrounding _verifier_dir(task_dir), so local_path_from_uri
failures are caught by the same exception handler. Preserve the current
failures.append and continue behavior to retain accumulated failures and
continue processing later tasks.
In `@plugins/nemo-eval-author/tests/test_eval_author_agent.py`:
- Line 263: Update the monkeypatch call for validate_metric_contracts in the
test to remove raising=False, ensuring the patch fails if the module-level
symbol is renamed or removed.
🪄 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: cc3bd9bb-1a36-4b6e-9482-2ee8713a6ece
📒 Files selected for processing (23)
plugins/nemo-eval-author/README.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/README.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/REFERENCE.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/models.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/tests/test_eval_author_agent.pyplugins/nemo-eval-author/tests/test_eval_author_materialization.pyplugins/nemo-eval-author/tests/test_eval_author_models.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_plugin_boundary.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/insight_promotion.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_helpers.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_model_config.pyplugins/nemo-experimentalist/tests/test_experiment_mirror.pyplugins/nemo-experimentalist/tests/test_experimentalist_backend.py
💤 Files with no reviewable changes (3)
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_reporting.py
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/insight_promotion.py
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
plugins/nemo-experimentalist/tests/experimentalist/test_reporting.py (1)
51-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the decline marker and the
n/apath.Every case here renders
▲. Two branches incandidate_evaluatedstay untested: the▼marker when an objective moves against its direction, andf"{target.name} n/a"when the evaluator omits an objective metric. The second branch is the one that signals a broken metric contract, so it deserves a test.🤖 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-experimentalist/tests/experimentalist/test_reporting.py` around lines 51 - 72, The test test_candidate_evaluated_shows_all_objectives_and_direction_aware_deltas currently covers only ▲ deltas; extend coverage for candidate_evaluated to verify a directionally worsening objective renders ▼ and an omitted objective metric renders the exact “{target.name} n/a” output. Include assertions for both branches while preserving the existing objective formatting checks.plugins/nemo-experimentalist/tests/test_metric_contract.py (1)
103-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the name-collision and empty-key branches of
_with_insight_objective.Two branches stay untested. First, an authored Insight key that matches a configured regression or objective name: the helper drops it from
regression_by_nameto avoid the overlapValueErrorraised byvalidate_metric_contract. Second, an emptymetric_keystuple, which returns the config unchanged. Both are cheap to assert and both guard against a run-time validation failure.🤖 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-experimentalist/tests/test_metric_contract.py` around lines 103 - 118, Add tests for _with_insight_objective covering an authored Insight key that collides with an existing objective or regression metric, asserting the resulting configuration passes validation without duplicate names and preserves the intended metric placement. Add a separate test for an empty metric_keys tuple, asserting the helper returns the configuration unchanged.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
from __future__ import annotations.Use concrete annotations to follow the plugin convention. All referenced types are imported at module scope.
nooaresolves deferred annotations withtyping.get_type_hints, so this is a consistency refactor, not a schema-generation fix.🤖 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-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.py` at line 6, Remove the from __future__ import annotations statement from selector.py, leaving the existing concrete type annotations and module-level imports unchanged.Source: Coding guidelines
🤖 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 `@plugins/nemo-experimentalist/README.md`:
- Around line 157-172: Split the combined YAML example in the documentation into
two separate fenced YAML blocks, each with its own descriptive introductory
text. Keep the single-metric example and the multi-metric example as
independent, valid YAML documents without duplicate objective_function keys.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.py`:
- Around line 417-424: Update the peer-comparison flow around
`_select_divergent_pairs` and `_find_complementary_failures` to accept and apply
each metric’s direction contract before computing deltas or selecting leaders,
so minimize metrics consistently treat lower values as better. Propagate the
metric definitions from the caller through both computations and add coverage
for a mixed maximize/minimize comparison.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`:
- Around line 1205-1220: Update the task import flow around _from_task_dir to
validate the staged task before renaming over an existing destination, and
retain the backup until reloading the replacement destination succeeds. Restore
the backup and clean up staging if validation or reload fails, preserving the
original task. Add a test covering replacement with malformed task.toml.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py`:
- Around line 1418-1442: Update the candidate filtering flow around eligible,
excluded, and ranked to handle the case where every candidate is excluded:
either fail fast with an explicit error or fall back to the original candidates
so the round cannot proceed with zero survivors. Build the exclusion list by
comparing candidate labels against the labels of eligible candidates, rather
than comparing Pydantic model instances by value.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/terminator.py`:
- Around line 196-200: Update the call to qualitative_stop_check to pass the
resolved active_objectives and active_regressions values instead of
objective_metrics and regression_metrics, preserving the fallback behavior
established when those active lists are resolved.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.py`:
- Line 6: Remove the from __future__ import annotations statement from
selector.py, leaving the existing concrete type annotations and module-level
imports unchanged.
In `@plugins/nemo-experimentalist/tests/experimentalist/test_reporting.py`:
- Around line 51-72: The test
test_candidate_evaluated_shows_all_objectives_and_direction_aware_deltas
currently covers only ▲ deltas; extend coverage for candidate_evaluated to
verify a directionally worsening objective renders ▼ and an omitted objective
metric renders the exact “{target.name} n/a” output. Include assertions for both
branches while preserving the existing objective formatting checks.
In `@plugins/nemo-experimentalist/tests/test_metric_contract.py`:
- Around line 103-118: Add tests for _with_insight_objective covering an
authored Insight key that collides with an existing objective or regression
metric, asserting the resulting configuration passes validation without
duplicate names and preserves the intended metric placement. Add a separate test
for an empty metric_keys tuple, asserting the helper returns the configuration
unchanged.
🪄 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: 827baef6-02cc-454c-8a52-097573fa0d87
📒 Files selected for processing (22)
plugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/examples/tau3-nooa-agent/.env.exampleplugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cache.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/dataset_staging.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.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/experimentalist_backend.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_terminator.pyplugins/nemo-experimentalist/tests/test_experimentalist_analyzer.pyplugins/nemo-experimentalist/tests/test_metric_contract.py
💤 Files with no reviewable changes (1)
- plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugins/nemo-eval-author/tests/test_eval_author_agent.py (1)
263-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
raising=False.
agent.pyimportsvalidate_metric_contracts, so the attribute exists. Withraising=False, a future rename silently creates a dead patch and the tests keep passing against unpatched code.🔧 Proposed fix
- monkeypatch.setattr(eval_author_module, "validate_metric_contracts", validate_metric_contracts, raising=False) + monkeypatch.setattr(eval_author_module, "validate_metric_contracts", validate_metric_contracts)🤖 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-eval-author/tests/test_eval_author_agent.py` at line 263, Update the monkeypatch in the test around validate_metric_contracts to remove raising=False, relying on the imported validate_metric_contracts attribute being present so renamed symbols fail loudly instead of silently creating an ineffective patch.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.py (1)
189-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the string-based return annotation.
Use
typing.Selfinstead of"EvolutionaryOptimizerConfig".♻️ Proposed change
`@model_validator`(mode="after") - def validate_metric_contract(self) -> "EvolutionaryOptimizerConfig": + def validate_metric_contract(self) -> Self:Add
Selfto the typing import at line 23:-from typing import Any, Literal +from typing import Any, Literal, SelfAs per coding guidelines: "Prefer concrete type hints over string-based type hints".
🤖 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-experimentalist/src/nemo_experimentalist_plugin/config.py` around lines 189 - 190, Update the return annotation of EvolutionaryOptimizerConfig.validate_metric_contract to use typing.Self instead of the quoted class name, and add Self to the existing typing imports.Source: Coding guidelines
plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py (1)
555-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not require exactly one authored metric key.
author_insight_metricsmay author several keys. Line 557 fails the canary when the model authors two. Compare every authored key instead.♻️ Proposed change
- assert len(violating_metric_names) == 1 - assert compliant_metric_names == violating_metric_names - metric_name = next(iter(violating_metric_names)) - violating_score = violating_payload[metric_name] - compliant_score = compliant_payload[metric_name] - assert isinstance(violating_score, int | float) and not isinstance(violating_score, bool) - assert isinstance(compliant_score, int | float) and not isinstance(compliant_score, bool) - assert 0.0 <= violating_score <= 1.0 - assert 0.0 <= compliant_score <= 1.0 - assert compliant_score > violating_score + assert violating_metric_names + assert compliant_metric_names == violating_metric_names + for metric_name in violating_metric_names: + violating_score = violating_payload[metric_name] + compliant_score = compliant_payload[metric_name] + assert isinstance(violating_score, int | float) and not isinstance(violating_score, bool) + assert isinstance(compliant_score, int | float) and not isinstance(compliant_score, bool) + assert 0.0 <= violating_score <= 1.0 + assert 0.0 <= compliant_score <= 1.0 + assert any( + compliant_payload[name] > violating_payload[name] for name in violating_metric_names + )🤖 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-eval-author/tests/test_eval_author_repair_e2e.py` around lines 555 - 566, Update the assertions in the evaluation comparison around violating_metric_names and compliant_metric_names so multiple authored metric keys are supported instead of requiring exactly one. Compare every authored key present in both payloads, validate each violating_score and compliant_score as numeric values in [0, 1], and require compliant_score > violating_score for every key while preserving reward exclusion.
🤖 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 `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/models.py`:
- Line 96: Update EvalAuthorResult’s metric_keys validation around
_validate_metric_keys to assign and return the normalized tuple rather than
discarding it. Implement this as a field validator that skips the empty default,
preserving empty defaults while stripping whitespace from supplied keys
consistently with MetricAuthoringResult.
In `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.py`:
- Around line 83-122: Update satisfies_regression_constraints and
is_eligible_for_metric_contract to handle a baseline_metrics mapping that lacks
declared regression targets before comparing candidate values. Choose and
implement an explicit failure or documented fallback policy so missing round-0
regression metrics cannot silently reject every candidate, while preserving the
existing constraint comparisons when baseline values are present.
---
Nitpick comments:
In `@plugins/nemo-eval-author/tests/test_eval_author_agent.py`:
- Line 263: Update the monkeypatch in the test around validate_metric_contracts
to remove raising=False, relying on the imported validate_metric_contracts
attribute being present so renamed symbols fail loudly instead of silently
creating an ineffective patch.
In `@plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py`:
- Around line 555-566: Update the assertions in the evaluation comparison around
violating_metric_names and compliant_metric_names so multiple authored metric
keys are supported instead of requiring exactly one. Compare every authored key
present in both payloads, validate each violating_score and compliant_score as
numeric values in [0, 1], and require compliant_score > violating_score for
every key while preserving reward exclusion.
In `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.py`:
- Around line 189-190: Update the return annotation of
EvolutionaryOptimizerConfig.validate_metric_contract to use typing.Self instead
of the quoted class name, and add Self to the existing typing imports.
🪄 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: f7a29ec0-5340-4368-8879-387dc0a453d9
📒 Files selected for processing (42)
plugins/nemo-eval-author/README.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/README.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/REFERENCE.mdplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/models.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/tests/test_eval_author_agent.pyplugins/nemo-eval-author/tests/test_eval_author_materialization.pyplugins/nemo-eval-author/tests/test_eval_author_models.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_plugin_boundary.pyplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/examples/tau3-nooa-agent/.env.exampleplugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/config.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cache.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/dataset_staging.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/insight_promotion.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.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/experimentalist_backend.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_helpers.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_model_config.pyplugins/nemo-experimentalist/tests/experimentalist/test_reporting.pyplugins/nemo-experimentalist/tests/experimentalist/test_terminator.pyplugins/nemo-experimentalist/tests/test_experiment_mirror.pyplugins/nemo-experimentalist/tests/test_experimentalist_analyzer.pyplugins/nemo-experimentalist/tests/test_experimentalist_backend.pyplugins/nemo-experimentalist/tests/test_metric_contract.py
💤 Files with no reviewable changes (4)
- plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_reporting.py
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/insight_promotion.py
🚧 Files skipped from review as they are similar to previous changes (30)
- plugins/nemo-eval-author/tests/test_plugin_boundary.py
- plugins/nemo-experimentalist/README.md
- plugins/nemo-eval-author/README.md
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/README.md
- plugins/nemo-experimentalist/tests/experimentalist/test_model_config.py
- plugins/nemo-experimentalist/tests/experimentalist/test_terminator.py
- plugins/nemo-experimentalist/tests/test_experimentalist_backend.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experimentalist_backend.py
- plugins/nemo-experimentalist/tests/test_experiment_mirror.py
- plugins/nemo-experimentalist/tests/experimentalist/test_reporting.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/REFERENCE.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/dataset_staging.py
- plugins/nemo-experimentalist/tests/test_experimentalist_analyzer.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/materialization.py
- plugins/nemo-eval-author/tests/test_eval_author_models.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
- plugins/nemo-experimentalist/tests/test_metric_contract.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cache.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_analyzer.py
- plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/reporting.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/terminator.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/agent.py
- plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/selector.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py
4fe22f6 to
1a657e7
Compare
Introduce a CLI-safe request/result boundary and immutable reference inventory so verifier authoring can be consumed independently of Experimentalist datasets. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Remove inferred snapshot and inventory machinery so metric authoring explicitly produces the reusable files alongside the generated tasks. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Restore per-task verifier authoring across staged train and validation datasets, and apply the same metrics to the generated Insight task set. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Preserve the original augmentation contract so Experimentalist can continue using the staged Dataset objects without an artifact rehydration layer. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Leave the authored Insight suite at the Eval Author handoff boundary so the Experimentalist team can define splitting, evaluation, and reward integration without legacy channel behavior. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Remove stale claims that train and validation remain unchanged or that Experimentalist immediately evaluates the authored Insight suite. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
Store normalized metric_keys on EvalAuthorResult, accumulate URI failures in contract validation, and harden related unit/canary asserts. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Signed-off-by: Gaia Di Lorenzo <75574163+gaiadilorenzo@users.noreply.github.com>
Signed-off-by: Gaia Di Lorenzo <gdilorenzo@ethz.ch>
1a657e7 to
d125c90
Compare
…ortable-verifier-patch/akhoury Signed-off-by: Alec Khoury <akhoury@nvidia.com> # Conflicts: # plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py
…ortable-verifier-patch/akhoury Signed-off-by: Alec Khoury <akhoury@nvidia.com>
…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…
Summary
Refactors Eval Author’s portable metric handoff and moves Experimentalist to metric-contract-driven optimization. Regression metrics are selector context rather than a hard eligibility gate, while candidates must still report every Pareto objective.
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses — blocked by 38 existing type-check diagnostics from rebasedmainfiles outside this PR.Targeted validation:
.venv/bin/pytest plugins/nemo-experimentalist/tests/test_metric_contract.py plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging.py plugins/nemo-experimentalist/tests/test_experimentalist_analyzer.py plugins/nemo-experimentalist/tests/experimentalist/test_terminator.py -v— 32 passed..venv/bin/pytest plugins/nemo-experimentalist/tests/test_experimentalist_backend.py -v— 19 passed.uv run generate-config-docs && git diff --exit-code docs/set-up/config-reference.mdx— passed.