fix(rollout/session): reseed instead of erroring when no assistant persisted - #1009
fix(rollout/session): reseed instead of erroring when no assistant persisted#1009DavidBellamy wants to merge 1 commit into
Conversation
…stant persisted When the agent retries with a divergent user message before the first assistant turn has been persisted, _try_detect_and_rollback_to_assistant _checkpoint previously raised MessageValidationError because the matched prefix contained no assistant checkpoint to roll back to. This killed trials that would otherwise recover on retry. Observed repro: the first LLM turn of terminus-2 hit max_tokens=2048 truncation, errored out before the assistant response could be stored, and the agent retried with a modified user prompt. Stored=[sys,user], request=[sys,user_modified,...], match_len=1, no assistant in stored[:1] -> raise. battery-charging-optimization trials 6 and 7 on RL360 job 1565412 hit this path and failed outright rather than just returning reward=0. Fix: in the checkpoint_index < 0 branch, if num_assistant == 0 the stored state is a prompt-only seed whose first assistant turn never persisted. Clear the session and return True so the caller treats this as a fresh session. The existing raise path is preserved for cases where at least one assistant has been stored (caller is trying to jump back across an assistant turn, which remains disallowed). The caller (prepare_pretokenized) now early-returns None on the reseed signal, equivalent to the existing empty-session branch.
There was a problem hiding this comment.
Code Review
This pull request introduces a 'pre-assistant re-seed' mechanism to handle session divergences that occur before the first assistant turn is persisted. By resetting the session state instead of raising a validation error, the system can recover during retries. Feedback indicates that the new logic branch might be unreachable due to existing early return guards in 'prepare_pretokenized' and the rollback method itself, which may need adjustment to allow the re-seed logic to execute.
| if self.num_assistant == 0: | ||
| # Pre-assistant re-seed: stored is a prompt-only state from | ||
| # a prior request whose first assistant turn never | ||
| # persisted. Clear state and let the caller treat this as a | ||
| # fresh session. | ||
| logger.info( | ||
| "Reseeding session: no assistant checkpoint stored yet, " | ||
| "request diverges at index %d (stored=%d msgs, request=%d msgs)", | ||
| match_len, | ||
| len(stored), | ||
| len(request_messages), | ||
| ) | ||
| self.messages = [] | ||
| self.trajectory_token_ids = [] | ||
| self.records = [] | ||
| self.num_assistant = 0 | ||
| return True |
There was a problem hiding this comment.
The logic in this new branch appears to be unreachable from prepare_pretokenized given the current implementation:
- In
prepare_pretokenized(line 76), the function returnsNoneifself.token_idsis empty. Sinceself.token_idsis derived fromself.trajectory_token_ids, andself.num_assistantis kept in sync with the length ofself.trajectory_token_ids(seeupdate_pretokenized_stateat line 145),num_assistant == 0impliestoken_idsis empty. Thus,prepare_pretokenizedwould have already returnedNonebefore calling this method. - Even if called,
_try_detect_and_rollback_to_assistant_checkpointreturns early at line 213 ifself.trajectory_token_idsis empty (which is the case whennum_assistant == 0).
If the intention is to handle reseeding when a divergence occurs before the first assistant turn is persisted, the check at line 76 in prepare_pretokenized (and line 212 here) might need to be adjusted. Additionally, when implementing logic to reset or rollback session state, ensure all generated outputs and metadata fields are cleared to their default state to prevent carrying over stale data from previous attempts.
References
- When implementing a function to reset objects for retry, ensure all generated outputs and metadata fields are cleared to their default state to prevent carrying over stale data from previous attempts.
|
Thanks for contribution. I understood that this PR intended to handle "The agent retries with a divergent user message" case. But we think that current session server design should only handle append-only trajectory... and diverge should be limited to one step |
When the agent retries with a divergent user message before the first assistant turn has been persisted,
_try_detect_and_rollback_to_assistant_checkpointraisesMessageValidationErrorbecause the matched prefix contains no assistant checkpoint to roll back to. This kills trials that would otherwise recover on retry.Repro
Observed on two Harbor trials (
battery-charging-optimization#6 and #7) aborted with:Chain of events:
max_tokenstruncation and errors out before the response can be persisted.[system, user]withnum_assistant=0.match_len=1(system matches, user diverges); no assistant in stored[:1], socheckpoint_index=-1, and the function raises.Fix
In the
checkpoint_index < 0branch, ifnum_assistant == 0the stored state is a prompt-only seed whose first assistant turn never persisted. Clear the session and returnTrueso the caller treats this as a fresh session. The existing raise path is preserved for cases where at least one assistant has been stored (rollback past an assistant turn remains disallowed).The caller (
prepare_pretokenized) early-returnsNoneon the reseed signal, equivalent to the existing empty-session branch.Diff shape
_try_detect_and_rollback_to_assistant_checkpoint: return typeNone->boolnum_assistant == 0branch inside the existingif checkpoint_index < 0:return Falseprepare_pretokenizedpropagates the reseed signalNo behavior change for any case where at least one assistant has been persisted.