[WIP] Unity Environment Registry #3967

vincentpierre · 2020-05-14T21:54:28Z

In This PR : Prototype of the Unity Environment Registry
Uploaded the 3DBall and Basic Environments for mac only

How to use on Python :

from mlagents_envs.registry import default_registry

print(default_registry["3DBall"].description)
env = default_registry["3DBall"].make()
env.reset()

for i in range(10):
   print(i)
   env.step()

env.close()

Types of change(s)

Checklist

Added tests that prove my fix is effective or that my feature works
Updated the changelog (if applicable)
Updated the documentation (if applicable)
Updated the migration guide (if applicable)

[JIRA ticket](https://jira.unity3d.com/browse/MLA-997) [Design Document](https://docs.google.com/document/d/1bFQ3_oXsA80FMou8kwqYxC53kqG5L3i0mbTQUH4shY4/edit#) In This PR : Prototype of the Unity Environment Registry Uploaded the 3DBall and Basic Environments for mac only How to use on Python : ```python from mlagents_envs.registry import UnityEnvRegistry registry = UnityEnvRegistry() print(registry["3DBall"].description) env = registry["3DBall"].make() env.reset() for i in range(10): print(i) env.step() env.close() ```

chriselion · 2020-05-14T22:45:22Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+    platform dependent : It will only return a Unity executable compatible with the
+    computer's OS. If no executable is found, None will be returned.
+    """
+    _, bin_dir = get_tmp_dir()


I think we should have the final location of these be in a user-overridable directory, not a temp dir (since ideally they won't be temp).

I think tmp dir is fine for now. This is the way ai2-thor does it for example.

ml-agents-envs/mlagents_envs/registry/unity_env_registry.py

chriselion · 2020-05-14T23:02:26Z

Here's a sketch of what I had in mind for the remote yaml files:

class UnityEnvRegistry(Mapping):
    def __init__(manifest_loc_or_url: str)
        self._manifest_loc_or_url = manifest_loc_or_url
        self._synced = False

    def _populate_from_manifest():
        if self._synced:
            return

        if is_remote_url(self._manifest_loc_or_url):
            manifest = download(self._manifest_loc_or_url)
        else:
            manifest = yaml_load(self._manifest_loc_or_url)

        for env in manifest["environments"]:
            self.register(RemoteRegistryEntry.from_manifest_entry(env))
        self._synced = True

    def clear():
        self._synced = False # Sync next time we do something
        # clear internal state

    def __getitem__(self, identifier: str) -> BaseRegistryEntry:
        self._populate_from_manifest()
        # do work
    
    def __len__(self) -> int:
        self._populate_from_manifest()
        # do work
    
    def __iter__(self) -> Iterator[Any]:
        self._populate_from_manifest()
        # do work

- UnityEnvRegistry is no longer static and needs to be instantiated - Providing a default_registry that will contains our environments - Added a functionality to register RemoteRegistryEntry with a yaml file

vincentpierre · 2020-05-15T01:18:14Z

Made some updates :

UnityEnvRegistry is no longer static and needs to be instantiated
Providing a default_registry instance that will contains our environments
Added a functionality to register RemoteRegistryEntry with a yaml file

yaml file looks like :

environments:
  - 3DBall:
     expected_reward: 100
     description: | ## 3DBall: 3D Balance Ball
      ...
     linux_url: null
     darwin_url: https://storage.googleapis.com/mlagents-test-environments/1.0.0/darwin/3DBall.zip
     win_url: null

  - Basic:
     expected_reward: 0.93
     description: | ## Basic
       ...
     linux_url: null
     darwin_url: https://storage.googleapis.com/mlagents-test-environments/1.0.0/darwin/Basic.zip
     win_url: null

vincentpierre · 2020-05-15T01:21:23Z

ml-agents-envs/setup.py

@@ -51,6 +51,7 @@ def run(self):
        "numpy>=1.14.1,<2.0",
        "Pillow>=4.2.1",
        "protobuf>=3.6",
+        "pyyaml",


Not too happy about this...

why not specify a lower bound for pyyaml version ?

I copied this from the mlagents setup.py file

copying from mlagents setup.py doesn't make it correct. I suggest having a lower bound on the pyyaml version in both places.

…he url in its name to make sure the right environment is opened and not just the one present. Useful if environments are renamed

anupam-142857 · 2020-05-15T18:20:35Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+    print("")
+
+    # Clean up zip
+    print_progress(f"  Cleaning up {name}", 0)


please use a logger instead of print statements in lines 117,118,123,126

I replaced one of the calls but I want a progress bar and logger can't do that

anupam-142857 · 2020-05-15T18:21:52Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+    Converts a local yaml file into a Python dictionary
+    """
+    with open(path) as data_file:
+        return yaml.safe_load(data_file)


might want to handle an exception here in case yaml.safe_load throws an error.

What is wrong with raising that error? I don't want to suppress it.

Firstly there is nothing wrong in what you did. Secondly, raising an exception allows you to give a more informative message to the user instead of a generic error message that comes from python. It's more of a matter of coding style rather than correctness.

It was just a suggestion to begin with :)

