Repository navigation
fix(experimentalist): stop discarding usable work in the optimization loop - #1163
Merged
Merged
Conversation
… the run A proposal round is expensive and everything before it is already paid for, but three separate conditions discarded the whole round rather than the offending improvement. The strictest was a requirement that every improvement in a round carry a distinct optimization_type. Nothing justifies it: a round that wants to edit two different methods is a perfectly good round, and forbidding it also meant a type used once could not be used again later in the run. Two real runs died on it after round 1, having already produced usable candidates. Surplus improvements, a type outside the allowed set, and duplicate optimization text now drop the individual improvement and log why. Only an empty result is fatal, because there is then nothing to build. Callers still pass the available types as a prompt hint, but no longer as a constraint that outlives the round. Adds the first tests for this validation, which had none. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
…turns GroupLeafScorer requires every score to cite span IDs, and the only sanctioned lookups -- get_span_id and get_turn_data -- are indexed by turn. An agent that makes no LLM calls produces a trace with zero turns, so both return None however often they are called. The scorer could not satisfy its own contract, retried to the CodeAct iteration ceiling, and raised GenerationError; because scoring runs inside asyncio.gather with no handler above it, that ended the whole run -- discarding candidates that had already been evaluated successfully. Nothing was wrong with the evidence. A turn-less trace still carries a full call graph: which methods ran, in what order, and their status, which is enough to tell a broken candidate from a fixed one. The scorer could see it; it just had no permitted way to cite it, since the ids visible in the overview text are abbreviated and explicitly disallowed. The contract now names the observable condition (`Turns: 0`), says to leave span_ids empty, and points at the call graph as the grounding to use instead. Guessed or abbreviated ids are still refused, and traces that do have turns are unaffected. Verified on a run against a deterministic agent: the scorer completed and separated two candidates that were tied on reward, citing method names and statuses. Not specific to one fixture -- any deterministic, rule-based, or purely tool-driven agent produces turn-less traces and hit the same dead end. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py`:
- Around line 199-205: Update the proposal filtering flow in the proposer method
so every returned improvement is validated for allowed content and duplicates
before applying the max_candidates limit. Truncate only the resulting usable
list, preserving the existing warning as appropriate, and ensure the method
raises only when no valid improvements remain.
In
`@plugins/nemo-experimentalist/tests/experimentalist/test_trace_scorer_contract.py`:
- Around line 43-46: Strengthen test_contract_offers_an_alternative_grounding by
asserting that the contract includes requirements for method names, execution
order, and statuses in addition to the call graph. Ensure the assertions verify
each required zero-turn evidence constraint so removing any one of them causes
the test to fail.
🪄 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: 035f20a5-869c-457c-a1ba-4d1bcfb3ebff
📒 Files selected for processing (4)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_scorer.pyplugins/nemo-experimentalist/tests/experimentalist/test_proposer_validation.pyplugins/nemo-experimentalist/tests/experimentalist/test_trace_scorer_contract.py
Contributor
|
gaiadilorenzo
approved these changes
Aug 7, 2026
_filter_improvements truncated to max_candidates before checking whether the improvements it kept were usable. When the improvements past the cut were the only valid ones, the round raised "none of them usable" while holding work it could have built on -- the failure this PR set out to remove. The cut now falls on the kept list. Also trims the history out of three docstrings, leaving what the reader needs to use the code, and pins the whole of the turn-less evidence contract (methods, order, status) rather than just "call graph" -- a prompt that dropped the other three still passed. Review feedback from @coderabbitai and @gaiadilorenzo on #1163. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian Schüller <cschueller@nvidia.com>
Contributor
📝 WalkthroughWalkthroughProposal filtering now drops unusable candidates, allows repeated optimization types, and updates prompt guidance. Trace scoring now defines handling for traces without LLM turns, including empty evidence span IDs. ChangesProposal filtering
Trace scoring contract
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py`:
- Around line 163-170: Update the generation flow around _run_with_context so
available_types is treated as a prompt preference rather than a hard
restriction, allowing previously used optimization types when appropriate.
Preserve validation against all declared types and keep _filter_improvements
configured with all_types for that validation.
🪄 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: e2bceb00-69af-4479-bda6-5c5181cf9895
📒 Files selected for processing (4)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_scorer.pyplugins/nemo-experimentalist/tests/experimentalist/test_proposer_validation.pyplugins/nemo-experimentalist/tests/experimentalist/test_trace_scorer_contract.py
schuellc-nvidia
enabled auto-merge
August 7, 2026 14:14
schuellc-nvidia
disabled auto-merge
August 7, 2026 14:14
Relaxing _filter_improvements only stopped the run from dying on a repeated optimization_type. The prompt still said each one MUST be in `available_types` -- the untried set -- so while any type was untried the model was told never to propose a reused one, and the allowance below it was unreachable. The comment claiming novelty was "a preference" was describing the validator, not the instruction the model actually follows. The pick is now legal from either set, preferred from the untried one, in the MANDATORY block and in the two other places that restricted it. test_proposer_contract.py pins the prompt half so it cannot drift back out of agreement with the validator half. Review feedback from @coderabbitai on #1163. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Christian Schüller <cschueller@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py (2)
204-209: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop empty normalized optimization text.
Line 204 strips the text, but Lines 205-209 keep
description == "". A whitespace-only optimization can consume a candidate slot or become the only returned candidate. Reject emptydescriptionbefore duplicate handling.Proposed fix
description = improvement.optimization.strip() + if not description: + logger.warning("dropping improvement with empty optimization text") + continue if description in seen_descriptions:🤖 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/proposer.py` around lines 204 - 209, In the improvement filtering flow, reject empty normalized optimization text immediately after assigning `description = improvement.optimization.strip()` and before checking `seen_descriptions`. Skip whitespace-only descriptions without adding them to `seen_descriptions` or `kept`, while preserving the existing duplicate handling for non-empty descriptions.
219-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConstrain
max_candidatesto positive values.EvolutionaryOptimizerConfig.max_candidatesaccepts0and negative values. These values make_filter_improvements()return an invalid candidate list. Enforcemax_candidates >= 1in the configuration or before truncation.🤖 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/proposer.py` around lines 219 - 225, Enforce that EvolutionaryOptimizerConfig.max_candidates is at least 1, preferably during configuration validation or before the truncation logic in _filter_improvements(). Reject or normalize zero and negative values so candidate filtering always produces a valid non-empty limit.
🧹 Nitpick comments (1)
plugins/nemo-experimentalist/tests/experimentalist/test_proposer_contract.py (1)
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the prompt contract assertions.
These assertions do not fully pin the clauses named by their tests. Assert the complete allowed-type clause, the explicit
available_typespreference, and the positive wording for the card selection.Proposed fix
- assert "must be in `available_types` or" in contract, "the contract must not restrict the pick to untried types" - assert "`tried_types`" in contract, "reuse is only reachable if tried types are a legal pick" + assert ( + "each improvement.optimization_type must be in `available_types` or `tried_types`" + in contract + ) ... - assert "prefer" in contract, "novelty has to survive as a preference, not vanish with the restriction" + assert "prefer `available_types`" in contract ... - assert "pick one optimization_type from `available_types`" not in contract + assert ( + "pick one optimization_type that the card covers, preferring `available_types`" + in contract + )🤖 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_proposer_contract.py` around lines 27 - 44, Strengthen the assertions in test_contract_permits_reusing_a_tried_type, test_contract_keeps_novelty_as_a_preference, and test_contract_does_not_restrict_the_card_pick_to_untried_types to match the complete contract wording: validate the full allowed-type clause, require an explicit preference for available_types, and assert positive card-selection wording rather than only rejecting the old phrase.
🤖 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.
Outside diff comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py`:
- Around line 204-209: In the improvement filtering flow, reject empty
normalized optimization text immediately after assigning `description =
improvement.optimization.strip()` and before checking `seen_descriptions`. Skip
whitespace-only descriptions without adding them to `seen_descriptions` or
`kept`, while preserving the existing duplicate handling for non-empty
descriptions.
- Around line 219-225: Enforce that EvolutionaryOptimizerConfig.max_candidates
is at least 1, preferably during configuration validation or before the
truncation logic in _filter_improvements(). Reject or normalize zero and
negative values so candidate filtering always produces a valid non-empty limit.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/tests/experimentalist/test_proposer_contract.py`:
- Around line 27-44: Strengthen the assertions in
test_contract_permits_reusing_a_tried_type,
test_contract_keeps_novelty_as_a_preference, and
test_contract_does_not_restrict_the_card_pick_to_untried_types to match the
complete contract wording: validate the full allowed-type clause, require an
explicit preference for available_types, and assert positive card-selection
wording rather than only rejecting the old phrase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4067e8d7-2aa2-4ecb-8513-8ef7cd9f7573
📒 Files selected for processing (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.pyplugins/nemo-experimentalist/tests/experimentalist/test_proposer_contract.py
schuellc-nvidia
enabled auto-merge
August 7, 2026 14:32
8 of 15 tasks
AnuradhaKaruppiah
pushed a commit
to AnuradhaKaruppiah/nemo-platform
that referenced
this pull request
Aug 27, 2026
…nt (NVIDIA-NeMo#1313) * refactor(experimentalist): a runner and a context, so the loop is just a strategy `_run` held everything: input resolution, all backend I/O, the entity lifecycle, the round loop, and PR publishing. There was no seam a strategy could be swapped at, because the runner and the sole strategy were the same code. Split them. `runner.py` is the composition root — it prepares inputs, runs one strategy, then persists and publishes. It is the only code that holds a Backend. `context.py` is everything a strategy may reach: its datasets, its candidates, and the verbs for measuring and recording them. `deps.py` is gone; config is a constructor argument. Four things move out of the loop and into the host, because they were never the strategy's to decide: - The insight/no-insight branch, with Eval Author now behind a lazy import: a run with no Insight never authors a suite and must not fail to import without the package. - Run resume. The runner re-opens the ExperimentRun; the strategy rebuilds itself from `ctx.candidates()`. A strategy that declares `supports_resume = False` is refused loudly rather than silently restarted — these runs cost hours, so the silent restart is the expensive failure. - The winner's copy-out. Its skip list mixed three owners (backend metadata, strategy documentation, evaluator scaffolding), so it is now composed from three named sets instead of one literal that would strip a third-party strategy's real output. - The Insight-suite report sections, which are a reward channel's epilogue. `report_progress(completed, total, unit, note)` replaces `rounds_completed` end to end. Not every strategy has rounds — DSPy's compile() is one opaque call — and the ones that most need reporting are exactly the ones that cannot produce a fraction. A counter is always producible, so `ExperimentRun` now carries one plus its unit, and a consumer renders a bar only when a total is actually known. Tests get the shared doubles the suite conspicuously lacked: an in-memory backend, a fake evaluator, and a context factory, in tests/experimentalist/. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): a Candidate is metadata and an artifact reference A Candidate was four jobs in one object: a proposed change, an identity, a set of measurements, and the optimized artifact. The artifact was always a directory, the identity was derived from that directory's name, and storage of the first three was implemented by walking the fourth — `list_candidates` globbed `agents/*/metadata.json`. A strategy that does not produce one directory per candidate could not store, list, or resume anything. Separate them. `Proposal` carries the build request; the runner stores identity and measurements at `eval-and-optimize/candidates/<id>.json`; and the finished work is *addressed* through `artifact: ResourceRef` rather than contained. That is the same pattern `persist_evaluation` already uses for traces, applied to the one entity that lacked it. The lifecycle inverts with it. A candidate used to be created and then mutated as the Coder filled its directory in, which is why a failed build needed a killed marker so resume would not resurrect it. Now `ctx.fork()` reserves and populates a directory, the Builder writes it, and `ctx.commit_candidate()` validates the result and creates the Candidate — so `artifact` can be required, a failed build leaves no record at all, and the killed marker for it goes away. The fields that went with the old shape: - `round` → `ancestor is None` for "this is the baseline", which `entities.py` already documented as the same thing, plus a strategy-supplied `generation` for grouping. DSPy leaves it 0; our loop sets the round. - `optimization`/`optimization_type`/`task_ids` → `description` plus the embedded Proposal, whose payload is owned by the Proposer/Builder pair. The Coder gets its own `BuildRequest` view and never sees the entity. - `ancestor` is a candidate id, not a directory name, so `benchmarks/run.py` reads the winner's location from its artifact instead of building `agents/{label}`. Candidate validation rejects a record whose `ancestor` or `description` disagrees with the Proposal they were derived from — two accounts of one candidate's origin must not be able to drift. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): stop stripping metadata.json from candidate artifacts Candidate metadata lives in the run's own store now, so ``metadata.json`` inside an artifact is no longer the host's file. Leaving it on the copy-out and publish skip lists is exactly the failure mode §9 warns about: a third-party strategy that writes one has produced real output, and the host would silently drop it. The remaining entries keep the owner they came from — ``architecture.md`` is the strategy's, the Harbor wrappers are the evaluator's — so a later reader can tell why each one is there. Also drops the Coder prompts' instructions about a file it can no longer see, and silences two nooa-stub return diagnostics that only surfaced because these files entered the changed set. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the loop's own prompts still read dropped Candidate fields ``select_diverse_survivors`` and ``merge_analysis`` are CodeAct methods: their docstrings are executed as Python against real Candidate objects. Both still told the model to read ``candidate.round`` and index ``get_metadata(...)["round"]``, which no longer exist — so the first cell of every round's analysis would raise and burn iterations recovering. Points them at ``generation``, ``is_baseline`` and ``generated_from.payload``, and uses ``label`` rather than ``name``/``id`` for the workspace lookups, since the display handle is what that tool is keyed by now. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the evolution tree is keyed by id, so finalize must be too An end-to-end run reached the last statement of the loop and died with ``KeyError: 'agent-0'``. `EvolutionTree.add` files nodes under the candidate id now that identity is no longer the directory name, but `_finalize` still took the winner's *label* and used it for both `mark_best` and the final node lookup. `mark_best` swallowed the wrong key silently, so the mistake surfaced one line later as an unrelated error — it now raises on an unknown key, which is what would have located this in seconds. `_finalize` keeps the node it selected rather than round-tripping through a string: `mark_best` gets the id, and `write_final_report` gets the label, because it reads the label-keyed workspace. Neither substitutes for the other. The test doubles hid this: `make_candidate` set `_id = label`, so every test saw identity and display handle as the same string — the one thing this milestone separated. They are now distinct by default, and a regression test covers the finalize path. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the mirror's ancestor lookup used an id as a label Studio lineage links were silently dropped. `_experiment_ids` was keyed by display label but looked up with `Candidate.ancestor`, which is a candidate id now — so the cache always missed, and the fallback then composed an Experiment name from that id, which can never exist. `NotFoundError` is caught by design, so the projection degraded quietly and an end-to-end run was the only way to see it. Experiment names are built from labels because they are meant to be greppable, and `ancestor` is an id because identity is no longer the directory name; neither should change to suit the other. So the mirror keeps both: the id keys the cache, and an id → label map lets it name an ancestor's Experiment. Within a run the ancestor is always projected first, so the map is warm; across a resume where a child is projected first the link is omitted rather than guessed. `experiment_metadata` now carries `candidate_id` (the real id) alongside `candidate_label`, so lineage is reconstructible from the projection alone — `candidate_id` previously held the label, which is what made this easy to write. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): a fork must not inherit the ancestor's architecture doc `architecture.md` is not documentation. It is the Proposer's only view of an agent — its prompt says to reason exclusively from that graph and not to read source — so whatever is in that file is what the next round proposes changes against. The pre-M1 fork excluded it and let the Coder seed it from the ancestor at the end of a build, right before rewriting it against the finished source. `_ignore_forked` copied it at fork time instead. Two consequences: - The Coder's seed step became unreachable: `ancestor_arch.exists() and not candidate_arch.exists()` can never hold once the fork provides the file. Dead code that still reads as live. - Absence stopped being a signal. `create_architecture_doc` is an LLM call and can fail; the Proposer renders a missing file as "(architecture.md missing for agent-N)" and knows it is blind. With the file inherited, a failed regeneration instead leaves the *ancestor's* graph in place and the Proposer reads it as the candidate's — a stale but plausible input, which is worse than a missing one because nothing surfaces it. Only reachable at max_rounds > 1, so the smoke config could not have caught it. Restores the exclusion and names why it sits alongside the run layout, the hygiene set and the evaluator's scratch. No claim is made that this caused the Coder to no-op in the M1 end-to-end run; that is still unexplained. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): resolve the model tiers once and hand them down Every component reached for a module-level getter to build its own clients, so nothing could say which models a run used, two runs in one process could not target different endpoints, and a test had to go through the environment to substitute a fake. `ModelTiers` resolves a tier on first use from one settings object. The runner builds one per run, the context carries it, and the strategy passes it to each of the nine components it constructs. The parameter defaults to this install's settings so a standalone caller still works, matching how every other config slice in this plugin is handed over. The run record follows from that. `config_snapshot` dumped only `EvolutionaryOptimizerConfig`, which since M0 split deployment settings out no longer mentions the endpoint or the tiers at all — so the one durable record of how a run was configured could not answer which models it used. It now carries a `deployment` block with the resolved endpoint and tier names, and deliberately not the credential: that record is written to run.json and mirrored to the platform. The gap predates M1 — the snapshot has dumped declared rather than resolved values since the plugin was migrated, and a live run.json shows it recording three nulls for a run that used three real models. Tests pin both halves; the snapshot one fails against the previous line. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): keep a candidate's record and its artifact in step Review of the M1 branch found ten defects, most of them one root cause: the record/artifact split left two halves of a candidate that nothing kept together. Deleting a directory used to delete the candidate, because the population was derived from directories. It is derived from records now, so rollback left ghost records addressing nothing — offered to the Proposer as branchable survivors, and selectable as the Pareto winner. `candidate_dir()` then resolved a vanished artifact to the shared `agents/` root instead of raising, so copying the winner out could `rmtree` the user's workspace entries and archival could push every candidate's code as one candidate's. Backends gain `delete_candidate`, `ctx.discard_candidate` removes both halves, and a missing artifact is refused. The rest: - `_reserve` gave the baseline's directory to every ancestor-less Proposal, so the HPO case the `generation` field exists for would have had N candidates overwrite one file while reporting distinct rewards. Only the baseline owns it. - Resuming before the first round analysis committed a second baseline: ids are uuids now, so a re-commit no longer overwrites a label-keyed record. Guarded. - `ancestor` became an id but was never validated, and the fork that resolves it ran outside the per-proposal error handling — so one hallucinated `"agent-2"` failed a multi-hour run. Validated in the Proposer, and an unforkable proposal is now dropped rather than fatal. - The build Coder was the one component not given the run's ModelTiers, so it wrote every candidate against the ambient endpoint while config_snapshot named the injected one. A test now asserts over the source that no construction omits it, since the failure is invisible per-component. - The winner was matched by value against candidates re-read from the store, so any field that did not round-trip dropped the Insight section silently. Matched by id. - `api_base` was copied verbatim into the run record, which is written to disk and mirrored to the platform — but a URL is a normal way to carry a key, which is why the log banner already strips userinfo. Sanitized. The previous guard test only checked the API-key value. - FakeBackend equated candidate id with label, which is what let the id/label confusions through in the first place. It now assigns uuids and keys by id. - The example's uv.lock kept the old nooa revision the repin replaced. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): carry M0's later fixes into the refactored loop Rebasing onto the current M0 branch brought three changes this milestone was written before, all in code it had since rewritten, so the conflict resolution that kept the new structure dropped them: - `Candidate.set_reward` is `record_reward` now. Every caller follows, including `ctx.record_reward`, which writes through it. - Cancellation is not a build failure. `CancelledError` derives from BaseException, so `_build_candidates`' filter would have let a cancelled build through as if it had succeeded — committed, evaluated and ranked. It re-raises instead, so the round unwinds. - A failed build logs its exception and traceback, not just the candidate name. `return_exceptions=True` turns a failure into a value, so the cause is only ever visible if it is logged, and a build that never became a candidate is the one thing a run cannot cheaply reproduce. Two tests M0 added for the reward accessor construct a Candidate directly; they now build one through the shared double, since the entity requires a description and an artifact. The evolution-tree table keeps M0's full channel names and this milestone's `description` column: M0 deleted the abbreviation map that the renamed column header was still reaching for. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): prove a method-level model tier reaches its model M0 made components take their tiers by injection, and M1 said the point of that was a canary: nothing in the suite proved an `@strategy(llm=...)` override actually resolves. `test_experimentalist_analyzer.py` builds its analyzer with `object.__new__` and assigns over `select_trials`, so it stubs past the decorator entirely — the override could stop working and every test would still pass while the run quietly moved to the class-level smart tier. Resolve it through nooa's own `resolve_method_llm`, which is what its dispatch calls, so the assertion is about our wiring and needs no LLM. Verified in both directions: deleting the `llm=lambda self: self._fast_model` line makes it fail. Set the tiers through the environment rather than passing them to `ExperimentalistConfig`. For a `NemoConfig` the environment wins over constructor arguments, so passing them reads back whatever the conftest exported and all three tiers collapse onto one client — which also makes `ModelTiers`' claim that a test can inject fakes without touching the environment false. Corrected that docstring rather than leaving the trap for the next person. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): the Builder owns the candidate span, and components register M1 said a Builder consumes one Proposal and returns a committed Candidate, and that a strategy is resolved by name. Neither was true: the strategy forked and committed, the Coder wrote into a directory handed to it through a `BuildRequest`, and `EvolutionaryOptimizer` was constructed by a factory that named the class. `Coder.build(ctx, proposal, *, generation) -> Candidate` now owns the whole span — it asks the context for a working copy, edits, verifies, and hands back the committed Candidate. No filesystem path crosses into it from outside, which is what lets candidate storage move without changing a Builder's signature. The seven places that rebuilt `eval-and-optimize/agents/<name>` from run layout, including inside prompt text, now descend from the Fork the context returned. `BuildRequest` is deleted. It re-read the code-change payload with `payload.get()` alongside the typed `CodeChange` the Proposer already writes, and dropped `root_cause` on the way — so the Coder was told to validate its fix against tasks chosen by a diagnosis it never saw. `CodeChange.model_validate(proposal.payload)` is now the single reading, and the diagnosis reaches the build. `fork` returns a Fork — a working copy plus the upstream it came from — because a fork inherently knows what it forked. Asking the context separately for "the ancestor directory of a proposal" was a confused question. `import_baseline` takes the baseline out of `fork`/`commit_candidate`, so both lose their "unless this is the baseline" branches and `proposal` is never None; it is also idempotent, since a resumed run re-enters it and the invariant is the host's to keep, not something each strategy reimplements. `save_candidate` could persist a Candidate that had never been validated, so the create path is private and `update_candidate` cannot reach it. Components are found by `(role, name)`. Ours ship entry points in the `nemo.experimentalist.components` group exactly like a third party's — there are no privileged built-ins, so if the mechanism a plugin developer depends on breaks, our own loop stops resolving too. Resolution never falls back; enumeration degrades, so one broken package cannot take down a run that does not name it. Roles are typed against Protocols in `seam.py` rather than the concrete context. That is structural, not stylistic: naming `ExperimentContext` in `roles` closes an import cycle through the backend, the config tree, and every component config slice. It also lets an out-of-tree strategy type its own `run` without importing our internals. `Candidate.rewards` becomes a mapping whose `__missing__` returns an empty record without storing it, replacing the `reward()`/`rewards` pair. Not a defaultdict: that one's `__missing__` inserts, so reading a channel would mark it measured, skip its evaluation and persist a phantom. A channel is written once, so the merge goes with it. Verified end to end: a smoke run completes with the baseline imported, a candidate built through the Builder, and records whose ancestor is the parent's id rather than its label. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): read the benchmark's winner through the store, not the layout The plan predicted this consumer would break: `benchmarks/run.py` reads the local backend's on-disk layout directly, including `ExperimentRun.winner_agent` — documented as a candidate id but used as a label to build `agents/{label}`, which only worked because the local backend set the store id equal to the label. Half of that was already fixed by resolving the winner through its own artifact reference. This does the other half: `load_winner` lives next to `load_candidate`, so the benchmark stops hardcoding `candidates/<id>.json` and the knowledge of where a run keeps its records has one home. Also fixes a defect introduced while making the file type-check. Harbor's task ids are a union and only `PackageTaskId` carries a `name`, so `str(getattr(task, "name", task))` returned a *pydantic repr* — "git_url='...' path=PosixPath('...')" — for the other two variants, silently substituting a garbage task id for a real one. There is no accessor that means the same thing across the union (`get_name()` gives "hello-world" for a git task and "org/hello-world" for a package one), and benchmark suites are pinned to published packages, so anything else is a suite authored wrong: say so instead of guessing. `metadata.version is None` now raises too, rather than validating "" against the suite's pinned revision. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): make the resume path survive being interrupted A workflow review of the branch found ten defects, five of them on resume — the one path no test drove, and which the MR description admitted had never been run live. Three of the five are regressions this branch introduced. **Resume.** A run interrupted after the baseline was scored but before round-0's analysis landed took the loop's *fresh* branch on restart, where the idempotent `_ensure_baseline` keeps the existing baseline and `_evaluate_validation_candidates` returns nothing pending — so indexing that map by label raised `KeyError` and failed the run at exactly the moment it must not. Recording is now its own named step that skips what was already measured. `run.json` is the only record of which run owns the candidates on disk, and it was rewritten in place on every progress report; a kill mid-write left a file resume could not parse, whereupon the runner minted a *new* run id and `ctx.candidates()` — which filters by it — returned nothing. Hours of candidates stayed on disk and became invisible. It is now written through a temp file and renamed, and starting over a populated candidate store refuses loudly instead of silently orphaning it. `_finalize` ran *outside* the handler that marks a run failed, so a run whose work had finished but whose winner artifact had gone stayed `running` forever with no result written. **Elsewhere.** An unknown ancestor from the Proposer aborted the whole run: that check runs after the CodeAct loop, so raising bought no retry — it unwound through the strategy and killed a multi-hour run over one bad string. Bad improvements are dropped now, with the batch still failing loudly if *every* one is unusable. `_ignore_forked` stripped `architecture.md` at every depth, deleting an agent's own `docs/architecture.md` from every candidate and from the winner copied back over the user's workspace; it is top-level only again, as the three paths it replaced were. Subclassing a registered component raised "duplicate" at import, because `name` was inherited — the most obvious way to customise a builder was the one way that could not work. The draft-PR title interpolated the Proposer's multi-sentence prose where a short handle belongs. `WorkspaceTool.get_metadata` re-parsed the whole candidate store per lookup, quadratic in a loop the prompts drive per agent. And the deleted `test_loop_failure.py` took the cancellation guard's only test with it. **Tests.** `test_resume_e2e.py` drives a real runner, context and local backend over a real directory, with only the model and evaluator faked, and cuts the run at the points a crash actually lands. Every fix here was checked by reverting it and confirming a test fails — including the atomic write, whose first test passed either way because it asserted the outcome rather than the mechanism. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): discard candidates by marking them, not deleting them Rolling back an incomplete round is the only thing that discards a candidate — a failed build never commits one, and a candidate that loses selection is marked `killed_generation` and kept. So the only work ever destroyed was work that is about to be redone, and destroying it made a wrong rollback unrecoverable. `discarded` is now a field. `discard_candidate` marks the record and leaves the artifact in place; `list_candidates` filters discarded candidates out unless asked for them, so a consumer cannot forget to. `delete_candidate` is gone from the backend interface — nothing needs it. Both halves survive together deliberately. Keeping the record while deleting the directory would let `_reserve` hand out a label a discarded record still claims, giving one run two candidates with the same handle — the id/label conflation that has already caused four defects here. Evaluator scratch is still removed on rollback: it is keyed by label, so leaving it would let the re-run read a previous round's results. `_delete_all_artifacts` is now `_roll_back_to`, which is what it does. Also sweeps the historical comments this branch introduced — "this used to be X", "no longer overwrites the way it did" — which read as noise to anyone who has not followed the refactor. State what the code does; the reason for a rule is worth keeping, the shape of what it replaced is not. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): review renames and clarifications Review feedback on names and comments that read as unclear: - `ctx.client` is `ctx.platform_client`, and `registry.get` is `get_component` — both were generic enough that a call site said nothing about what it returned. - `context.py`'s docstring names both `ExperimentContext` and `StrategyContext` and says which is which: the class and the Protocol components are typed against. - `_reserve` states the rule about fresh directories rather than illustrating it with HPO, which does not exist yet. - The dataset lookup asserts what it relies on — train and validation are guaranteed by the runner, insight exists only with an Insight — instead of leaving `.get` to imply it. - The run's own directories are named once in `_roll_back_to` rather than four times, and the tree/report keying comment sits on the line it explains. - `pyproject.toml` says why the entry points list modules rather than components. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): the baseline is just the candidate with no parent The baseline had its own creation verb, its own hardcoded `agent-0` handle, and its own `generated_from = None` case, so "is this the baseline" had to be answered in three ways and every creation path carried an "unless" branch. It is now an ordinary build of an ordinary Proposal: `kind="import"`, `ancestor=None`, built by an `Importer`. `commit_candidate` is the only way a Candidate is born, `generated_from` is never empty, and `ancestor is None` is the single place the distinction lives — as data, not a branch. Two things fall out. The baseline lands on `agent-0` because the first fork takes the first free handle, so nothing has to name it. And a strategy that wants several roots — importing three agents to compare — gets that for free, where the old verb assumed exactly one. Host-side idempotence goes with it, deliberately. "Exactly one baseline" is the evolutionary strategy's invariant, not the host's, and `_ensure_baseline` already enforces it; a strategy comparing three imported agents would have been wrong to stop. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): selection becomes a component Selection was spread over three places in the loop: a Pareto sort, an LLM method choosing diverse survivors, and a separate Pareto front picking the winner from evolution-tree nodes. Nothing named the policy, so nothing could replace it. `ParetoDiversitySelector` owns all three as `rank` / `survivors` / `winner`, resolved as `selector: pareto-llm-diversity`. It reads reward channels and nothing else — no Proposal, no artifact — which is what lets one selector serve a code-optimizing run and a numeric one. Which channels it ranks on is config (`objectives`, defaulting to validation) rather than a flag on the reward channel: "insight rewards must never feed Pareto selection" is a selector's business, and a `selecting: bool` on the channel would be wrong for anyone who wants to rank on train. Ranking and diversity are separate methods on purpose. Ranking is arithmetic over channels; choosing among incomparable candidates inside one front is a judgement that needs a model. A numeric strategy can reuse the first and skip the second. `_finalize` now takes the selector rather than reaching through `self`, so the winner comes from the same component that chose the survivors. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): every seam the strategy delegates to resolves by name The registry existed but held two roles; the other five were constructed directly by the loop, so a new optimization paradigm would have had to edit the strategy to reach them. All seven now resolve through it: strategy, builder, proposer, selector, terminator, root-cause-analyzer, trajectory-scorer. The two `disable_*` booleans are gone. Turning a step off is the degenerate case of choosing a different implementation, so `terminator: null` stops only on the round budget and `trajectory_scorer: null` skips step scoring and the goal tree it needs — the same spelling as `analyzer: null`. Each role's config moved to `<role>_config`, leaving the role key to name the component. `EvolutionaryOptimizer` becomes `EvolutionaryStrategy` in `strategies/evolutionary.py`: it fills the `strategy` role, and the file held one strategy rather than a loop shared by several. `EvolutionaryOptimizerConfig` keeps its name — it configures the algorithm, not the role. `nemo agents experimentalist components` lists what an install can resolve, which is how a developer checks their own package was picked up. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): M1's acceptance bar, as an assertion The milestone claims the core is done, and the test of that claim is that a new optimization paradigm needs no edit to the strategy or to any existing component. This registers a stand-in for all seven roles the way a `pip install`ed package would, names each in config, and checks the registry hands back what config asked for. If a seam is not swappable, this says so now rather than when HPO is written. Also covers the two failure modes that are easy to ship: a default naming a component that does not resolve, which kills a run after the user has waited for it, and the optional roles accepting null while the load-bearing ones do not. The fixture restores the registry's *contents* rather than swapping the mapping. Entry-point discovery writes into whichever dict is bound when it runs, and it runs once per process, so replacing the object sends the real components into a copy that is then discarded — leaving every later lookup empty. That is the same hazard as the `_loaded` flag, and it bit here first. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): evaluation resolves by name, and its type stops being a Literal `EvaluatorType = Literal["harbor"]` meant adding an evaluator required editing this package — the closed set that §2 names as the fourth reason the code could not host a plugin system. It is now the name of a registered `evaluation` component, so the set is open and a package can add to it. `HarborEvaluator` registers as `evaluation: harbor`, and the runner resolves it from `config.evaluation` rather than a hardcoded factory key. That makes eight roles swappable from config, which is the whole set the strategy delegates to. Deferred deliberately: lifting the ~130 lines of generic machinery out of `harbor.py` — the scoped Python import, the syntax preflight, the trial-dir walk — and the Harbor-specific fields that reach the generic loop through `EvaluatorConfig`'s `extra="allow"`. That is a quality problem inside one component, not a seam: it changes nothing about whether an evaluation component can be replaced, and it touches the path every run's numbers come from. It wants its own change with its own e2e, not a rider on this one. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): prove the plugin mechanism from outside the repository `examples/acme-strategies` is a separate package with its own pyproject, depending on the plugin rather than the monorepo, shipping one entry point. Installed beside us, its strategy resolves as `strategy: random-search` with no change here. That is the only evidence both discovery levels of §1 work for someone who never checked this repository out. Our own components register through the same group, but they live in this tree and this venv, so they prove the mechanism compiles rather than that it works for a stranger. Writing it surfaced a missing verb: `ctx.component(role, name)`, which §3.2 specifies and nothing had implemented. Without it a third-party strategy has to import our registry to reach a builder — reaching around the seam the context exists to be. It supplies the run's model tiers by default, since that is the one argument every component needs and the one a strategy should not have to know about. The strategy itself is deliberately trivial. What it demonstrates is the wiring: it imports only entities, the role, and the context Protocol, and reaches the platform only through the context. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the round budget is the loop's, and a bad proposal is not a bad run An end-to-end run failed at round two and exposed two defects, one of them a regression from promoting the terminator. **The loop had no bound of its own.** It was `while True`, and the only thing enforcing `max_rounds` was the budget check *inside* the default terminator. Turning the convergence check off used to keep that terminator, so the budget survived; selecting a different terminator — or none — removed it, and the run kept proposing past its configured budget. A component's opinion must not be the only thing between a config and an unbounded run, so the loop bounds itself and the terminator decides whether to stop *early*. **A near-miss optimization_type ended the run.** The Proposer returned `edit_method` where `edit_concrete_method` was valid; that check runs after the CodeAct loop, so raising bought no retry and unwound through the strategy after hours of work. Unusable improvements are now dropped with a warning, the same policy already applied to an unknown ancestor and to a failed build, and the batch still fails loudly when nothing in it is usable. Both were found by the end-to-end run, not the suite: the first needs a config that selects a non-default terminator, the second needs a model to make a plausible mistake. Regression tests for both. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): make the seams swappable in fact, not only in lookup A review found the acceptance test proved the wrong thing. Every assertion in it was `resolve(role, name) is not None`, so a role could resolve by name and then die on its constructor — and two did. `evaluation` was the worst: the runner built the evaluator *and every dataset* through a factory keyed on a hardcoded `{"harbor": (...)}`, so `evaluation: acme` passed the swap test and raised "Unsupported evaluator type" at run start. The component now declares its own `dataset_type` and `config_type`, and the factory resolves rather than looks up — which is what plan item 8 asked for. `ctx.component` did not supply the run-scoped arguments a component cannot know for itself, so `ctx.component("builder", "coder")` raised TypeError. It now passes the model tiers and the run root to whichever of them the constructor names. Named parameters only: a `**kwargs` on a component almost always means "forward to my base class", so treating it as permission hands an nooa Agent a `workspace` it rejects. Two more the same run turned up. `analyzer: null` skipped writing `round-N.md`, and that file is the strategy's only resume marker — a restart could not tell which rounds had finished, started from zero, and left every earlier cohort alive in the store to be re-selected. The train evaluation now goes with the analyzer that consumes it, which is what made `analyzer: null` claim to be cheaper. The acceptance test now constructs every default through the context and drives a swapped Builder to a committed Candidate. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): stop discarded candidates leaking, and removed config keys passing silently Three from the review. A rolled-back candidate was still projected to the platform as `survived`, because `update_candidate` mirrors on every write including the one that discards, and the status derivation had no branch for it. It also still reached the report and analysis prompts through `WorkspaceTool`, which globs the candidate store directly. Both now respect the same rule `list_candidates` does. `disable_convergence_check` and `disable_trajectory_scoring` were removed fields on a model with pydantic's default `extra="ignore"`, so an existing config carrying them ran full goal-tree scoring and early termination while reading as if it had turned both off. They now raise and name their replacement, like the `curator` and `models` renames before them. Same for `analyzer:`/`proposer:` when they hold a config block. And the fourth registry bug: a `monkeypatch.setattr` handed the object it was already holding records that object as "old" and restores it unchanged, so the test terminator stayed registered for every later test. Every one of these four has been invisible to the full suite and visible only in isolation or reverse order, so the no-leak invariant is now asserted rather than assumed. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): candidate identity survives serialization, and comments say less A Candidate round-tripped through JSON came back with an empty `id` while its label survived, because `id` is a computed field backed by a private attribute and `model_validate` ignores computed fields. Selection crosses exactly that boundary — survivors are the return value of an LLM method — so a model that returned reconstructed candidates would produce `survived = {""}`, and the round would mark every candidate killed, survivors included. `ExperimentRun` already had this validator; `Candidate` did not. The rest is comment economy, from a review that read every comment the branch adds. Two were worse than verbose: `roles.py` opened by saying five of the seven roles "are still constructed directly by the evolutionary strategy and become registry citizens in M2" — all five are declared and resolved in that same file. And `Builder.accepts` spent ten lines describing a resolution-time check that does not exist, on the premise that the Proposer is not a resolved component, which it is. Also: `EvolutionaryStrategy`'s class docstring is its system prompt, and it was spending it on host provenance and the runner's orchestration. The prompt guard that should have caught this only checked four literal strings, so it now bounds length and covers all seven Agent components. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the seams a review found still half-connected A verification pass over the whole branch. The finding that mattered: the out-of-tree example package — the milestone's own evidence that a third party can do this — could not build a candidate. `ctx.component` supplied the model tiers, the run root and the workspace, but not the evaluator or the dataset, so the Coder it returned raised on its first build. It now supplies every run-scoped argument a component cannot know for itself, and a test pins each one: deleting any single line from that mapping fails. Two roles were resolved by name and then ignored: `_generate_architecture_doc` instantiated the Coder directly, so `builder: acme-overlay` still ran our Coder's LLM against every baseline and wrote an architecture doc for a Builder that never asked for one. Documenting an artifact is now a Builder verb with a do-nothing default. And the goal tree was built and updated whenever *any* trajectory scorer was configured, though it is the built-in scorer's own data structure — a replacement paid two LLM passes per round for something it cannot read. Scorers declare whether they want one. `Proposer.produces` was documented as the Proposer/Builder compatibility declaration and read by nothing. A pairing that can never build now fails before the run starts rather than after `max_rounds` empty rounds. `slim()` empties trials so a candidate can go into a prompt, and the loop carried the selector's slim copies back into the population. Recording any later reward persisted one, wiping every channel's trials in the store — including the validation traces trajectory scoring reads, which then reported 0.0 for candidates that had them. The loop maps survivors back to the real records, and persisting a slim copy is now refused outright. This one predates the branch. Four tests could not fail, including two added by the earlier review round: `test_a_role_can_be_turned_off` asserted that pydantic returns what it was handed; the round-budget test asserted substrings of `inspect.getsource`. Both now drive the loop. Every fix here was checked by reverting it. Also: the proposer derived an artifact path from a display label, which the Candidate contract forbids; the strategy resolved its own model tiers rather than the run's, so `config_snapshot["deployment"]` described models it had not necessarily used; a re-scan kept stale import failures; ~35 lines of dead code; and one docstring where an in-place edit had left half of the old sentence behind. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): two of the previous commit's fixes were wrong Teaching `ctx.component` to supply the run's dataset made it supply `datasets[PRIMARY_SPLIT]`, which is `validation` — the split `holdout_utils` hides from the agent because the winner is chosen on it. A Builder runs bounded LLM repair loops against whatever dataset it is handed, so every Builder resolved through the seam was repairing against the held-out set, while the strategy's own `_new_builder` passed train. The seam now passes train too, and the test that asserted the old behaviour was pinning the leak rather than catching it. And gating the goal tree on a new `TrajectoryScorer.needs_goal_tree` did stop a replacement scorer paying for two LLM passes it cannot read — by also gating the only call site that constructs a scorer at all. `trajectory_scorer: acme-steps` became silently equivalent to `trajectory_scorer: null`: the role resolved and was never invoked. Reverted. The underlying complaint stands and is not fixed here: the goal-tree pipeline lives in the strategy, so a scorer is handed `GoalNode` objects whether or not it models goals. That is now stated on the role instead of papered over with a flag. It is the narrowest of the eight seams and moving the pipeline into the component is what would widen it. Also: `Coder.build` called `create_architecture_doc` directly, so a subclass overriding `describe` was honoured for the baseline and bypassed for every candidate; a selector choice outside the population was dropped in silence; the proposer read the architecture doc without the file-vs-directory normalization the context applies. Two more tests could not fail — the analyzer one asserted a substring of `inspect.getsource`, the same anti-pattern this branch already replaced once, and the terminator's round-budget test asserted pydantic defaults. Both now drive the loop, and each fix above was checked by reverting it. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * docs(experimentalist): say what the slim guard is worth, and pin describe The marker `update_candidate` refuses on is a private attribute, so it is lost through JSON — and the path it exists for, an LLM method returning candidates, is exactly the one that crosses that boundary. The docstring claimed a stronger invariant than the code has. It now says the guard is in-process only, and that this is why the loop looks survivors back up by id rather than relying on it. `Builder.describe` did not say who calls it when. The strategy calls it for the baseline; a Builder calls it for the candidates it builds, and one that never does produces candidates the Proposer reads as "(architecture.md missing)". `Coder.build` delegating to `self.describe` had no test, which for a hook whose only purpose is being overridden is the wrong way round. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one naming rule for config, one for roles Review found three inconsistencies that make the plugin contract harder to read than it needs to be. `evaluation:` selected a component while `evaluator:` configured it — two near-identical words for different things. Every role now follows one rule: the bare key names the component, `<role>_config` configures it. So `evaluator` becomes `evaluation_config`, `coder` becomes `builder_config` (it was a config named like a component, which is backwards), and `goal_config` becomes `trajectory_scorer_config`. All three are rejected with a migration error naming the replacement, like the keys removed earlier. Components declared their role four different ways; they now all use `roles.<Name>`, which distinguishes the role from `Agent` and from the same-named component class — there is both a `Proposer` component and a `roles.Proposer`. And `_evaluation()` returned `Any`, so callers read `dataset_type` off it unchecked. Making the role inherit `Evaluator` gives it one type that is both constructible and carries the ClassVars, rather than an implicit pairing a plugin author has to infer. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * feat(experimentalist): every role declares its entry point, in terms only entities name Three of the eight roles declared a method; the other five declared nothing, so their real contracts lived implicitly in the evolutionary strategy's call sites. A plugin author had no way to learn the signature except by reading our loop — which is the difference between "swappable" and "swappable if you already know how it works". All eight now declare one, and the test for what each should say is M2's: would adding HPO force a change to a core file? Four of the five failed it. - `Terminator.run` took `EvolutionaryOptimizerConfig` and an `EvolutionTree`, so any strategy had to build ours to consult a terminator. It now takes the population and reads its own thresholds from `terminator_config`. `assess_round_budget` is gone: the budget is the loop's, and a component's opinion must not be the only thing bounding a run. - `Analyzer.run` took a display label — which the Candidate contract forbids deriving storage from — plus a platform client and workspace name, pushing the `ctx` escape hatch through a public role. It takes a Candidate; the handles arrive at construction. - `Proposer.run` took an `EvolutionTree` *and* a pre-rendered markdown table of the same data, plus an evolutionary-only `phase`. It takes candidates, renders its own table, and `hint` is a free-form steer any strategy can ignore. - `TrajectoryScorer.run` took `GoalNode` — the built-in's own data structure — so a replacement was handed goal-tree leaves whether or not it models goals. The goal tree is now the scorer's: ~190 lines of generation, refinement, loading and scoring moved out of the strategy, which went from three touchpoints to one call that hands over candidates and takes back reward records. `Evaluation` is the exception: it inherits `Evaluator`, so `run` is already declared and working. Restating it as a stub shadowed the real implementation in the MRO and broke every evaluator — caught by the suite, and the reason the role documents the inheritance rather than repeating it. No signature now names `EvolutionTree`, `GoalNode`, `EvolutionaryOptimizerConfig` or a platform client. `PRIMARY_SPLIT` moved to `seam.py`, where the components that need it can reach it without importing the implementation. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the analyzer was built without the run's platform handles Moving `client` and `nmp_workspace` off `Analyzer.run` is what got platform types out of the role's signature, but the construction site was never given them. The analyzer ran blind for a whole round: it could not load `intake://` traces, and it reported that as the *agent's* root cause — "the analyzer lacks the Intake client/workspace configuration required to load the count..." The Proposer acted on it and the Coder, with nothing in the agent to fix, committed a candidate identical to its ancestor. The whole suite passed while this was broken, because every analyzer test constructs the component itself and passes a client. Nothing asserted the strategy supplies one. The new test drives `_analyze_round` with a recording analyzer and pins what it is built with; re-introducing the omission fails it. Found by an end-to-end run, not by the suite. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit 1a81761db4809293e82d76c7fb3ed6ecc6ba186f) * fix(experimentalist): a Candidate's identity and its measurements stop being writable The reward record the trajectory scorer returns is keyed by candidate id, not by the Candidate: a Candidate is a pydantic model and unhashable, so `dict[Candidate, ...]` raised at the moment of return — after the round had paid for the scoring. Keying by id also matches the rule the rest of this branch follows. The suite passed while that was broken because the stub scorer returned `{}`, and an empty dict never exercises the key type. It returns a real record now, and there is a test on the built-in scorer, because the first fix was not enough: reverting the real line still passed, since the swap test only ever drove the stub. Which raised the larger question — twelve public mutable fields, and nothing but convention keeping them straight. Two are now enforced: Identity and provenance are frozen. `run_id`, `label`, `ancestor`, `generation`, `generated_from`, `description` and `artifact` are set once by `commit_candidate` and never again — verified: nothing in src wrote them post-construction, and `update_candidate` only ever receives `discarded` and `killed_generation`. Rewriting a candidate's lineage is now a ValidationError rather than a convention, which is the class of mistake behind five defects here. And `RewardMap` refuses `__setitem__`. A direct write mutates memory and is never persisted, so the candidate looks measured until the next reload — the same silent-loss shape as the `trajectory_detail` mutation this commit's first paragraph is about. `record_reward` remains the one writer and bypasses the guard on a single visible line. Fixtures that need a pre-scored candidate use `doubles.seed_reward`, which says in its name that nothing was persisted. Found by an end-to-end run with `trajectory_scorer: goal-tree`, which is the only configuration that reaches the scorer at all. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): tell the scorer which round its analysis describes The loop's counter has already advanced when the trajectory scorer is called, so passing it raw made the scorer name its state after the wrong round — a round-1 goal tree was written as `round-2-goal.json`. Nothing broke, because the scorer reads back the highest number it finds, but resume reads those files and the numbering was a lie. Caught by the first end-to-end run with `trajectory_scorer: goal-tree`, which is the only configuration that reaches the goal-tree pipeline at all — both smoke scenarios and the tau3 config set it to null. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): smoke-check the tasks a change claims to fix `integration_check` drew one task at random from the whole train split and never looked at `CodeChange.task_ids`, so it routinely verified a task the change does not touch. That matters because the check is not just a gate — it drives the Coder's repair loop: for attempt in range(max_fix_attempts + 1): evaluation = await self.run_smoke_eval(...) if healthy: return True await self._fix_runtime_issues(workdir, evaluation) Passing on an unrelated task returns True on attempt 0, so the repair never runs. Seen end to end: a candidate added an aggregation handler whose regex matched nothing, was smoke-checked against a lookup control that already passed, and shipped with both targeted tasks at 0. The Coder had two repair attempts it never used. It checks the targeted tasks now, falling back to the old single random task when a Proposal names none. Not a correctness fix — the loop still evaluated that candidate on validation and the selector correctly kept the baseline — but a wasted round and a wasted self-repair. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): the run's metric contract must reach the components `objective_function` and `regression_metrics` were defined and validated on the run config and then read by nothing. Every component fell back to its own default of "maximize reward", so a run that configured an objective was silently optimizing a different one -- and it still completed, reported, and picked a winner, which is why the merge did not surface it. Threads both into the selector, proposer and terminator at construction, keeping role signatures free of strategy config, and gives the selector the finalization contract main added in #1195 -- ported rather than adopted, because M1 had already split the question in two. Survivor selection is an LLM judgement told to favour new blood; `winner` is arithmetic that must be able to answer "nothing here beat doing nothing". Main routed both through the former, which is the bug #1195 fixed; here they were already separate methods, so `winner` only had to gain the rules: - the generation-0 baseline is re-admitted even if survivor selection killed it, and anchors the regression comparison even when it does not win - a candidate that worsens a protected metric is dropped before ranking, so a gain on the objectives cannot pay for a regression - ranking projects through `pareto_objectives`, so a minimized target is not ranked backwards and a protected metric does not sway the front - ties go to the oldest candidate: equal score means the diff bought nothing `rank` gets the same projection; it had the same direction bug one level up, where it decides which candidates breed. Main's `test_winner_selection.py` is ported onto `Selector.winner` and kept whole -- 26 cases, each verified to fail when the behaviour it names is reverted -- plus a new test naming each construction site, since the wiring was exactly what nothing covered. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): stop stubbing a method that no longer exists `test_role_swap` silenced `_generate_initial_goal_tree` in four places. That method moved into the trajectory-scorer during this refactor and no longer exists on the strategy, so those lines set an attribute nothing reads -- silencing nothing while reading as though a model call were being suppressed. The remaining stubs assigned one `-> dict[str, Any]` function to five methods returning `None`, `str` and `dict[str, EvaluationResult]`. Split into one stub per return type, and set through `monkeypatch.setattr` so the type checker sees the assignment and pytest undoes it. Type diagnostics for the file: 31 to 1. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): say what each removed config key became, and cover them all The rejection for a removed key reused one sentence for two different migrations, so a rename was explained with "turning a step off is how you choose no implementation of it" -- advice that does not apply to it. Split into the three cases that actually exist: a `disable_*` flag becomes `<role>: null`, a config block moves under `<role>_config` while the bare key starts naming the component, and a role renamed outright takes both its name and its config key with it. The guard also only fired on a dict, so `evaluator: harbor` -- a plausible thing to write, and no longer a field -- was accepted and ignored. Keys that are not fields at all are now rejected whatever the value. Only `curator` had a test. The config tolerates unknown keys, so every other removal was silently accepted and the run then did something other than what the file said, with `config_snapshot` recording it as correct. All nine are covered, including the half that is easy to break while fixing the other: a *string* under `analyzer` or `proposer` still selects the component. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): a repeated optimization_type is not a malformed batch #1163 removed this raise on main -- "repeated `optimization_type` is fine" -- and the merge kept the pre-#1163 lines while also taking #1163's duplicate-text warning, so the validator ended up a mixture of both versions. The raise contradicts the reasoning two methods above it, which argues that these checks run after the CodeAct loop, that raising buys no retry, and that one bad proposal is not a bad round. There are twenty optimization types and two genuinely different edits to the same method share one, so it is not evidence of a malformed batch; duplicate optimization *text* is, and is already dropped. Found by an end-to-end run: the generalization scenario died at `Proposer returned duplicate optimization_type 'edit_concrete_method'` after paying for a baseline evaluation and an analysis. Nothing covered the raise -- removing it broke no test -- which is how the mixture survived the merge, so both halves are pinned now. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit aa4afa721c26d88c29e6100e97f877243dc1ee67) * refactor(experimentalist)!: name each component for its implementation, and the evaluator for what it measures Two naming inconsistencies from review, both in the same rule. **`coder` -> `llm-code-edit`.** Eight of nine components name an implementation -- by method (`evolutionary`, `pareto-llm-diversity`), evidence (`agent-trace`), structure (`goal-tree`), backend (`harbor`), output (`code-change`) or operation (`import`). `coder` named an *agent* instead, which is what the role already does, and it sat next to `import` under the same role spelled the other way. The class stays `Coder`; only the registered name changes. **`evaluation` -> `outcome-evaluator`.** The role was named for what the step produces rather than the thing that does it, leaving it the one role that was not an agent-noun, and "Evaluation" said nothing about *which* evaluator it is. What makes it specific is already in its own docstring: it measures the outcome, where the trajectory-scorer measures the process of the same run. `evaluator` alone was not available -- the role inherits `Evaluator`, and one word for both is the confusion the `<role>` / `<role>_config` rule exists to remove; `outcome-evaluator` keeps them apart and matches the `root-cause-analyzer` precedent. So `evaluation:` becomes `outcome_evaluator:`, `evaluation_config:` becomes `outcome_evaluator_config:`, and `ctx.evaluation` becomes `ctx.outcome_evaluator`. Both old keys are rejected with a migration error naming the replacement, as with every key removed before, and both are covered. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): restore three main-side changes the merge had reverted Found by auditing every main commit against our tree rather than by another dead run. The merge combined two change sets in 42 files, and in three places it kept ours and dropped theirs: **#1163's allowlist fix.** `allowed_types = set(available_types) or all_types` makes a type forbidden the moment it is used, because `available_types` is `all_types - tried_types`. Christian removed exactly this line five days ago and we brought it back. It killed a real multi-round run: rounds 1 and 2 took the agent from 0.333 to 1.000 with `edit_concrete_method`, round 3 proposed refining it, the only proposal was rejected as unusable, and the run raised and died -- precisely when the loop was succeeding. Now `all_types`, with novelty left as the prompt-side preference it was always documented to be. **#1159's litellm removal.** `_LiteLLMModule`, `_enable_litellm_drop_params` and its test came back. Models resolve through the platform now, so the shim was dead code that still ran an import on every run. Main carries no reference to it. **#1107's insight-channel reads.** Four survived: a summary line, two prompt code examples, and an "Insight Suite Reward" table with instructions on when to render it. Nothing has recorded that channel since insight promotion was deleted -- zero writers, four readers -- so the table could never have data, and the instruction was paying tokens in an Agent docstring to describe it. The audit script is /tmp/merge_audit2.py: for each main commit, check whether the lines it added are present and the lines it deleted are absent, per file, restricted to the files both sides touched. The four remaining hits are accounted for -- `_validate_improvements` is this branch's deliberate split of `_filter_improvements`, and the rest are docstring wording. Signed-off-by: Severin Klingler <sklingler@nvidia.com> (cherry picked from commit 651d45d3fe28fdbb0e028025737abfec4ef14851) * fix(experimentalist): restore the run-config values the refactor disconnected Review found five places where a value the run config declares reached nobody. Each fails the same silent way: the run completes, reports, and picks a winner, against settings other than the ones the file states, while `config_snapshot` records it as correct. **Insight suites reached nothing (P1).** In Mode 1 the Eval Author writes verifiers that emit their own metric keys, plus a suite of production-trace tasks. Neither was used: `with_insight_objective` had no caller outside a test, so selection kept ranking on `reward` — which those verifiers never emit, leaving no candidate eligible to win — and the suite was parked in `datasets["insight"]`, a key nothing read, so its tasks were never evaluated. The host now applies the authored keys as the objective, demoting the configured targets to guardrails, and distributes the suite into the splits the loop evaluates, bracketed by the held-out restore/hide pair. Because the strategy is constructed before the Eval Author runs, it cannot read this off its own config. The effective contract now travels on the context, which is where the host settles it, and `StrategyContext` declares it. **The terminator judged convergence from a partial history (P1).** It received the round's working set — survivors plus the previous round's builds — while convergence counts distinct generations and compares an older Pareto front against the current one. Eliminated candidates were invisible, so with `max_survivors=1` the generation window could never fill and a converged run paid for every remaining round. It now receives the whole evolution tree. **Two values had two homes (P2).** `min_rounds_before_stopping` and `max_trajectory_tasks` existed on the run config *and* on the component config, and the components read their own copy — so the shipped benchmark configs set values that did nothing. Each now has one home, on the run config beside `max_rounds`, and reaches its component the way the objective and regression targets already do. **Reporting fell back to `reward` (P2).** `ExperimentContext` was built without the objective list, so progress lines showed configured metrics as unavailable and `run_finished` omitted the winner's actual scores. Every fix is pinned by a test verified to fail when the fix is reverted. The first attempt at those tests caught only the P2s, which is the same gap that let these through: asserting a component stores what it was handed says nothing about whether the loop hands it over. The terminator and Insight tests now drive the real call site. Signed-off-by: Severin Klingler <sk…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two places where the optimization loop threw away work it had already paid for. A
proposal round died if any single improvement was imperfect, and trajectory
scoring killed the entire run when it met a trace it could not cite. Both now
degrade to less rather than to nothing.
Found while building a deterministic smoke fixture (#1089), but neither is
specific to it.
Changes
Proposer — salvage imperfect proposals. Three conditions discarded a whole
round rather than the offending improvement. The strictest required every
improvement in a round to carry a distinct
optimization_type; nothing justifiesthat, and it also meant a type used once could not be used again later in the
run. Two real runs died on it after round 1, having already produced usable
candidates. Surplus improvements, disallowed types, and duplicate optimization
text now drop the individual improvement and log why. Only an empty result is
fatal. Adds the first tests for this validation, which had none.
GroupLeafScorer — survive a trace with no turns. Every score must cite span
IDs, and the only sanctioned lookups (
get_span_id,get_turn_data) are indexedby turn. An agent that makes no LLM calls produces a trace with zero turns, so
both return
Nonehowever often they are called. The scorer could not satisfyits own contract, retried to the CodeAct ceiling, and raised
GenerationError—and because scoring runs inside
asyncio.gatherwith no handler above it, thatended the run, discarding candidates that had already evaluated successfully.
The evidence was never missing: a turn-less trace still carries a call graph of
which methods ran, in what order, and their status. The contract now names the
observable condition (
Turns: 0), says to leavespan_idsempty, and points atthe call graph instead. Guessed or abbreviated ids are still refused, and traces
that do have turns are unaffected.
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run --frozen pytest plugins/nemo-experimentalist/tests/experimentalist/— 320 passeduv run ruff check/ruff format --checkon the changed files — cleanuv run pre-commit run -a— every hook passes exceptCheck for uv.lock drift, which also fails on a pristineorigin/maincheckout with no changes applied. It isuv 0.9.30on this machine re-locking to a narrower platform set (dropping armv7l, ppc64le, s390x, riscv64 wheels); committing that would break other architectures, souv.lockis deliberately left untouched by this PR.Behavioural verification. The proposer fix was confirmed by a multi-round run
that previously died at round 2 and now completes. The scorer fix was confirmed
by a run against a deterministic agent: the scorer completed and separated two
candidates that were tied on reward, citing method names and statuses from the
call graph.
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests