Skip to content
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

Adding wrapper for gym #499

Merged
merged 18 commits into from
Mar 31, 2022
Merged

Conversation

arjun-kg
Copy link
Contributor

@arjun-kg arjun-kg commented Mar 7, 2022

Adding Gym Wrapper for Vizdoom

  1. Base env, env definitions, documentation borrowed from vizdoomgym with minor changes wrt recent gym API (0.22.0).
  2. env.render is done using pygame after extracting it from vizdoom's screen buffer. Can maybe use Vizdoom API directly if gym rendering API is changed.
  3. Added a test to check the env for different obs types for every configuration. (health, position, labels, depth = True / False). Another test to check obs at terminal state, since this is handled separately.
  4. Unsure about the folder placement. I felt this was okay. Added commands to copy it to build destination.

@jkterry1
Copy link
Member

jkterry1 commented Mar 7, 2022

Hey, I'm the maintainer of Gym. I asked Arjun to work on this because vizdoom is by far the largest RL project that doesn't use the gym interface (which virtually all standard learning libraries support), so adding proper Gym support on top of your API is fairly advantageous to both this project and the general community. While third party gym support existed, that project is no longer maintained and having the support be natively included is helpful in the standardization of new APIs and for accessibility to beginners.

I'm also happy to maintain this help wrapper as needed so you guys don't have to deal with it.

Copy link
Collaborator

@Miffyli Miffyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh big thanks to you guys! This has been a requested feature for years but nobody has had the time to work on it 😅 . Big thanks for figuring out the cmake stuff especially, that was the major roadblock here!

vizdoomgym is a good start but I have some major comments, some of which I put in the code:

  1. Hmm I wonder if we should use opencv for rendering instead of pygame. I feel that opencv has less dependencies itself and is more common in scientific environments. The rendering code might be a step easier (cv2.imshow, cv2.waitkey and cv2.close functions only needed).

  2. To stay consistent with the scenario configs, the env should not override the settings, apart from the necessary ones (e.g. show_window). This would also mean including GameVariables that are set by the config file in the observation (a new array).

  3. action_space should read game.get_available_buttons_size to dynamically adjust the spaces.Discrete size. Also, since we have one-hot type of action space (one button can be pressed down), the documentation should reflect this and there should be a check for invalid buttons (i.e. DELTA) buttons.

  4. Frameskip should ideally be a parameter to the environment. Yes, you could do this with a wrapper, but it is much more efficient to have it as part of the make_actions call.

Those are comments I can come up with for now ^^'.

I realize this is a mouthful amount of comments. I would be happy to help out getting this into a better condition (I have coded vizdoom env code few times), and test it out, if you have no objections.

gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
gym_wrapper/base_gym_env.py Outdated Show resolved Hide resolved
@arjun-kg
Copy link
Contributor Author

arjun-kg commented Mar 8, 2022

@Miffyli thank you for the detailed comments!

  1. Hmm I wonder if we should use opencv for rendering instead of pygame. I feel that opencv has less dependencies itself and is more common in scientific environments. The rendering code might be a step easier (cv2.imshow, cv2.waitkey and cv2.close functions only needed).

I used pygame solely because it was being used in gym by other envs. In hindsight, it was used to mostly draw stuff which is not needed here. I can replace it with cv2.

I realize this is a mouthful amount of comments. I would be happy to help out getting this into a better condition (I have coded vizdoom env code few times), and test it out, if you have no objections.

Would love your help :) For tests, right now, github workflow doesn't run them. The test I wrote worked fine locally. I tried adding tests to the workflow and run it locally through act, it ran into some issues (something with game.init() crashing) which I was not familiar with. Also the other tests took a really long time to complete on my pc so I just let it be.

I think all the other comments make sense. I'll modify whatever I can first and reply in detail.

@jkterry1
Copy link
Member

jkterry1 commented Mar 8, 2022

@Miffyli

pygame is a core dependency of a ton of gym and pettingzoo and and opencv is being removed in favor of tiny scaler (a new custom library we wrote) because we've had so many issues with pyglet and opencv installations. They've been the largest source of github issues for gym (and supersuit wrappers in pettingzoo) for years. I very much think it's the least awful option, and if nothing else is the most used by related packages now.

All your other comments make sense to me :)

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 8, 2022

@jkterry1 Fair point :). OpenCV can be its own can of worms indeed. Gym is made as an optional dependency here as well (thank you for that!), so I think it will be fine. Only thing I would ask as an extra is making pygame optional requirement: I do not know exactly what dependencies it has, but ViZDoom has been pretty minimal on dependencies and I can see a situation where somebody wants to use Gym but installing pygame gives headaches.

In any case, that is a "nice to have", so no need to implement it yet! I might look into it once other major things have been handled :)

nitpick: should the directory be named just "envs" or something else that is consistent with gym libraries? gym_wrapper is fine by me, though.