ml-agents-envs/mlagents_envs/registry/remote_registry_entry.py

anupam-142857 · 2020-05-15T18:28:59Z

ml-agents-envs/setup.py

@@ -51,6 +51,7 @@ def run(self):
        "numpy>=1.14.1,<2.0",
        "Pillow>=4.2.1",
        "protobuf>=3.6",
+        "pyyaml",


why not specify a lower bound for pyyaml version ?

ml-agents-envs/mlagents_envs/registry/unity_env_registry.py

chriselion · 2020-05-18T20:49:12Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+
+    # Download zip
+    try:
+        request = urllib.request.urlopen(url)


Please pass a timeout here, otherwise it could hang indefinitely. I'd say 30 seconds as a default, with optional parameters to override?

chriselion · 2020-05-18T20:52:09Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+        logger.debug(
+            f"Local environment {name} not found, downloading environment from {url}"
+        )
+        download_and_extract_zip(url, name)


Connection errors will raise from here and aren't caught. Since these might be transient errors and not the users fault, we should catch them and return null (or even better, retry a few times).

chriselion · 2020-05-18T20:52:54Z

ml-agents-envs/mlagents_envs/registry/binary_utils.py

+    with open(zip_file_path, "wb") as zip_file:
+        downloaded = 0
+        while True:
+            buffer = request.read(BLOCK_SIZE)


In the same vein, pretty sure this could fail sporadically; needs some better error handling.

docs/Unity-Environment-Registry.md

Co-authored-by: andrewcoh <[email protected]>

docs/Unity-Environment-Registry.md

andrewcoh · 2020-05-18T21:20:00Z

docs/Unity-Environment-Registry.md

