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

Support for downloading ROMs at install-time & ale-py plugin support #13

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Conversation

JesseFarebro
Copy link
Collaborator

@JesseFarebro JesseFarebro commented Sep 16, 2021

This allows for downloading ROMs at install-time with user intervention requiring them to accept the license.

Download at install-time

Old method using `--install-option`

Here's how it works: If you upload a source distribution of the package to PyPI this forces setup.py to get run upon installation. We can hook into the install command and check for a user-provided flag through --install-option which is natively supported by pip https://pip.pypa.io/en/stable/cli/pip_install/#install-install-option. Thus if the user provides --install-option="--accept-license" the ROMs will be downloaded at install-time.

The advantage of this is that you can provide --install-option in requirements.txt, pyproject.toml, or in extras. An example of this is:

requirements.txt

AutoROM~=0.4 --install-option='--accept-license'

Now for personal projects/research code, you can easily install your project with ROMs by default.

This also opens up the possibility for public projects to have extras_require with this option. For example,

pip install gym[atari] # Does not download ROMs, license wasn't accepted

pip install gym[atari,accept-rom-license] # ROMs auto download, license was accepted

This still requires intentionality on the user's behalf and the default package would NOT download ROMs.

ale-py plugin

If we're downloading ROMs at install-time there's no guarantee ale-py would be installed. This is where the plugin system comes into play. AutoROM by default will download ROMs to the new AutoROM.roms package where we have an entry-point to ale-py.roms that exposes the ROMs installed at AutoROM.roms.

This means that regardless of if ale-py was installed when AutoROM was installed ROM discovery will work properly once ale-py is installed. Cool!

Using extras_require

The above method was using --install-option but this has numerous disadvantages. For starters, you can only supply --install-option in requirements.txt, not in install_require, extras_require, etc.

This PR now has a new method which is implemented using extras_require. Essentially what happens is as follows:

AutoROM/
├─ AutoROM/
│  ├─ license/
│  │  ├─ setup.py
├─ setup.py

Inside of the root setup.py we specify the following extras_require:

    extras_require={
        "accept-rom-license": [
            f"AutoROM-licensed-roms @ file://localhost/{os.path.join(here, 'AutoROM', 'licensed')}"
        ]
    },

Now, when we install AutoROM from a source distribution without the extra [accept-rom-license] setup.py will be invoked as usual and nothing changes. But when you supply the extra [accept-rom-license] the subpackage license is built. When this package is built it downloads the ROMs to the appropriate directory during install.

Caveats

Relative Package Path

When building the source distribution it actually hardcodes the path in the egg metadata. There's ongoing work to support relative paths for sub-packages like this (see pypa/pip#6658, https://discuss.python.org/t/what-is-the-correct-interpretation-of-path-based-pep-508-uri-reference/2815, pypa/packaging#120).

Currently, I have a script scripts/build-sdist.sh which patches this in the egg metadata so the full path isn't leaked. Once these issues have been solved upstream we'll have a more elegant solution here.

Calling AutoROM from a subpackage

When pip is installing the subpackage license it doesn't yet have access to the main AutoROM package. Furthermore, pip creates a temporary directory for each package regardless of where it was originally located. This means we can't just modify sys.path to look for AutoROM in the appropriate directory. The solution here is to use MANIFEST.in with a symlink from license/AutoROM.py -> AutoROM.py. When building the source distribution the symlink is realized and the file is copied to license/AutoROM.py. I think this is the best solution we're going to get.

To make this work I removed checksums.txt and put it in AutoROM.py. This is fine for the time being, once multi-agent ALE is upstreamed we won't even need this checksum map.

Copy link
Contributor

@benblack769 benblack769 left a comment

Choose a reason for hiding this comment

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

Generally the code looks fine. Two big things are:

  1. There should not be duplicated code in the project.
  2. The usage description in the PR should be included in the README.md of the project so that people know how to use this feature in the future.

AutoROM/licensed/checksums.txt Outdated Show resolved Hide resolved
AutoROM/licensed/AutoROM.py Outdated Show resolved Hide resolved
@benblack769
Copy link
Contributor

I don't see any clear problems in the code, but can you put a note in the README.md on how to release a new source build version of AutoROM? I see you wrote a .sh file to do this, and it would be nice to know how to use it.

@JesseFarebro
Copy link
Collaborator Author

@benblack769 included some information on packaging and ./scripts/build-sdist.sh.

@benblack769 benblack769 merged commit ceb1411 into Farama-Foundation:master Sep 24, 2021
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.

2 participants