fix(experimentalist): retain the baseline when no candidate beats it - #1195
Merged
schuellc-nvidia merged 3 commits intoAug 10, 2026
Merged
Conversation
Finalization picked the winner with `select_diverse_survivors(..., k=1)`. That agent is prompted to always include a candidate created in the current round, so at k=1 the rule consumes the only slot and the round-0 baseline can never be returned. Every run therefore shipped a diff, including runs where no candidate measured better than doing nothing. Winner selection is not the same question as survivor selection. Picking a set to carry forward wants diversity and fresh blood; picking a winner wants the best measured result, and no judgement call. Finalization now uses `select_winner_node`: the Pareto front by the configured objectives, with the lowest round winning ties. A candidate that genuinely improves dominates its ancestor and removes it from the front, so the age preference only ever decides ties and never blocks a real gain. Mid-loop survivor selection is unchanged and still uses the diversity selector. Objective-metric eligibility (`has_metric_dimensions`) and `pareto_objectives` ranking are both preserved. The previous behaviour was correct only by accident: `pareto_front` preserves input order and agent-0 is inserted first, so ties fell to the baseline. The rule is now stated explicitly rather than relying on dict ordering. This path had no test coverage, which is how the regression shipped. Adds `test_winner_selection.py` pinning the tie case, order-independence, real improvements, regressions, multi-objective trades, and minimized objectives. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe experimentalist loop now builds a baseline-aware finalization pool, filters candidates that regress against the baseline, ranks the remaining Pareto front, and selects a deterministic winner. Tests cover objectives, protected metrics, killed baselines, missing metrics, and fallback behavior. ChangesWinner selection
Sequence Diagram(s)sequenceDiagram
participant Finalization
participant finalization_pool
participant select_winner_node
participant pareto_front
Finalization->>finalization_pool: collect eligible candidates and baseline
finalization_pool-->>Finalization: candidate pool and baseline reference
Finalization->>select_winner_node: pool, objectives, and baseline
select_winner_node->>select_winner_node: remove regressing candidates
select_winner_node->>pareto_front: rank eligible candidates
pareto_front-->>select_winner_node: non-dominated candidates
select_winner_node-->>Finalization: deterministic winner or None
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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/loop.py`:
- Around line 149-150: Add a deterministic creation-order secondary key to the
winner selection in pareto_front/min within loop.py, while retaining round as
the primary key. In
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py
lines 149-150, update the min key using the existing creation-order symbol. In
plugins/nemo-experimentalist/tests/experimentalist/test_winner_selection.py
lines 78-89, add a reversed-input test covering equal-round, non-dominated
candidates and asserting the same creation-order winner.
🪄 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: e62da7b5-debd-4ff0-a762-c19e1f2f6ab4
📒 Files selected for processing (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_winner_selection.py
Contributor
|
Review follow-up. Three gaps in the first pass. `regression_metrics` was dropped. The old finalization call passed it to the selector; ranking on the objectives alone let a candidate buy an objective gain with a regression the config says must not happen. Candidates that worsen a regression target against the baseline are now filtered out before ranking. The baseline is exempt from its own comparison, so a round where every candidate regresses keeps the baseline rather than shipping the least-bad regression. A target absent from either side is skipped rather than treated as a regression: the evaluator not reporting a dimension is not evidence of harm. Tie-breaking relied on input order. `min` returns the first minimum and `pareto_front` preserves input order, so two same-round candidates that nothing dominates could swap winners if the caller reordered its input -- the same implicit-ordering dependence this change set out to remove, one level down. Ties now fall to creation order, recovered from the run-scoped label's numeric suffix so `agent-9` precedes `agent-10`. The docstring described how the previous behaviour broke. That belongs in history, not in an interface contract, so it now states what the function guarantees and the reasoning stays in the commit that made the change. Adds nine tests: regression in both directions, an objective gain that cannot pay for one, multiple objectives alongside a protected metric, every candidate regressing, a metric missing from one side, reversed input at equal round, and numeric-versus-lexicographic creation order. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py`:
- Line 1847: Update _finalize so the round-0 agent-0 baseline remains eligible
for final winner selection even when survivor selection marks it non-surviving;
alternatively compare all candidates against agent-0 independently of
is_survivor. Ensure select_winner_node receives the real baseline context,
preventing regressions against agent-0 from winning, and add an _finalize
integration test covering this case.
🪄 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: dd101a15-2269-49e6-b1e9-ceae7aadf388
📒 Files selected for processing (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_winner_selection.py
…d-loop Review follow-up. Baseline retention still had a second way to fail. The candidate pool passed to survivor selection starts as the baseline and stays in it every round -- `candidates = list(evolution_tree.survivors(0))`, then `candidates = survivors + new_candidates`. Everything the selector does not keep gets a `killed_round`, and that selector is the one told to always keep a candidate created this round. So the baseline can be killed mid-loop. `_finalize` filtered on `is_survivor`, so a killed baseline never reached winner selection. Two consequences, both silent: a run whose baseline was killed could only choose among diffs, and the regression comparison rehomed onto the oldest surviving candidate, measuring "does not regress" against a candidate that had itself already regressed. Adds `finalization_pool`, which re-admits the round-0 node regardless of `is_survivor` and returns it separately as the regression reference. Survivor selection and finalization answer different questions -- "what is worth carrying forward" versus "is any of this better than shipping nothing" -- and only the second requires the baseline to remain on the table. `select_winner_node` now takes that baseline explicitly rather than inferring it from the oldest eligible node, which is only the same thing when the baseline survived. A baseline lacking the configured objective dimensions cannot be ranked, so it is not used as a reference and selection falls back rather than failing. Adds five tests over an EvolutionTree with a killed baseline, including that the regression reference stays the real baseline rather than the oldest survivor. Signed-off-by: Christian Schüller <cschueller@nvidia.com>
gaiadilorenzo
approved these changes
Aug 10, 2026
schuellc-nvidia
deleted the
fix-winner-selection-baseline-retention/cschueller
branch
August 10, 2026 12:33
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Finalization picked the run's winner with
select_diverse_survivors(..., k=1). That agent is prompted to always include a candidate created in the current round, so atk=1the rule consumes the only slot and the round-0 baseline can never be returned. Before this change, every run shipped a diff — including runs where no candidate measured better than doing nothing. After it, finalization is deterministic: the Pareto front over the configured objectives, with the oldest candidate winning ties, so a run that finds no improvement keeps the baseline.Winner selection and survivor selection are different questions. Picking a set to carry into the next round wants diversity and fresh blood; picking a winner wants the best measured result and no judgement call. Mid-loop survivor selection is untouched and still uses the diversity selector.
Changes
select_winner_node()inexperimentalist/components/loop.py:pareto_frontoverpareto_objectives, thenmin(front, key=round). Used by_finalizein place of theselect_diverse_survivors(..., 1, ...)call.has_metric_dimensionseligibility filtering andpareto_objectivesranking are unchanged.select_diverse_survivorscall site unchanged.plugins/nemo-experimentalist/tests/experimentalist/test_winner_selection.py(10 tests). This path had no coverage, which is how the regression shipped.Preferring the lowest round only decides ties: a candidate that genuinely improves dominates its ancestor and removes it from the front before the tie-break is consulted, so it can never block a real gain. The previous behaviour was correct only by accident —
pareto_frontpreserves input order andagent-0is inserted first, so ties happened to fall to the baseline. The rule is now stated explicitly rather than relying on dict insertion order.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
The one failing hook is environmental and pre-existing, not caused by this change. It was reproduced on pristine
mainwith these changes stashed, and fails identically. This PR modifies nopyproject.tomland nouv.lock; the separate "Check for uv.lock drift" hook passed. It needs uv 0.9.14 locally, which this machine does not have. TheVerificationcheckbox above is left unchecked rather than claiming a gate that did not pass.End-to-end behaviour was also confirmed against a fixture whose expected outcome is known in advance: a single-round generalization scenario where all three agents tie at 1/3 on validation now finalizes with
winner=agent-0(the baseline). Under the previous code the baseline was structurally ineligible, so a candidate that improved nothing would have been published as the winner.Provenance
The finalization behaviour corrected here was introduced in #1107, which also added
selector.py. Before that change, finalization ranked withpareto_frontand did not call an agent.Summary by CodeRabbit