+      - ...
+```
+
+Your users can now use your environment with the following code :


Should it be made clear that they are not pushing these builds to our storage but they must host it on their own? Is there another step to use the new env i.e. pip install foo-env in gym or is this beyond the scope of this WIP?

There are no additional step. No need to pip install anything, just get the url of the yaml

So if you have a new env do you submit a PR to our repo to have it added to the registry? Or do you need to go to a fork?

Ok, so it seems the documentation is not clear. Please advise on how to make more explicit.
If you made a brand new environment, you need to :

Compile it into an executable

Zip the executable

Upload the executable to YOUR OWN hosting service (example : gcs)

Create a yaml file that contains the url to the zip file

Upload the yaml file to hosting service

Once this is done, to share your environment, all you need to do is advertise the URL to the yaml. If a user wants your environment, all they have to do is call

env_registry = UnityEnvRegistry() env_registry.register_from_yaml("your-url-here")

And they will have your environment.
No PR, no fork, no pip

@andrew is this confusing? Should I change the documentation to give more details ?

I think that makes sense. I was being dense. Also, thats not me^

ml-agents-envs/mlagents_envs/registry/base_registry_entry.py

chriselion · 2020-05-20T17:29:04Z

I can come in another iteration, but I think we should have some notion of versioning of a given environment (separate from the communicator version) to indicate changes/iterations on the scene itself.

yijiezh · 2020-07-14T05:42:51Z

I tried

from mlagents_envs.registry import default_registry

print(default_registry["Basic"].description)
env = default_registry["Basic"].make()

print(env.reset())

I got None in the stdout.
Didn't the env here same as the a gym env, and that should return an observation vector? Anything I missed?

chriselion · 2020-07-14T15:40:00Z

@yijiezh BaseRegistryEntry.make() returns a BaseEnv, and BaseEnv.reset() returns None, so that's expected:

ml-agents/ml-agents-envs/mlagents_envs/base_env.py

Lines 375 to 379 in c75eacb

    
               @abstractmethod 
        
               def reset(self) -> None: 
        
                   """ 
        
                   Signals the environment that it must reset the simulation. 
        
                   """

Please create a new issue (or forum thread) if you have more problems.

vincentpierre self-assigned this May 14, 2020

vincentpierre requested review from andrewcoh, chriselion, mmattar and anupam-142857 and removed request for mmattar May 14, 2020 21:54

chriselion reviewed May 14, 2020

View reviewed changes

ml-agents-envs/mlagents_envs/registry/unity_env_registry.py Outdated Show resolved Hide resolved

Other approach:

8e8ec16

- UnityEnvRegistry is no longer static and needs to be instantiated - Providing a default_registry that will contains our environments - Added a functionality to register RemoteRegistryEntry with a yaml file

vincentpierre commented May 15, 2020

View reviewed changes

Some extra verification of the url : The binary will have a hash of t…

4c2750b

…he url in its name to make sure the right environment is opened and not just the one present. Useful if environments are renamed

anupam-142857 reviewed May 15, 2020

View reviewed changes

vincentpierre added 8 commits May 15, 2020 12:18

Using logger rather than print for debug statement

d62c3ef

More strict version of pyyaml

e87313f

Added the additional argument option in the remote_registry_entry

974627d

preparing the registry for windows

2b4ba36

Documentation and changelog

eeb6ceb

added a simple test

3325839

fixing test

8426b04

no graphics in the test

fb7dfae

chriselion reviewed May 18, 2020

View reviewed changes

andrewcoh reviewed May 18, 2020

View reviewed changes

docs/Unity-Environment-Registry.md Outdated Show resolved Hide resolved

[skip ci] Update docs/Unity-Environment-Registry.md

1012ee2

Co-authored-by: andrewcoh <[email protected]>

andrewcoh reviewed May 18, 2020

View reviewed changes

docs/Unity-Environment-Registry.md Outdated Show resolved Hide resolved

andrewcoh reviewed May 18, 2020

View reviewed changes

ml-agents-envs/mlagents_envs/registry/base_registry_entry.py Show resolved Hide resolved

vincentpierre added 3 commits May 18, 2020 14:41

addressing comments

c508185

changing the base port

eb8fdc2

[skip ci] Updating doc

5fed901

vincentpierre marked this pull request as ready for review May 18, 2020 23:39

awjuliani mentioned this pull request May 19, 2020

Feature/google collab example #3990

Closed

10 tasks

Changing the test to not use the default_registry environments

c8e3018

chriselion approved these changes May 20, 2020

View reviewed changes

vincentpierre merged commit 742c2fb into master May 20, 2020

delete-merged-branch bot deleted the develop-env-registry branch May 20, 2020 20:30

Unity-Technologies locked as resolved and limited conversation to collaborators Jul 14, 2020

[WIP] Unity Environment Registry #3967

[WIP] Unity Environment Registry #3967

Uh oh!

Conversation

vincentpierre commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of change(s)

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chriselion commented May 14, 2020

Uh oh!

vincentpierre commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anupam-142857 May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chriselion commented May 20, 2020

Uh oh!

yijiezh commented Jul 14, 2020

Uh oh!

chriselion commented Jul 14, 2020

Uh oh!

Uh oh!

vincentpierre commented May 14, 2020 •

edited

Loading

anupam-142857 May 15, 2020 •

edited

Loading