ppaquette / gym-pull

Add-on for OpenAI Gym that supports automatic downloading of user environments.
MIT License
45 stars 19 forks source link

Suggestion: moving to Pip packaging #1

Closed gdb closed 8 years ago

gdb commented 8 years ago

What would you think about moving away from the top-level spec file, and instead changing to shelling out to pip install? You'd still be able to clone from Github if you wanted (pip install git+ssh://...). I think this wouldn't have to change the gym_pull user experience, and it would mean that gym_pull will work for free with any "officially" packaged pip package.

ppaquette commented 8 years ago

It's a move in the right direction.

I think a perfect implementation should: 1) Require no pull request 2) Require editing only one file 3) Be compatible with the (future version of the) scoreboard 4) Be indistinguishable from core envs (which are basically openai/ envs) 5) Install dependencies automatically 6) Be future proof (i.e. can't break with changes in gym) 7) Prevent features from not working (i.e. if you can pull it, you can make it, and see it on the scoreboard) 8) Allow quick customizations (i.e. changing a couple of lines from someone else's env) 9) Allow auto-loading of user envs when gym is imported

For 1), a PyPI-like workflow where you register a repo, and then issue refresh commands when the config file changes (i.e. new/deleted envs, or changed in params for register functions) could make sense. Both can support this

For 2), git+ssh requires editing setup.py + registry.register() + scoreboard.add_task(). My implementation only has a top-level spec file. The git+ssh requires 2 unrelated version numbers (setup.py and the env version), which are not fully compatible.

For 3) Both can support. Config files would be downloaded, parsed, and automatically registered with the scoreboard every time the user issues a refresh command.

For 4). I could make sense to split envs and core at some point (where only the openai/ envs would be installed by default). Both can support

For 5) Both can support. pip+ssh uses dependencies in setup.py, top-level spec uses pip dependencies in requirements property.

For 6) git+ssh requires making an explicit call to gym.register() and gym.scoreboard.registration.add_task(). This restricts changes to these functions. Top-level spec parses the file and makes the call afterwards. Much easier to change these functions if needed.

For 7), 8) and 9) Both can support.

gdb commented 8 years ago

2: some of this depends on how serious you want these environments to be. Our philosophy has been to create high-quality environments, which means accepting the environment author doing incrementally more work to make life easier for the environment users. In practice, the usual workflow is to update an environment with a non-breaking change, so usually you'll want to bump the version of your packaging but keep the environment version unchanged. In super rare circumstances you'll bump to -v1, -v2, etc..

I actually suspect that Pip packaged environments will be easier for new users to learn, since there's plenty of documentation on how to build a Pip package, and no magical file to find.

3: I'm not sure what we're going to do with respect to user-defined envs and the scoreboard. Some of this is about the goals of these environments, and how to make them best push forward research. I need to see how much serious demand there is to share results across user-defined envs.

6: breaking .register would be a major breaking change, and if we ever did it lots of other things would break too.

Anyway, ultimately the choice is up to you. Our choices for Gym core are generally:

I think the biggest argument for building this on top of Pip packaging is that the serious 3rd party environments will also need to build Pip packages, and so having two packaging standards will actually increase the friction.

ppaquette commented 8 years ago

I agree with your points.

What do you think of:

1) syntax: gym.pull('username/repo') would download and register all envs in the repo (so no 'env-name' and no version) 2) using git+https rather than git+ssh, to avoid issues with missing keys 3) force env-name to be '{username}/{env-name}-v{version}', so if a user registers 'env-name-v0', it would get deregistered and re-registered with their username 4) For the scoreboard, I would add spec and scoreboard parameters to the manifest env_info, to avoid pull requests for scoreboard registration.

And Either of: 4.1) Issue an error explaining that user envs are not supported on the scoreboard 4.2) Accept and store API requests from user envs, and issue a warning that the results won't be displayed on the scoreboard for now, and may never be displayed at all.

I'd create an gym_doom example, and others can just copy my implementation.

gdb commented 8 years ago
  1. I'd prefer not to monkey-patch gym if we can. Also, it might be nicer to copy how golang does this; gym_pull.pull("github.com/username/repo"). (Has the advantage of being pretty clear where it's coming from.)
  2. Yeah, git+https by default is probably fine. For golang, you can get it to do git+ssh by setting something like this:
[url "git@github.com:"]
      pushInsteadOf = "github:"
      insteadOf = https://github.com/

It'd be nice if that also works here so people can also use this with their private repos.

  1. I don't fully understand — you want to override registrations? That kind of defeats the point of the naming system right?
  2. I'm happy to have the scoreboard registration live in the package. For now, we'll manually include the ones we're interested in.
ppaquette commented 8 years ago
  1. I'll avoid monkey-patching. but I'm assuming if this gets popular. you'll merge in core at some point.
  2. For registration, I'll just throw an error if the envs are not registered using the proper format. It's going to get confusing if we have non-core envs without the username prefix.
gdb commented 8 years ago
  1. Cool. Yep, I'd be happy to have a package manager (with a simple implementation/interface) that people are excited about and works well for a wide-variety of use-cases.
  2. Got it. Yes, good to restrict the names.
ppaquette commented 8 years ago

Published a working version on PyPI (pip install gym-pull)

You can download my envs using:

gym_pull.pull('github.com/ppaquette/gym-doom')
gym_pull.pull('github.com/ppaquette/gym-super-mario')

Failing envs test:

gym_pull.pull('github.com/lusob/gym-ple')
gym_pull.pull('github.com/lusob/gym-tetris')
ppaquette commented 8 years ago

Did you have a chance to look at it?

Can I close this issue?

gdb commented 8 years ago

Yep, seems reasonable. Will be curious what users think!