Add option to save ttyrec files every "M" episodes #260
Conversation
facebook-github-bot
added
the
CLA Signed
|
@heiner so weird - this passed all the checks |
Is this about the test failing in https://github.com/facebookresearch/nle/pull/259/checks?check_run_id=3667649209 ? That's a flake, showing up only in a small percentage of cases (due to the NetHack RNG and perhaps also the random actions supplied). |
|
BTW it would be better for this PR to not contain the changes from #259. Some git-fu would make that possible. |
|
Yeah - I remember you mentioning you squash commits, so I can rebase and drop the plot commits |
|
@heiner Done! sorry - i forgot you guys squash commits |
dmadeka
changed the title
| @@ -216,6 +216,7 @@ def __init__( | |||
| allow_all_modes=False, | |||
| space_dict=None, | |||
| spawn_monsters=True, | |||
| episode_save_cycle=1, | |||
There was a problem hiding this comment.
While I like this idea, I think this is also a good opportunity to rethink this API a bit.
We've not done some changes to this API that we wanted to do in the past (e.g., remove archivefile which doesn't do anything anymore) in order to not create undue churn during the NetHack competition.
After the competition, we have another chance getting some of the behaviors right. I could imagine:
- Calling this
save_episode_every, or evenepisodes_per_ttyrec(there might be something better). - Right now,
savedir=Nonedisables saving of episodes.savedir=''is a magic argument that creates a random temporal directory. Any other string is taken as a path. That is a bit too magical. With this new argument, we could turn0into "don't save at all", which would freesavedir=Noneto mean whatsavedir=''does right now.
But we'll need a bit of time to discuss between @condnsdmatters, @tscmoo, @samvelyan and myself.
There was a problem hiding this comment.
Yeah - this is a lot faster, so internally we're working off this branch. If you guys have a slack channel (with public sub-channels) happy to weigh in or continue off our internal branch
There was a problem hiding this comment.
Not saving any ttyrecs is even faster ;)
But I get that having some ttyrecs is useful, too.
We don't currently have slack channel, but you can reach us on the NetHack competition discord. Note though that the competition runs until mid October and we are probably not going to make API changes until then -- would you be okay with working on your own branch until then?
There was a problem hiding this comment.
Yeah - Im fine with that (CC: @deanpfoster)
| @@ -460,7 +465,12 @@ def reset(self, wizkit_items=None): | |||
| """ | |||
| self._episode += 1 | |||
| new_ttyrec = self._ttyrec_pattern % self._episode if self.savedir else None | |||
| self.last_observation = self.env.reset(new_ttyrec, wizkit_items=wizkit_items) | |||
| if self._episode % self._episode_save_cycle == 0: | |||
There was a problem hiding this comment.
We can/should probably roll that into the logic of the line before.
There was a problem hiding this comment.
yeah i just commented on my own repo - we can do that too
|
Merging this into a dev branch for further refinement of the API. |
Initial attempt to add Episode Save Cycle
Initial attempt to add Episode Save Cycle
|
@heiner Will get back to this after the COLT deadline! |
@heiner Solves #256