fix(experimentalist): stop discarding usable work in the optimization loop - #1163
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
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
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