Farama-Foundation / Arcade-Learning-Environment

The Arcade Learning Environment (ALE) -- a platform for AI research.
GNU General Public License v2.0
2.1k stars 416 forks source link

Pull Gym env in favor of Gymnasium and include ROMs with PyPI #516

Closed pseudo-rnd-thoughts closed 2 months ago

pseudo-rnd-thoughts commented 3 months ago

With Gymnasium v1.0 being released soon, one of the features removed is the namespace entry point used to register environments behind the scene, through what is a hack imho. In short, previous with ale-py (and shimmy) installed, the following code is possible

import gymnasium as gym

env = gym.make("ALE/Pong-v5")

That despite ale-py not being installed, then behind the scenes, Atari environments are still registered.

While an interesting approach, the more this has been used, the more of a headache it has been found to be. Therefore, it was decided to be removed in v1.0

This means that future users would be required to do

import gymnasium as gym
import ale_py

env = gym.make("ALE/Pong-v5")

explicitly, importing ale_py for the modules to be loaded

As a result, this PR makes the following changes

pseudo-rnd-thoughts commented 3 months ago

@JesseFarebro Hey, this is holding up Gymnasium's alpha 2, could you review this as soon as possible. Otherwise, I'll need to merge in the next week or so

pseudo-rnd-thoughts commented 3 months ago

Thanks for the review @JesseFarebro, I will talk to @jjshoots about the changes to the plugin system. It might be helpful to have a short discord call about this, if possible, to find to better route through. You can easily find me on the Gymnasium discord

The other suggested changes about Ruff, we can easily look at and could you look at the Windows CI issues linked above

jjshoots commented 3 months ago

Thanks for the review @JesseFarebro. I'm addressing the inclusion of bins on CD at #520.

pseudo-rnd-thoughts commented 3 months ago

@JesseFarebro Could you re-review?

jjshoots commented 3 months ago

@JesseFarebro Apologies for seemingly going in circles with this. What component of the plugin system is required at Google?

More specifically, the current release already includes the binaries as part of package data on PyPI. Running pip3 install ale-py would pull the binaries from the python index itself at build, it is no longer "lazy-downloaded" at runtime.

The only cases I can see where the plugin system is needed are:

  1. Google scrubs all .bin file from packages downloaded via pip.
  2. There are some atari games not within ale-py that Google is using, and requires external linking
  3. There are modifications to existing atari bins that are not native to ale-py.

Either 3 of those are valid use cases, but it'd be helpful to know which one in specific Google (or other companies) are using so that the plugin system's inclusion is justified.

Once again, sorry if this is going around in circles, we just want to make sure that the package doesn't have code that non of the maintainers know why should exist.

pseudo-rnd-thoughts commented 3 months ago

@JesseFarebro Any update? Otherwise, we will cut a release as is and can add changes later if necessary. Also, do you have any thoughts on the windows problem? It appears that zlib is not able to be built for some reason. I can replicate it on my local windows machine but don't understand vcpkg enough to fix it

JesseFarebro commented 2 months ago

@jjshoots Google doesn't use pip or any of the Python packaging infrastructure/ecosystem, only the source code exists in our version control system. This is why we need a way at runtime to modify the path where the ROMs can be discovered. As I said in my previous comment, at minimum we need an additional search path for ROMs. This can be through an environment variable or the plugin system, I don't really care which.

@pseudo-rnd-thoughts I'd probably try updating vcpkg to the latest version, they usually fix any major issues in a timely manner.

pseudo-rnd-thoughts commented 2 months ago

@JesseFarebro that makes sense, we will add an environment variable for overriding the rom path

@JesseFarebro i don't understand vcpkg enough, could you make a pr for it?

jjshoots commented 2 months ago

We resorted to using an environment var instead of a kwarg. It's in #522

pseudo-rnd-thoughts commented 2 months ago

Thanks @JesseFarebro for your review, I'm going to merge this PR and cut a release in the next few days (hoping that the wheels ci works)