Avalon-Benchmark / avalon

A 3D video game environment and benchmark designed from scratch for reinforcement learning research
https://generallyintelligent.com/avalon/
GNU General Public License v3.0
175 stars 16 forks source link

Update to gym 0.26 #12

Closed timokau closed 1 year ago

timokau commented 1 year ago

Note: I realize that this is a big change and you may decide not to merge this. I worked on it for my own purposes (applying the avalon dreamer baseline to gym 0.26 environments) and thought it would be best to propose it upstream, but I understand if you decide to close the PR. I have also not tested this exhaustively, I have only verified that it works for my purposes. So it is likely that there are still some mistakes and this PR would be a starting point rather than an immediately mergeable.

If you want me to run the test suite, I'd be happy to. In that case please let me know how best to do that (use the dockerfile? pytest?).


See [1] for a list of breaking changes. In particular we now

Note that future Gym development will happen in the "Gymnasium" [2] fork, which is based on Gym 0.26. Migrating from Gym 0.26 to Gymnasium should be easy.

[1] https://github.com/openai/gym/releases/tag/0.26.0 [2] https://github.com/Farama-Foundation/Gymnasium

mx781 commented 1 year ago

@timokau appreciate the PR, thanks! Just to signal intention - this is indeed an upgrade we're looking to do, but we were waiting for the new API to stabilize (perhaps when v.1.0 is released?). Part of the reason is that we run mypy against our entire codebase and would prefer the types to fully settle before making this upgrade.

For now we'll leave this PR open, but you've done a fair bit of legwork here so it should come handy once we upgrade - we'll make sure to attribute appropriately in any commits we pull in - thanks!

timokau commented 1 year ago

Great, thanks for the update :) For what it's worth, I think gym(nasium) 0.26 is supposed to be fairly stable. Quoting from the release notes:

All of the previously "turned off" changes of the base API (step termination / truncation, reset info, no seed function, render mode determined by initialization) are now expected by default. We still plan to make breaking changes to Gym itself, but to things that are very easy to upgrade (environments and wrappers), and things that aren't super commonly used (the vector API). Once those aspects are stabilized, we'll do a proper 1.0 release and follow semantic versioning. Additionally, unless something goes terribly wrong with this release and we have to release a patched version, this will be the last release of Gym for a while.

But yes, the dust has probably settled a bit more by the time 1.0 is released.

micimize commented 1 year ago

Yeah – we'll see. Realistically, there's a decent amount on their roadmap that could end up impacting that plan once they're in the weeds. But if they stay on schedule we should know relatively soon :slightly_smiling_face:

Another consideration for me is that I'd like to put off unpacking done into terminated, truncated until we have the bandwidth to do it rigorously so that the semantic distinction is actually honored, which will entail adding some logic on the godot side and to the bridge. Not unmanageable but will require some attention.

Did you try using EnvCompatibility & were there issues? If not, maybe loosening our dependency constraint and documenting the usage would be a good stop-gap

timokau commented 1 year ago

Another consideration for me is that I'd like to put off unpacking done into terminated, truncated until we have the bandwidth to do it rigorously so that the semantic distinction is actually honored, which will entail adding some logic on the godot side and to the bridge. Not unmanageable but will require some attention.

That makes sense, getting this entirely right probably requires digging a bit deeper.

Did you try using EnvCompatibility & were there issues? If not, maybe loosening our dependency constraint and documenting the usage would be a good stop-gap

No I didn't try that, I don't remember why. Probably should have tried that first.

zplizzi commented 1 year ago

I'm going to go ahead and close this, given that we'll probably start fresh when we're ready to make this change. Thanks for proposing it though!