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

Includes roms within build for pypi and testing #513

Closed jjshoots closed 4 months ago

jjshoots commented 4 months ago

Changes:

  1. Move roms installation from AutoROM to here, installation is via ale-accept-license-install-roms.
  2. Some other minor QoL stuff.
JesseFarebro commented 4 months ago

Not a huge fan of downloading ROMs from a gist, if y'all are going to install ROMs can't you reuse the AutoROM infrastructure and make it optional with a similar accept-rom-license extra.

pseudo-rnd-thoughts commented 4 months ago

Not a huge fan of downloading ROMs from a gist, if y'all are going to install ROMs can't you reuse the AutoROM infrastructure and make it optional with a similar accept-rom-license extra.

The AutoROM infrastructure is just downloading from the gist and unpacking from my knowledge

On the accept-rom-license, to my knowledge, we never explicitly explain what rom license people are accepting. Could we have a ale-py (without the roms) and an ale-py with the roms (using a --accept-rom-license) requirement?

JesseFarebro commented 4 months ago

The AutoROM infrastructure is just downloading from the gist and unpacking from my knowledge

I thought it was downloading from a torrent as of the most recent iteration?

jjshoots commented 4 months ago

@JesseFarebro That was in the older version of AutoROM, but it was causing a lot of problems for people who were on e.g.: university networks.

I've modified it so it doesn't automatically download roms anymore. Now the user must do ale-accept-license-install-roms after pip install, re: the last commit before your comment.

JesseFarebro commented 4 months ago

Can we have the script run during build? It would be easier for the PyPI to include the ROMs directly rather than every user download but might not be possible with accept-rom-license

If you are going to package ROMs without any type of user intervention I'd prefer this than downloading them.

@JesseFarebro That was in the older version of AutoROM, but it was causing a lot of problems for people who were on e.g.: university networks.

I see!

pseudo-rnd-thoughts commented 4 months ago

@jjshoots It would be nicer to include the ROMs within the PyPI release as users wouldn't need to download anything else Like the stella message when the first environment is created, could we do something similar for loading roms?

jjshoots commented 4 months ago

@pseudo-rnd-thoughts That was the original approach (where users didn't need to do any external action).

Should I just revert to that?

We can move the license agreement one stage earlier to "We assume you will be supplying your own roms or have already accepted the license agreement prior to installing this library", instead of having an explicit pip3 install ale-py[accept-rom-license] which I still don't think is the right way to do this.

pseudo-rnd-thoughts commented 4 months ago

Sorry for chopping and changing so much.

Yes, I think in the end this is the right approach with just a warning to the user about this. Probably with a link to the website

jjshoots commented 4 months ago

@pseudo-rnd-thoughts No worries! I'll make the changes and add in a warning with a small sleep.

jjshoots commented 4 months ago

@pseudo-rnd-thoughts yes, can confirm that is the approach. To me, that feels the same as including the ROMs within the PyPI package but with slightly less moving parts. Are you suggesting we have CI that downloads the ROMs and prepackages before publishing to PyPI instead? This approach may complicate things for dev since ROM download during dev must be handled externally then. This will also end up putting the ROMs directly within the tar.gz file on PyPI (not that anyone will look at that). Open to suggestions.

jjshoots commented 4 months ago

Discussed with @pseudo-rnd-thoughts on Discord, we agreed that packaging the roms as a tar.gz file within the PyPI package and unpacking after is the most ideal way to skirt around this grey area.