[rllib] Fix error messages and example for dataset writer #23419
Conversation
|
Btw @gjoliver, it seems like final batches are not written because Thus in the example Is this something we can fix? I.e. is there a deconstructor/cleanup method we can use for this? |
| @@ -44,7 +44,7 @@ def __init__( | |||
| output_config: Dict = ioctx.output_config | |||
| assert ( | |||
| "format" in output_config | |||
| ), "output_config.type must be specified when using Dataset output." | |||
| ), "output_config.format must be specified when using Dataset output." | |||
There was a problem hiding this comment.
ah thanks for catching this. went through iterations ...
| @@ -1,7 +1,7 @@ | |||
| # To generate training data, first run: | |||
| # $ ./train.py --run=PPO --env=CartPole-v0 \ | |||
| # --stop='{"timesteps_total": 50000}' \ | |||
| # --config='{"output": "dataset", "output_config": {"type": "json", "path": "/tmp/out"}, "batch_mode": "complete_episodes"}' | |||
| # --config='{"output": "dataset", "output_config": {"format": "json", "path": "/tmp/out", "max_num_samples_per_file": 1}, "batch_mode": "complete_episodes"}' | |||
There was a problem hiding this comment.
just to double check, you want to write 1 sample per file??
There was a problem hiding this comment.
Well 1 sample = 1 episode here so usually has a few hundred timesteps. When I ran this I ended up with 39 files.
There was a problem hiding this comment.
yeah, but still, why write 1 episode per file? :)
the default half-cheetah json dataset is a single file of maybe a few thousand episodes.
is there any efficiency consideration behind this?
anyways though, this is just an example, so whatever you see fit. I am just curious what made you make this change.
There was a problem hiding this comment.
The main reason is that currently remaining samples are not written at all - they are only written when at least max_num_samples_per_file samples are reached during training. I.e. we don't flush remaining results when training is over and I didn't see a great way to make that happen
There was a problem hiding this comment.
oh this is pretty bad. please add a Note or TODO in dataset_writer.py
this is another case where we are using a workaround to fix underlying problems. and per eric's message, we shouldn't do this long-term.
let's intercept a signal somewhere and flush the last shard before things go down.
There was a problem hiding this comment.
Ah yeah that's what I mentioned in my first comment to this PR. Ok, will add a todo in the file. I've tried to flush on __del__ but that doesn't seem to work with actors, so no quick fix. Probably we need to call some kind of deconstructor/cleanup method manually
There was a problem hiding this comment.
ah, it all comes back. I didn't understand what you meant at the time :)
cool, interesting topic ...
| @@ -1,7 +1,7 @@ | |||
| # To generate training data, first run: | |||
| # $ ./train.py --run=PPO --env=CartPole-v0 \ | |||
| # --stop='{"timesteps_total": 50000}' \ | |||
| # --config='{"output": "dataset", "output_config": {"type": "json", "path": "/tmp/out"}, "batch_mode": "complete_episodes"}' | |||
| # --config='{"output": "dataset", "output_config": {"format": "json", "path": "/tmp/out", "max_num_samples_per_file": 1}, "batch_mode": "complete_episodes"}' | |||
There was a problem hiding this comment.
yeah, but still, why write 1 episode per file? :)
the default half-cheetah json dataset is a single file of maybe a few thousand episodes.
is there any efficiency consideration behind this?
anyways though, this is just an example, so whatever you see fit. I am just curious what made you make this change.
Why are these changes needed?
Currently the error message and example refer to a field
typethat is actuallyformat.Related issue number
Checks
scripts/format.shto lint the changes in this PR.