Default savedir to None (and remove unused archivefile) #174
Conversation
savedir is a feature that will save ttyrecs of runs - this can be unexpected when training a model and may write a *huge* amount of unwanted files. This changes the default to *not* save ttyrecs. If you want ttyrecs to be saved, please specify savedir when instantiating the env from now on. `archivefile` is a parameter that may have been used at some point, but is currently unused, so let's remove it.
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
Generally sensible wrt savedir, but we need to communicate these changes to AIcrowd.
WRT archivefile: this will break the benchmark training we provide to contestants and if we go ahead, release should be timed to match update of model, action space, etc...
In general, we should just consider that removing archivefile is a breaking change to NLE api - why are we so certain we don't want an archivefile that keeps track of stats from all the ttyrecs that env created? I think this would be useful if you are generating 1000s of ttyrecs, so I think we should think twice before breaking the api, lest we need it again in future.
|
Indeed - I would suggest fixing archivefile and getting a stats/index file that I believe was demonstrably useful for the NLE paper, rather than just canning the last vestigial artifect. |
| @@ -186,8 +186,7 @@ class StepStatus(enum.IntEnum): | |||
|
|
|||
| def __init__( | |||
| self, | |||
| savedir="", | |||
| archivefile="nethack.%(pid)i.%(time)s.zip", | |||
There was a problem hiding this comment.
After talking to Eric he convinced me to leave this here for now in order to not confuse participants too much.
Let's just remove savedir in this PR (and also the archivefile=None calls above, given that the argument gets ignored) and we'll deal with archivefile in a future PR.
There was a problem hiding this comment.
Can archivefile be defaulted to None, too, then? Same argument as for savedir
|
@tscmoo any chance of reverting the deletion of archive file? happy to accept if so |
1 similar comment
|
@tscmoo any chance of reverting the deletion of archive file? happy to accept if so |
|
Have done w @heiner, and will accept. |
|
Ah, sorry, forgot about this. I'll merge it once the tests pass. |
|
Flaky test failing is probably down to starting with 'female gnomish archaeol-More-'.. Rerunning test and will submit a test fix. |
|
Test fix is in #182 |
savedir is a feature that will save ttyrecs of runs - this can be unexpected when training a model and may write a huge amount of unwanted files.
This changes the default to not save ttyrecs.
If you want ttyrecs to be saved, please specify savedir when instantiating the env from now on.
archivefileis a parameter that may have been used at some point, but is currently unused, so let's remove it.