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

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

Merged
merged 11 commits into from
Apr 23, 2024

Conversation

pseudo-rnd-thoughts
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Mar 27, 2024

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

  • remove gym env
  • Add gymnasium env
  • Includes AutoROM roms within pypi (but not github)
  • Add python 3.12 support

@pseudo-rnd-thoughts
Copy link
Member Author

@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

Copy link
Collaborator

@JesseFarebro JesseFarebro left a comment

Choose a reason for hiding this comment

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

Overall this looks fine, my biggest complaint is around getting rid of the ROM plugins. Just assuming that the ROMs will be packaged and this will work for everyone's environment isn't going to cut it. The ALE is widely used at many companies where this isn't true so having a more customizable way to load ROMs is required IMO. I don't see any reason why you can't do the following:

  • Package the bin files during CD
  • Use the ROM plugin infrastructure to load these ROMs from ale_py.roms, in fact, it already did this, there should be no need to change anything here. All that needed to be done is to extract the ROMs into ale_py.roms during the wheel build.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
src/python/roms/__init__.py Outdated Show resolved Hide resolved
# 2. External ROMs
# 3. ROMs from atari-py.roms
# 4. ROMs from atari-py-roms.roms
_ROM_PLUGIN_REGISTRY: List[plugins.Plugin] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing ROM plugins is an issue for me. You have to remember that the ALE is used by many organizations and it's not trivial to just import ROMs in these corporations due to legal issues. For example, I built the plugin registry so at Google we can have an easy way to load ROMs as we can't just download them.

This is kind of a deal breaker, I would VERY much prefer if you kept all the ROM plugin infrastructure and just used this infra to load ROMs you'll package from ale_py.roms. Why did you feel the need to get rid of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for information, that is very helpful to know. We will look to see what we can do.
If the ROMs are packaged inside the PyPI install, does that cause issues for google?
The primarily issue I see is for users with additional external ROMs not included in AutoROMs standard set then I'm not sure how this would work with it.

src/python/roms/md5.json Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep this, see the discussion above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep this, it's useful locally or in cases where there's 3rd party packaging when ROMs wouldn't be included.

@pseudo-rnd-thoughts
Copy link
Member Author

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
Copy link
Member

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

@pseudo-rnd-thoughts
Copy link
Member Author

pseudo-rnd-thoughts commented Apr 15, 2024

@JesseFarebro Could you re-review?

  • changing to Ruff - We will investigate this but for simplicity, for this version, we will keep the updated pre-commit settings
  • md5.txt - I don't understand the point about history, looking at the blame, there seem to be three different PRs that have updated it, none of them look critical to keep. Therefore, it seems to me reasonable to keep the updated JSON format. If I've missed something please say
  • plugin system - With the ROMs being included within the project downloads (except git clone), to my understanding, the only feature missing from the updated method is interoperability with atari-py roms. What feature in the plugin system that the updated version doesn't include is important for organisations, i.e., google. Otherwise, the updated version is much simpler for everyone, therefore, I would wish to remove it.
  • Windows CI is broken - Reason is unknown

@JesseFarebro JesseFarebro self-requested a review April 16, 2024 03:25
Copy link
Collaborator

@JesseFarebro JesseFarebro left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Please consider my comment RE: ROM loading via an environment variable and if that's fixed I don't have any further comments.


md5.txt - I don't understand the point about history, looking at the blame, there seem to be three different PRs that have updated it, none of them look critical to keep. Therefore, it seems to me reasonable to keep the updated JSON format. If I've missed something please say

It's fine, I can go hunting in the history to find any changes to md5.txt.

plugin system - With the ROMs being included within the project downloads (except git clone), to my understanding, the only feature missing from the updated method is interoperability with atari-py roms. What feature in the plugin system that the updated version doesn't include is important for organisations, i.e., google. Otherwise, the updated version is much simpler for everyone, therefore, I would wish to remove it.

I took a look at the latest changes and this still wouldn't work for Google at least. At a minimum, we would need a way to specify a different or additional directory to search for ROMs. If you don't want to bring back the ROM plugins could you provide this functionality via an environment variable?

"""Expects name as a snake_case name, returns the full path of the .bin file if it's valid, otherwise returns None."""
# the theoretical location of the binary rom file
bin_file = f"{name}.bin"
bin_path = Path(__file__).parent / bin_file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe prefer to use importlib.resources which will provide better compatibility.

@jjshoots
Copy link
Member

@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
Copy link
Member Author

pseudo-rnd-thoughts commented Apr 20, 2024

@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
Copy link
Collaborator

JesseFarebro commented Apr 23, 2024

@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
Copy link
Member Author

@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
Copy link
Member

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

@Farama-Foundation Farama-Foundation deleted a comment from jjshoots Apr 23, 2024
@pseudo-rnd-thoughts
Copy link
Member Author

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)

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 9ac218a into master Apr 23, 2024
32 checks passed
@pseudo-rnd-thoughts pseudo-rnd-thoughts deleted the v0.9.0 branch April 23, 2024 23:22
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.

3 participants