[RLlib] SlateQ training iteration function. #24151
Conversation
sven1977
requested review from
gjoliver,
avnishn,
ArturNiederfahrenhorst and
smorad
as code owners
|
@avnishn , we use the exact same |
…eq_training_itr
…eq_training_itr
| # Update weights and global_vars - after learning on the local worker - | ||
| # on all remote workers. | ||
| global_vars = { | ||
| "timestep": self._counters[NUM_ENV_STEPS_SAMPLED], |
There was a problem hiding this comment.
If this is using LR schedule or Entropy schedule, shouldn't this be num agent steps trained?
There was a problem hiding this comment.
True. The thing is that NUM_AGENT_STEPS_SAMPLED is always a sum over all (multi) agents. So let's say you have 2 agents in your env and 1 policy, which both these agents map to. Then you would update this policy's timestep counter with the sum of these 2 agents' steps, which would be incorrect (as this count is possibly much larger than env steps; double if the two agents always act at the same time).
There was a problem hiding this comment.
So I would leave it for now to reflect our current behavior.
Definitely worth looking into this and maybe provide a better per-policy fix for this.
| # Update weights and global_vars - after learning on the local worker - on all | ||
| # remote workers. | ||
| global_vars = { | ||
| "timestep": self._counters[NUM_ENV_STEPS_SAMPLED], |
There was a problem hiding this comment.
same comment here as above
| "replay_buffer_config": { | ||
| "type": "MultiAgentReplayBuffer", | ||
| # Enable the new ReplayBuffer API. | ||
| "_enable_replay_buffer_api": True, |
There was a problem hiding this comment.
is there a reason that you have to set this and its not just the case already?
Does this make Slateq use another replay buffer api?
There was a problem hiding this comment.
Yeah, it'll use the new replay buffer API we are currently rolling out across RLlib.
…eq_training_itr
…eq_training_itr
…eq_training_itr
…eq_training_itr
…eq_training_itr
SlateQ training iteration function.
_disable_execution_plan_api=Trueby default for SlateQ.Why are these changes needed?
Related issue number
Checks
scripts/format.shto lint the changes in this PR.