[RLlib] Also tag set_state #57651
[RLlib] Also tag set_state #57651
Conversation
| if num_healthy_remote_workers > 0: | ||
| async_results = self.env_runner_group.foreach_env_runner_async_fetch_ready( | ||
| func="sample_get_state_and_metrics", | ||
| tag="sample_get_state_and_metrics", |
There was a problem hiding this comment.
Not the reason for this PR, just an addition I picked up on the way.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds tagging to set_state calls to enable limiting in-flight requests, which is a good step towards better resource management. The changes in rllib/algorithms/impala/impala.py are correct. However, in rllib/env/env_runner_group.py, the switch from synchronous to asynchronous calls introduces a few issues. For sync_env_runner_states, a more efficient fire-and-forget method is available. More critically, for sync_weights, the change alters the waiting behavior of timeout_seconds, which could lead to unexpected behavior if callers rely on its previous synchronous nature. I've added specific comments with suggestions.
2 participants
Why are these changes needed?
Changes should have been part of #56953.
The reason we did #56930 and #56953, was that any kind of task would always be tagged and therefore limited per actor/type of task.