[RLlib] Add correct terminated and truncated batch sizes on zero-length episodes #46721
Conversation
Signed-off-by: Mark Stephenson <[email protected]>
Mark2000
requested review from
sven1977,
ArturNiederfahrenhorst and
simonsays1980
as code owners
Signed-off-by: Mark Stephenson <[email protected]>
Mark2000
changed the title
|
Bumping this! |
|
@sven1977 Could you review when you're able to? |
anyscalesam
added
triage
|
Sorry to keep bugging you @sven1977 @simonsays1980 , but any way this could get merged in? |
| @@ -100,6 +100,8 @@ def __call__( | |||
| Columns.TERMINATEDS, | |||
| items_to_add=( | |||
| [False] * (len(sa_episode) - 1) + [sa_episode.is_terminated] | |||
| if len(sa_episode) > 0 | |||
There was a problem hiding this comment.
We should probably remove any zero-length episodes in the loop here. They cannot be used for learning anyways.
There was a problem hiding this comment.
Leave this to @sven1977 to decide.
There was a problem hiding this comment.
@sven1977 could we give this a go? I think this is fine like this. The other option is to remove zero-length epiosdes entirely as they offer nothing to learn from. This could also happen if an agent in a multi-agent scenario received only the initial observation and nothing else.
There was a problem hiding this comment.
Thanks for the fix @Mark2000 . LGTM.
A slightly more defensive fix would probably be to filter out empty episodes already in the connector pipeline, but maybe that would confuse some code that does draw information from those.
Our built-in EnvRunners do not return empty episodes ever, which is why this problem never came up.
|
auto-merge enabled ... |
…th episodes (ray-project#46721) Signed-off-by: ujjawal-khare <[email protected]>
Why are these changes needed?
In
AddColumnsFromEpisodesToTrainBatch, it is assumed that eachsa_episodehas length ≥ 1. When a zero-length episode is passed to the connector, the data added to the terminateds and truncateds columns are incorrectly sized; they should just be an empty list. This case generally doesn't come up, but when adding custom connectors that modify episode lengths (e.g. for semi-MDP type problems), zero-length episodes can be produced.Related issue number
Didn't open issue, but I can if necessary.
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.