Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RLlib; Offline RL] Support writing and reading composite spaces samples. #47046
[RLlib; Offline RL] Support writing and reading composite spaces samples. #47046
Changes from 3 commits
23b12af
4139560
26a7dcc
2efa1e9
c4cef4d
9a8b51d
e688111
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we merge the other (
__env__
constant) PR into this one and then push again. This way, we won't forget to change this here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if
observation_space
is None here? Could that happen? Or should we make the arg non-optional?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could happen and is in fact okay. As it uses
from_jsonable_if_needed
the conversion simply does not take place, if the input space isNone
. We need them only, if we want to convert from JSONable data types to a composite space type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated question on the above:
{}
?else {},
would yield a tuple, correct? So the resulting final list would be:[{}, ({},)]
. Maybe I'm wrong, but can you check this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, because there is only a single INFO column in all the offline data. As we need two in the
Episode
we need to fill in a default and this is filled in at timestep zero.YOur second point is valid - this could lead to a tuple, even though the comma should be part of the list ... I check this. Thanks1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, makes sense. We don't have a
NEXT_INFOS
.Hmm, I wonder whether this could be a general problem in some strange use cases :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking about it, but could not came of with cases that need an
info
at ts=0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for
action_space
. What if it's None (not provided by caller)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above ;)