drop faild - #1945
Conversation
ErenAta16
left a comment
There was a problem hiding this comment.
Filtering failed rollouts out of training batches is the right idea, and gating it on mode == "train" so eval still sees them is correct: eval wants the failures in the denominator.
return filtered or samples inverts the behaviour in the worst case.
filtered = [sample for sample in samples if sample.status != Status.FAILED]
return filtered or samplesWhen every rollout in the group fails, filtered is empty and therefore falsy, so the function returns samples, which is the list of failures in full. The guard is presumably there to avoid handing an empty batch downstream, and that concern is real, but the effect is that the one case where dropping matters most is the case where nothing is dropped. A step where all rollouts failed then trains on all of them.
The two outcomes worth choosing between are an empty batch, which the caller has to handle, and skipping the step entirely with a log line saying why. Both are recoverable. Silently training on a batch of known-failed samples is not, because nothing downstream can tell it apart from a normal batch.
If the empty-list path genuinely cannot be handled by the caller today, then at minimum this wants a warning at the point the fallback triggers, so an operator seeing bad convergence can find it. A silent or is the hardest version to diagnose.
The helper is duplicated verbatim. The same function body appears in both agent_in_localhost_loop.py and agent_in_sandbox_loop.py. They will drift, and the filtered or samples decision above is exactly the kind of thing that gets fixed in one copy and not the other. Worth putting it next to RolloutState/Status so both loops import it.
Minor, but the commit and PR title is "drop faild", which carries a typo into the history and does not say what changed. Something like "drop failed rollouts from training batches" would make this findable later, and the body is currently empty, so the filtered or samples decision has nowhere to be explained.
No description provided.