fix: make canonical metrics terminal-info resolution safe - #116
Merged
Merged
Conversation
… downstream consumers - Enforce invariant in extract_episode_metrics where collision=True overrides any conflicting positive success flag to False - Align Evaluator and GeneralizationEvaluator success rate aggregation directly on canonical EpisodeMetrics - Fix Dict type hint in CurriculumTrainer for backward typing compatibility - Add regression tests verifying conflicting info resolution and cross-consumer consistency
…, and canonical public API - Introduce EpisodeMetricsAccumulator for multi-step collision and success tracking across evaluation routines - Preserve traffic-specific evaluation contract requiring controlled queues and no overflow/premature termination - Document canonical rate denominator semantics over evaluation episodes - Update callback fallback extraction to respect truncation/termination flags - Consolidate canonical EpisodeMetrics public API under adaptive_rl.metrics - Add regression tests for multi-step outcomes, traffic overflow, and mixed outcome rates
…tcome policies - Preserve nullable tri-state metrics (None/False/True) end-to-end across models, trainers, and callbacks - Ensure outcome rates compute over defined episodes with valid denominators (returning None when undefined) - Extract traffic domain-specific logic into TrafficOutcomePolicy via composition - Wrap extract_episode_metrics cleanly around EpisodeMetricsAccumulator - Establish adaptive_rl.metrics as single canonical public owner with backward-compatible re-exports - Update benchmark reports, metadata serialization, and experiment records to handle nullable metrics - Add full lifecycle regression tests for None preservation, explicit False, multi-step precedence, and rates
…bility roundtrip - Prevent intermediate success flags from latching into traffic episode outcomes - Add load_episodes_csv to ensure full lossless round-trip for tri-state outcome fields - Document precise PPO/SAC data flow through SB3CallbackAdapter to EpisodeMetricsAccumulator - Safely handle nullable success rates in curriculum rolling metrics and stage advancement - Add explicit regression tests for traffic terminal success rules and CSV serialization roundtrip
…nterpretation and enforce single outcome source of truth
…isode-local outcome isolation
…olation Fix/pr 114 metrics policy isolation
Replace unsafe dict equality comparison in _resolve_episode_step_infos with object identity (is). Environments can include NumPy arrays or other non-scalar values in info dicts, and comparing them with == raises ValueError. Identity is the correct contract here — we only need to skip appending when the caller passes the exact same object.
…olation fix: use identity check for terminal-info deduplication
Owner
|
@AryanXCode646 Can we add an explicit test/documentation for this contract and confirm that callers are expected to preserve the same "info" object when it is already present in "step_infos"? Otherwise, a caller that reconstructs the terminal mapping could still produce a duplicate terminal step without any indication that it is semantically equivalent. |
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
This follow-up hardens the canonical episode metrics pipeline against arbitrary environment telemetry in terminal info mappings.
The previous implementation used dictionary equality to determine whether the terminal info had already been recorded. That is unsafe when telemetry contains non-scalar values such as NumPy arrays, because equality comparisons can raise ambiguous truth-value errors.
Changes
Removed unsafe equality-based comparison of arbitrary telemetry mappings in _resolve_episode_step_infos().
Switched terminal-step deduplication to an explicit, safe representation/identity contract.
Preserved existing episode ordering and terminal-step semantics.
Added regression coverage for terminal info containing NumPy arrays and nested non-scalar telemetry.
Added coverage ensuring an already-recorded terminal mapping is not duplicated.
Verified that existing canonical metrics semantics remain unchanged.
Regression case
The metrics pipeline now safely handles telemetry such as:
{
"success": True,
"queue_lengths": np.array([2, 1, 0]),
}
without attempting to use arbitrary telemetry equality as a boolean predicate.
Validation
Focused canonical metrics tests pass.
Broader test suite passes.
Lint/type checks pass where configured.
No unrelated public API behavior was intentionally changed.
Motivation
This closes the correctness issue identified during review and ensures the canonical metrics layer remains robust when environments provide complex telemetry payloads.