@mwydmuch poking in case you want to give input :)

@mwydmuch
Copy link
Member

mwydmuch commented Mar 9, 2022

@arjun-kg many thanks for the PR, @jkterry1 for the input, and @Miffyli for starting the review. I will take a look and add my input this weekend, mostly on how to handle some ViZDoom stuff, I trust you with the rest :)

@Miffyli as far as I remember (might be wrong), PyGame doesn't have any Python deps, it's built on top of SDL2, which is I think included in the wheel, and is also one of the main ViZDoom deps, so I think we are good from this point of view.

@arjun-kg
Copy link
Contributor Author

arjun-kg commented Mar 9, 2022

@Miffyli I've made some changes. I hope I have addressed all your comments :)
I have added a condition to check for DELTA actions. In the given scenarios, VizdoomDeathmatch-v0 has a DELTA action, so making this environment throws an error. I'm not sure how to resolve this, I've just left the environment there for now.

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 10, 2022

@arjun-kg Looking good, thanks for the quick replies! I will test the code out properly after the weekend when I am back at proper machine :).

Regarding DELTA actions: now on a fresh thought we could just skip them an print out a warning with warnings.warn, informing that DELTA actions are simply discarded. Deathmatch still has all the other actions so that works. I can add this and tidy up for approve when I look into this, though :)

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 15, 2022

Ok I made some changes. Can you comment/take a look over that you agree @arjun-kg :)

  • Added a warning for DELTA buttons (not supported, simply ignored)
  • Removed the "depth", "label" etc arguments for the environment, and instead they are inferred from the config. I feel fixing all config stuff into one place is healthier for everyone.
  • Changed observation space to Dict (easier to handle), and removed the "return only spaces.Box obs if there is only one obs". I feel this should be done by a wrapper.
  • Cleaned up duplicate code (e.g. gym envs can now be all instances of one class but with a specific parameter).

One problem now is that the tests do not test all labels/depth buffers too accurately (because they are not controlled by the env) 😅. We could add some custom scenario files to adjust for that.

Another big thing I would like to do is training agents with this wrapper with, say, stable-baselines3. While code runs fine, I just want to make sure the data is not mangled up in some nasty way that would prevent agents from learning. At the same time I could add quick example on how to train agents in ViZDoom with stable-baselines3. @mwydmuch ?

Edit: TODOs for myself:

  • Update docs/README with this info
  • (optionally) do the SB3 examples.

@arjun-kg
Copy link
Contributor Author

@Miffyli Thanks for all the changes! :) Looks great!

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 16, 2022

Added stable-baselines3 example for a more tangible example on training agents, and they seem to be learning well :).

@mwydmuch I will let you approve and merge so you can have a look-over :)

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 22, 2022

@mwydmuch polite poke so that this is not left hanging :). A release at the same time might be useful so people can pypi install and start using wrappers.

@arjun-kg @jkterry1 Are you guys planning to add vizdoom on some list of "gym-compliant" envs once this is done? I can quickly write a PR on this.

@jkterry1
Copy link
Member

@Miffyli yes we are, that'd be great

… other than RGB24 or GRAY8, fix displaying of GRAY8 images by PyGame
@mwydmuch
Copy link
Member

mwydmuch commented Mar 29, 2022

@arjun-kg, @jkterry1, @Miffyli I'm sorry that it took me so long to check this PR, it looks great and is ready to merge. And I will also do a new release after that. I'm only thinking a bit about some aspects of handling the buffers:

  • Is naming the main buffer "rgb" some kind of convention in Gym? If so, then it clearly suggests that this is RGB buffer and I think supporting other ViZDoom's build-in formats of buffers is not really needed, they were added for user convenience, not really as env property. I've pushed small changes that force RGB buffer when it's not set and I left support for grayscale format as (HIGHT, WIDTH, 1) array as it was before.
  • render() method displays only the screen buffer, I believe that this method more or less should present as much human-readable information as possible, so maybe it is a good idea to stack also other buffers (if they are available) as one image, to create output similar to our gif in README.md, I'm just not sure if this is in line with other Gym conventions.
    vizdoom_deadly_corridor

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 29, 2022

@mwydmuch Thanks for the checks, some embarrasing bugs there indeed ^^.

I agree supporting other modes than RGB is unnecessary, so I remove all GRAY8 code and force it to RGB24.

I also updated the render() to include all available buffers similar to the script (with less fancy code for labels, though).

@jkterry1 If you can give 👍 on the render change, I will merge this :)

@mwydmuch
Copy link
Member

Looks good @Miffyli, thank you for the changes!

@Miffyli
Copy link
Collaborator

Miffyli commented Mar 29, 2022

Holy snap, thanks @mwydmuch for double-checking my changes 😅 . Getting too used to nifty tools.

Copy link
Collaborator

@Miffyli Miffyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terry gave 👍 on this. Thanks @arjun-kg for your work! Merging this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants