openai / gym-http-api

API to access OpenAI Gym from other languages via HTTP
MIT License
292 stars 143 forks source link

Remove duplicate declaration of get_action_space_contains #36

Closed andrewschreiber closed 7 years ago

andrewschreiber commented 7 years ago

gym_http_server.pycontains two identical definitions of get_action_space_contains, one at line 73 and one at line 92.

def get_action_space_contains(self, instance_id, x):        
        env = self._lookup_env(instance_id)     
        return env.action_space.contains(int(x))

This PR removes the one at line 92.

catherio commented 7 years ago

Based on its position in the file next to get_observation_space_info, I'd guess it was actually meant to be implemented as get_observation_space_contains but that the author (cough, yours truly) copy-pasted the skeleton and never fixed it - would you be up for implementing get_observation_space_contains?

andrewschreiber commented 7 years ago

I've been there :). Looked into aget_observation_space_contains implementation. I can see two options for defining a match:

1) Specify a specific parameter and value:

@app.route('/v1/envs/<instance_id>/observation_space/contains/<parameter>/<value>', methods=['GET']) 

2) Pass JSON specifying a list of parameters and values:

    @app.route('/v1/envs/<instance_id>/observation_space/contains', methods=['GET'])
    { "parameter1" : "value1", "parameter2": "value2" }

Flask supports both options. Advantage of (1) is greater consistency with the existing API patterns. Advantage of (2) is the ability to check multiple criteria simultaneously.

I'm inclined towards implementing the latter as it's more robust. What do you think?

catherio commented 7 years ago

I don't have a strong feeling here - (2) seems fine!

andrewschreiber commented 7 years ago

@catherio Ready for a review pass on get_observation_space_contains method.

It turns out Flask doesn't support passing JSON as part of a GET request (as far as I could tell). The 'contains' method therefore needed to be a POST. Though the HTTP semantics are less than ideal, I believe passing JSON is still the stronger option due to support for collections.

PR also includes client-side Swift and Python implementation of the /observation_space/contains route.

andrewschreiber commented 7 years ago

Woot!