Skip to content

Avoid requesting policy actions after terminal timesteps - #370

Open
sylvesterkaczmarek wants to merge 2 commits into
google-deepmind:mainfrom
sylvesterkaczmarek:fix/avoid-terminal-policy-action
Open

Avoid requesting policy actions after terminal timesteps#370
sylvesterkaczmarek wants to merge 2 commits into
google-deepmind:mainfrom
sylvesterkaczmarek:fix/avoid-terminal-policy-action

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

Avoid requesting another population action after the substrate has already returned a terminal timestep.

evaluation.run_episode currently sends the terminal timestep to the population and then unconditionally calls await_action(), even though that action can never be consumed because the episode is over.

This change:

  • still forwards the terminal timestep to the population
  • skips await_action() once the terminal timestep has been observed
  • leaves action collection unchanged for non-terminal timesteps
  • adds focused regression coverage

Testing

Added a regression test for a one-step episode and verifies that await_action() is called only for the initial non-terminal timestep, while both the initial and terminal timesteps are still delivered to the population.

@jzleibo

jzleibo commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Can you elaborate on why this is needed? The change makes sense of course, but I've often taken a "if it's not broken, why fix it?" sort of attitude to this kind of thing. Is this fixing a specific issue on your end? I'm happy to approve if so. But if it's just because it "makes more sense this way" then maybe better to leave it alone? What do you think?

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Can you elaborate on why this is needed? The change makes sense of course, but I've often taken a "if it's not broken, why fix it?" sort of attitude to this kind of thing. Is this fixing a specific issue on your end? I'm happy to approve if so. But if it's just because it "makes more sense this way" then maybe better to leave it alone? What do you think?

Good point. This came from code inspection rather than a concrete user-facing or production failure. Population.reset() clears or cancels pending action futures before the next episode, so I don't have evidence that the extra terminal action causes an actual correctness issue. I agree it is better not to change the behavior without that evidence, so I'll close this PR. Thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants