-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] In the Json_writer convert all non string keys to keys #33896
[RLlib] In the Json_writer convert all non string keys to keys #33896
Conversation
Signed-off-by: Amog Kamsetty <amogkam@users.noreply.github.com>
… from-items-force-simple
…_writer_convert_keys_to_string
Signed-off-by: Avnish <avnishnarayan@gmail.com>
Signed-off-by: Avnish <avnishnarayan@gmail.com>
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.
all ray data changes in this pr are from #33837 and should be ignored during review.
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.
ptal at this file
rllib/offline/dataset_writer.py
Outdated
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.
ptal at this file
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.
ptal at this file
rllib/offline/json_writer.py
Outdated
return v | ||
|
||
|
||
def _convert_keys_to_strings_json_dict(d: Dict[Any, Any]) -> Dict[Any, Any]: |
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 am actually not sure if you can do this.
like what should we do after we read this json data back in? how would we know which key to convert back into integers?
maybe just clear infos column here for now?
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.
Ok I'm in agreement sure.
Strange, I would think the error about "can't pickle _thread.lock objects" would be related to #33660, but it appears that "Dataset tests (Arrow nightly)" is passing right now on the main branch. |
Signed-off-by: Avnish <avnishnarayan@gmail.com>
…_writer_convert_keys_to_string
we need to wait for #33837 to be merged but otherwise this is ready to go afaict. |
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.
ok, this looks reasonable.
can we wait for Amog to merge his PR, then rebase here, so we can have a final look before merge?
thanks for the fix.
…_writer_convert_keys_to_string
…_writer_convert_keys_to_string
Signed-off-by: Avnish <avnishnarayan@gmail.com>
as it turns out there was a way to store infos in tensorflow for training. Its not publicly documented, but there is 1 test for it, so there were 2 occurences in the codebase. This whole pr pretty much disables you from being able to write env infos out in a sample batch, so we'll allow it to still be used for training, but not allow it for output writing. Signed-off-by: Avnish <avnishnarayan@gmail.com>
@@ -80,20 +80,6 @@ def test_agent_output_logdir(self): | |||
agent = self.write_outputs("logdir", fw) | |||
self.assertEqual(len(glob.glob(agent.logdir + "/output-*.json")), 1) | |||
|
|||
def test_agent_output_infos(self): |
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.
as it turns out there was a way to store infos in tensorflow for training.
Its not publicly documented, but there is 1 test for it, so there were
2 occurences in the codebase. This whole pr pretty much disables you from being able
to write env infos out in a sample batch, so we'll allow it to still be used for training,
but not allow it for output writing.
I removed the test where we check for env infos in the output batch for the time being.
cc @gjoliver
…ay-project#33896) Signed-off-by: Avnish <avnishnarayan@gmail.com> Signed-off-by: elliottower <elliot@elliottower.com>
…ay-project#33896) Signed-off-by: Avnish <avnishnarayan@gmail.com> Signed-off-by: Jack He <jackhe2345@gmail.com>
Signed-off-by: Avnish avnishnarayan@gmail.com
Currently ray data has 2 formats in which it can store python dictionaries as datasets internally.
In case 1, when writing data out to a json file, the data will be written out in the expected schema. e.g. {YOUR DATA: ..., }.
IN case 2, when writing data out to json, the schema is {"value": {YOUR DATA: ....}.
Ray data tries to go for 1 always but falls back onto 2 in the case that the dictionary being converted to a dataset can't be processed by arrow.
One of the reasons for fall back from 1 to 2 is if any of the keys in this dictionary are not strings
IN RLLIB this happens whenever our sample batch includes environment infos, and those environment infos have non string keys. This happens when the env infos are mapping agent ids to infos and those agent ids are integers.
Env infos are included as a part of the batch with torch but not tensorflow which is why we saw this issue when we made the default framework torch last week.
The solution for now is to convert non string keys to string keys, but to also force ray data to always do 1 and use py arrow to store the underlying data. We do this with the
output_arrow_format
flag added tofrom_list
in #33837Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.