-
Notifications
You must be signed in to change notification settings - Fork 8.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
Plans for Future Maintenance of Gym #2259
Comments
This is super exciting stuff. Thanks so much for maintaining @jkterry1! |
First of all this is great to have you as a maintainer @jkterry1, and looking forward to what Gym will become. I will happily reopen my stale PRs. It's also great to see the documentation being among the high priority items. This is long overdue, especially since some features such as the However regarding this change (which I understand is lower priority)
I am strongly in favor of keeping this feature. This does not seem to be a breaking feature, and it does not require any maintenance at all (the majority of which is done on the atari-py side, not on the Gym side). It is very dangerous to remove a feature like this, no matter how unlikely people may use it. Edit: I forgot to mention that earlier, but saying that "literally no one uses them" is also not true. See for example the AtariARI environment at NeurIPS 2019. |
Fair enough; I can keep RAM observations as an argument if it comes to that. |
@jkterry1 I can certainly help with adapting the documentation to the new site. If I recall, there were two sites for documentation: https://gym.openai.com/docs/ I presume these would be consolidated and updated? Any additions? You mention a current designer creating a Jekyll theme for this project. I've worked with Jekyll before and am happy to use that. Alternatively, I'm comfortable with any frontend template or code we want to use. @jkterry1 Who would you suggest I coordinate with? |
Great plan! 👍 |
@ZachariahRosenberg I'm very happy to hear that, please email me at [email protected] @lebrice I don't have the relevant permissions |
@ openAI can you please give @jkterry1 the relevant permissions lest we be forced to make a discord |
We've just been using the general purpose RL discord: https://discord.gg/amy9KW4p. There's actually a private Gym maintainers channel. |
I'll release a nee release after CI is fixed, all the old PRs are sorted
out and ideally once ALE-Py is merged. It'll probably take a week.
On Wed, Jul 28, 2021 at 3:33 AM Rai ***@***.***> wrote:
Really good that this piece of common infra will get actively maintained
again. Thanks for taking on the commitment <3
As a useful but easy task, maybe you could release a new version? The
current version has #1925 <#1925>
still open, though the fix in #2151
<#2151> is already in master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2259 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEUF33FWEJYOXP6ICVH54ZDTZ6XGNANCNFSM5BCWLALQ>
.
--
Thank you for your time,
Justin Terry
|
Good to hear =) (even though I wish OpenAI would have done that earlier)
Why such choice and not the opposite? (adding dependencies is something that needs to be carefully discussed, related to DLR-RM/stable-baselines3#524) EDIT: while reading the README, I stumbled upong "New release notes are being moved to releases page on GitHub", why not adding a |
I completely agree with @araffin. What is the goal of deprecating features (3133e99) that are already there in Gym in favor of a third-party library? This feels convoluted and, as far as I understand (I might be wrong, I'm not that familiar with SuperSuit), SuperSuit is missing features that Gym already has, e.g. all the components of the standard pre-processing for Atari, such as no-op reset and terminal on loss of life, while Gym has a all-in-one wrapper What features does SuperSuit have that Gym does not (currently), that would justify pushing users to install a third-party library? How hard would it be to move them to Gym? |
One additional note: I understand and share your enthusiasm about finally having Gym maintained again, but I would highly recommend you to take it slow and make careful decisions about any modification (that is not documentation or fixing typos) to avoid bloating or breaking the package. |
The values of out-sourcing are broadly understood. Avoiding duplication of effort, focusing on real differentiation, moving complexity behind clearly defined interfaces, making the architecture easier for contributors & maintainers to understand. |
So for SuperSuit, like I keep saying, this will not break existing code using the internal wrappers. Too much depends on that. The SuperSuit API is such that using SuperSuit should also basically be drop in for existing Gym functionality. Moreover, SuperSuit has no onerous dependencies. It's a small and lightweight pure Python package. Gym would also not actually depend on SuperSuit in setup.py for installation. There are several reasons why I'd like SuperSuit to be the mainstream solution: In other words, upstreaming SuperSuit's functionality to Gym would create breaking changes to Gym's wrappers (which I don't want to do), it would result in a bunch of code duplication that would create maintenance overhead, and it would be a very large software engineering effort to do so (despite it seeming like a superficially simple one). And again, SuperSuits external API is basically the same as Gym's wrapper API and it's a lightweight pure Python library with no problematic dependencies. I essentially am of the opinion that switching to SuperSuit is the most conservative course of action here given all of this. However, @tristandeleu is correct that SuperSuit does not have all the features Gym does on re-review. In addition to the two you pointed out, SuperSuit does not have the pixel observation, dictionary filtering, time aware observations wrappers or episode statistic recording wrappers and SuperSuit's frame stacking wrapper does not have lazy frame stacking. I'll definitely need to add lazy frame stacking to SuperSuit. Beyond that, I'm currently of the opinion that the 3 missing wrappers I just mentioned are best left as things people can use lambda wrappers for because I managed to become the maintainer of Gym without knowing they even existed (though I could be persuaded otherwise if I'm missing something here). Regarding no-op reset and terminal on loss of life, per discussion with Ben Black having those as wrappers essentially looks like hacks to us? Learning code does not require wrappers to be able to properly function, and the old wrappers will be left in Gym for compatibility purposes with existing code basis that depend on having that logic on those wrappers to function. I'd be willing to add these to SuperSuit if there's an important reason to have this logic in wrappers that's I'm missing though. |
Regarding the changelog, if there's some sort of reason to have changes there instead of with the GitHub releases I could? Putting the changelog in the readme was becoming excessive and it just had to go somewhere. Using the GitHub releases feature is fairly standard across open source projects and it's my personal preference is all. Regarding "I would highly recommend you to take it slow and make careful decisions about any modification (that is not documentation or fixing typos) to avoid bloating or breaking the package." is there anything in particular you're thinking of here that I'm missing? This sounds like my expressed goal as well. The only things I intend to break are what I stated- the vector environment API, because all the RL library developers have a bunch of problems with it and recommending developers use different preprocessing wrappers. |
I do not understand this, what are those breaking changes that you have in mind? Is it to maintain wrappers compatible for both Gym and Petting-Zoo? If so, this compatibility should be enforced by Petting-Zoo/SuperSuit, and should not require changes to Gym; putting it differently, the users of Gym that have always relied on those wrappers should not have to deal with these deprecations because of incompatible APIs in third-party libraries. In order to get a better understanding, I made this comparison table (limited to the wrappers that can be applied to Gym environments, not the multi-agent ones).
As you can see, a majority of the wrappers available in SuperSuit are already there in Gym, and the ones that are not can be easily added, or can be left as lambda wrappers as you said. If I understand correctly, the maintenance overhead seems to come from SuperSuit being compatible with both Gym and Petting-Zoo, and trying to catch up with the Gym wrappers rather than the other way around: wrappers for Gym environments were added in Farama-Foundation/SuperSuit#2 in SuperSuit, while at this point all of these wrappers above (and more) had been added to Gym already by @zuoxingdong (prior to SuperSuit being created). From a UX point of view, I also feel like Gym wrappers as they stand right now are nicer to work with than SuperSuit. For example, if I want to get started with Atari with standard pre-processing, this is readily available in Gym: import gym
from gym.wrappers import AtariPreprocessing
env = gym.make('BreakoutNoFrameskip-v4')
env = AtariPreprocessing(env) Whereas with SuperSuit: import gym
from supersuit import color_reduction_v0, frame_skip_v1, resize_v0
# Eventually: from supersuit import no_ops_v0
env = gym.make('BreakoutNoFrameskip-v4')
env = no_ops_v0(env, 30)
env = frame_skip_v1(env)
env = resize_v0(env, 84, 84)
env = color_reduction_v0(env)
Currently, SuperSuit has a dependency on Petting-Zoo, which means that in order to get standard wrappers for Gym (e.g. the ones for Atari for example), then you'd need to install Petting-Zoo, which is probably unnecessary for any non-multi-agent project. This could be avoided by having an I would understand the move to go for SuperSuit instead of Gym if SuperSuit was overwhelmingly used by the community over Gym, but that does not seem to be the case: for example, comparing Gym's
The vector environment API can definitely be improved! However I think that it could benefit the most from documentation first, to show that this API exists in the first place, rather than breaking it. I also hope that if and when the time comes to break this API, you will keep the community in the loop first before making those changes. |
-"I do not understand this, what are those breaking changes that you have in mind?" Wrapper versioning for reproducibility and fully featured lambda wrappers are the two main ones So far no one other than you has raised major concerns about SuperSuit like has happened with other proposed changes (e.g. removing the Atari RAM wrappers). My plan for now is to privately discuss this with other maintainers of some the major open source DRL libraries and go from there. Your opinion will be taken seriously. |
I'm not sure why
(RL baselines is not a library but a training framework (training/plotting scripts) that uses SB3 library)
this is what software version + proper changelog is for.
I was not talking about making Gym wrappers look like SuperSuit but rather add the missing features/documentation that you consider very useful to the current wrappers (in fact, most gym wrappers are documented, mostly missing an autogenerated doc website with sphinx). What important features do you consider missing from current wrappers? (to estimate the work needed)
thanks for the table @tristandeleu =)
Well, I would say that both work the same way to me: you do
I already count three people ;)
I would rather keep the discussion public for transparency. ==============
yes, as you don't do a release at every commit, it helps users that are based on master version to keep track of changes (or also helps when you are offline). It would also allow to have it in the documentation. From a maintainer point of view, it makes things easier when drafting a release: you just need to copy-paste the changelog entry to Github release and don't have to go through all what have been done between two releases.
Good to hear. I was mostly worried when you merged all that PRs and started changing readme + add deprecation in only two days. |
Want to again say I really appreciate you work in maintaining gym and the amount of effort that was put into supersuit, but I agree that the existing wrappers should not be depreciated. In addition to the Atari wrappers that @tristandeleu mentioned, there are other super helpful wrappers such as Philosophically, I agree with @araffin and prefer the foundational utilities to come from the same repo. I think it creates a better experience. When you see the documentation for the environment, you also see that related wrappers in the same documentation versus opening documentations of different repos. |
why is that? Looking at the comment from @semitable (received by email), it does not contain anything that would justify that (no insult, no off-topic reply, mostly expresses his point of view). Note: this deletion should not be the topic of this issue but a short justification of it would be appreciated |
I think the discussions around Change Log
My understanding of Github releases is that it already bundles several commits with a provided narrative of the changes. What would the changelog have in addition to this? @araffin, can you provide an example of an open source project you think does this well? I certainly agree with you Wrappers As I stated in the beginning of this post, I don't believe decisions regarding Further, the wrappers need to be maintained. The problem with the word, "wrapper", is it makes the concept sound like it's just a nice add-on. But anyone working with However, as @cclauss concisely put it, the question is whether I'd assume, maybe incorrectly, that wrappers will have different dependencies than the gym engine. We wouldn't want breaks in one to hold up development in the other. @tristandeleu @araffin, as you pointed out, the wrappers are a pretty core part of interfacing with the gym. They should probably get more care and attention. |
I greatly appreciate everyone's comments here. I'm going to find another solution to addressing my concerns than deprecating Gym's wrappers for SuperSuit and create a new post about it when I have a plan. I'm locking this post because the comments have derailed from the original goal of this thread, please feel free to create new issues for things as they arise. I'll continue to discuss plans regarding the future of wrappers in the gym maintainers chat on discord before offering a new plan up to the public for comment. |
The wrapper deprecation will be reverted in this PR: #2273 |
So OpenAI made me a maintainer of Gym. This means that all the installation issues will be fixed, the multi-year backlog of PRs will be resolved, and in general Gym will now be reasonably maintained.
I, or specific people I know, plan to do the following:
Deal with all the outstanding PRs. (Thanks @benblack769, @cpnota, @vwxyzjn, and those still working through theirs!)Add the following warning to preprocessing wrappers: ""Gym's internal preprocessing wrappers are now deprecated. While they will continue to work for the foreseeable future, we strongly recommend using SuperSuit instead: https://github.com/PettingZoo-Team/SuperSuit"Fixes to CI (though if anyone knows what they're doing with this, please reach out)I am actively soliciting contributions for the following:
Fixes to code style (use the same style tests as either PettingZoo does or SB3 does and that to CI tests once they're properly functioning)(Thanks @cclauss !)Built in API compliance testing (Similar to what PettingZoo has for environments and what SB3 added for Gym environments)(Thanks Add compliance testing for API #2289)I am not interested in contributions for the following:
New environment preprocessing wrappers (SuperSuit has more features and is better for this by every measure)There are certain things that I would like to do, but I cannot immediately do them for various reasons (this may change in the future):
Remove the RAM observation based Atari environments because literally no one uses themMake the RAM observation mode and argument of the regular Atari environmentsIt is also important to note that beyond the described pieces of maintenance and merging PRs, I physically do not have the time to add new features or fix bugs myself right now. I also will likely not be able to generally respond to issues do to the fairly high number, though I highly encourage those in the community to do so and would be very appreciative of this. This is not because I do not want to, this is because I am only one person and have a tremendous number of other professional obligations in my life. In other words- if you want a feature added or a bug fixed, please submit a PR, and please submit the PR with a sufficiently detailed description that I can figure out what's happening in it at 2am when my brain is fried.
Edit:
An astonishing number of people have messaged me about open source donations, so I created links. This money goes to me directly, and has no affiliation or connection to OpenAI (who does not pay or employ me).
https://liberapay.com/jkterry
https://www.buymeacoffee.com/jkterry
The text was updated successfully, but these errors were encountered: