Add call, get_attr and set_attr methods to VectorEnv #1600

tristandeleu · 2019-07-12T22:24:03Z

As suggested in #1513, this PR adds 3 new methods to VectorEnv:

call(name, *args, **kwargs), which calls get_attr in each individual environment, and returns either its property name (in which case *args and **kwargs are ignored), or the result of name(*args, **kwargs).
get_attr(name), which is syntactic sugar for a call to call to get a property of the individual environments.
set_attr(name, values), which alls setattr on each individual environment.

namheegordonkim · 2020-02-27T21:45:28Z

Is this getting merged anytime soon?

tristandeleu · 2020-08-31T12:36:00Z

@pzhokhov I have rebased on master to remove any merge conflict. This PR is now ready to be merged or closed.

FirefoxMetzger

Nice work 🚀 I left some minor comments that you may choose to accept or reject depending on what you think the best solution is here.

Edit: FYI, I didn't review the test code; just the changes to the library code.

FirefoxMetzger · 2021-08-28T17:17:26Z

gym/vector/async_vector_env.py

+                        "Trying to call function `{0}` with "
+                        "`_call`. Use `{0}` directly instead.".format(name)


Suggested change

"Trying to call function `{0}` with "

"`_call`. Use `{0}` directly instead.".format(name)

f"Trying to call function `{name}` with "

f"`_call`. Use `{name}` directly instead."

Perhaps this can become more readable with format strings.

FirefoxMetzger · 2021-08-28T17:18:27Z

gym/vector/async_vector_env.py

+        if self._state != AsyncState.DEFAULT:
+            raise AlreadyPendingCallError(
+                "Calling `set_attr` while waiting "
+                "for a pending call to `{0}` to complete.".format(self._state.value),


Suggested change

"for a pending call to `{0}` to complete.".format(self._state.value),

"for a pending call to `{self._state.value}` to complete.",

FirefoxMetzger · 2021-08-28T17:19:18Z

gym/vector/async_vector_env.py

+        Parameters
+        ----------
+        name : string
+            Name of the method or property to call.


Do you think we should also add a line documenting args and kwargs here?

FirefoxMetzger · 2021-08-28T17:19:45Z

gym/vector/async_vector_env.py

+        if self._state != AsyncState.DEFAULT:
+            raise AlreadyPendingCallError(
+                "Calling `call_async` while waiting "
+                "for a pending call to `{0}` to complete.".format(self._state.value),


Suggested change

"for a pending call to `{0}` to complete.".format(self._state.value),

f"for a pending call to `{self._state.value}` to complete.",

Since the minimum python version in CI is 3.6 I assume that we can use f-strings? If so, they might make things a little more readable here

Indeed f-strings are much nicer! At the time CI was testing also for versions 2.7 and 3.5, that shows how old this PR is.

FirefoxMetzger · 2021-08-28T17:24:15Z

gym/vector/async_vector_env.py

+        ----------
+        timeout : int or float, optional
+            Number of seconds before the call to `step_wait` times out. If
+            `None`, the call to `step_wait` never times out.


Suggested change

`None`, the call to `step_wait` never times out.

`None` (default), the call to `step_wait` never times out.

FirefoxMetzger · 2021-08-28T17:51:14Z

gym/vector/async_vector_env.py

+        self._assert_is_running()
+        if not isinstance(values, list):
+            values = [values for _ in range(self.num_envs)]
+        assert len(values) == self.num_envs


Suggested change

assert len(values) == self.num_envs

if len(values) == self.num_envs:

raise ValueError("values must have one value for each environment or be a single (non-list) object.")

Potentially an opinionated comment: I'm not a fan of assert outside of testing code. I'd raise a specific exception here that will tell the user what went wrong (instead of implicitly doing so by showing the assertion that failed.

Also, it may be worthwhile to remove this check and simply fail in the zip loop below if necessary.

FirefoxMetzger · 2021-08-28T18:12:42Z

gym/vector/async_vector_env.py

+                        "Trying to call function `{0}` with "
+                        "`_call`. Use `{0}` directly instead.".format(name)


Suggested change

"Trying to call function `{0}` with "

"`_call`. Use `{0}` directly instead.".format(name)

f"Trying to call function `{name}` with "

f"`_call`. Use `{name}` directly instead."

FirefoxMetzger · 2021-08-28T18:18:46Z

gym/vector/async_vector_env.py

+                    "be one of {`reset`, `step`, `seed`, `close`, `_call`, "
+                    "`_setattr`, `_check_observation_space`}.".format(command)


I can't suggest a format string here, since the relevant line was only partially modified, but I'd probably use it here, too.

FirefoxMetzger · 2021-08-28T18:19:40Z

gym/vector/sync_vector_env.py

+        Parameters
+        ----------
+        name : string
+            Name of the method or property to call.


Similar to above, would it be worthwhile to document args and kwargs?

FirefoxMetzger · 2021-08-28T18:20:26Z

gym/vector/sync_vector_env.py

+        if not isinstance(values, list):
+            values = [values for _ in range(self.num_envs)]
+        assert len(values) == self.num_envs


Similar to above, are we worried about other iterables?

FirefoxMetzger

Some updates based on the changes you added :)

FirefoxMetzger · 2021-08-29T06:18:20Z

gym/vector/async_vector_env.py

+
+        args, kwargs :
+            Arguments and keyword arguments to apply to the method call.


Suggested change

args, kwargs :

Arguments and keyword arguments to apply to the method call.

args, kwargs :

Arguments and keyword arguments to apply to the method call.

Nitpick: remove extra whitespace.

I didn't know what was the change you suggested (where the extra whitespace was), but I changed the docstring to match more closely the Numpy docstring guidelines. I also moved the docstrings from SyncVectorEnv to the base class VectorEnv to match what was done for the other methods.

it's really just a small thing; there was a leading newline (\n) above args, kwargs that separated it from the previous parameter. If you look closely the suggestion changes 3 lines of code into 2 lines of code.

FirefoxMetzger · 2021-08-29T06:21:05Z

gym/vector/async_vector_env.py

@@ -362,20 +362,25 @@ def set_attr(self, name, values):
        name : string
            Name of the property to be set in each individual environment.

-        values : list or object
-            Values of the property to bet set to. If `values` is a list, then
+        values : list of object


Should this be or like before (suggested by the description and by the code), or of as changed to now? If the latter, I think the description should be updated to reflect that change. Should the source code raise an exception on non-list types, too?

No this was a mistake indeed, I thought it was a typo but it is list or object.

FirefoxMetzger · 2021-08-29T06:21:46Z

gym/vector/async_vector_env.py

-        assert len(values) == self.num_envs
+        if len(values) != self.num_envs:
+            raise ValueError(
+                "The values must be a list of length the number "


Suggested change

"The values must be a list of length the number "

"Values must be a list with length equal to the number "

FirefoxMetzger · 2021-08-29T06:23:01Z

gym/vector/sync_vector_env.py

@@ -125,14 +128,19 @@ def set_attr(self, name, values):
        name : string
            Name of the property to be set in each individual environment.

-        values : list or object
-            Values of the property to bet set to. If `values` is a list, then
+        values : list of object


Same question as previously; is the of here intentional?

tristandeleu · 2021-09-10T17:57:28Z

What is the status of this PR? Any timeline for this PR getting merged? @jkterry1

vwxyzjn · 2022-01-25T01:51:00Z

@jkterry1 this PR makes sense to me. In gym-microrts we use this feature to get invalid action masks.

Everything in the PR looks good to me except the testing environment might need to be replaced with CartPole-v1. CubeCrush-v0 has been deprecated in #2553.

jkterry1 · 2022-01-25T03:16:20Z

There's also merge conflicts. If you're happy with it I'll merge it once tests and merge conflicts are fixed.

vwxyzjn · 2022-01-29T17:32:46Z

LGTM

* Add call, get_attr and set_attr methods * Use f-strings and remove assert * Allow tuples in set_attr and move docstrings * Replace CubeCrash by CartPole in tests

tristandeleu force-pushed the feature/vector_env_call branch from bf17629 to afeff82 Compare August 30, 2020 11:55

tristandeleu closed this May 31, 2021

tristandeleu reopened this Jul 27, 2021

tristandeleu force-pushed the feature/vector_env_call branch from afeff82 to c309cd6 Compare July 27, 2021 21:09

cpnota mentioned this pull request Jul 30, 2021

Environment reset() should also return "info", like step() does. #1683

Closed

jkterry1 added the Needs Reviewer label Jul 30, 2021

tristandeleu mentioned this pull request Jul 31, 2021

Discussion of Vector Environment API Breaking Changes #2279

Open

tristandeleu force-pushed the feature/vector_env_call branch from c309cd6 to e7f19d3 Compare August 5, 2021 17:10

FirefoxMetzger reviewed Aug 28, 2021

View reviewed changes

FirefoxMetzger reviewed Aug 29, 2021

View reviewed changes

jkterry1 removed the Needs Reviewer label Sep 11, 2021

tristandeleu added 4 commits January 25, 2022 19:15

Add call, get_attr and set_attr methods

c638e93

Use f-strings and remove assert

b108424

Allow tuples in set_attr and move docstrings

420c2b5

Replace CubeCrash by CartPole in tests

2d90563

tristandeleu force-pushed the feature/vector_env_call branch from 27d04f6 to 2d90563 Compare January 26, 2022 00:30

jkterry1 merged commit 39ba73f into openai:master Jan 29, 2022

tristandeleu deleted the feature/vector_env_call branch January 29, 2022 23:34

vwxyzjn mentioned this pull request Mar 1, 2022

[Proposal] New info API for vectorized environments #2657

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add call, get_attr and set_attr methods to VectorEnv #1600

Add call, get_attr and set_attr methods to VectorEnv #1600

tristandeleu commented Jul 12, 2019

namheegordonkim commented Feb 27, 2020

tristandeleu commented Aug 31, 2020

FirefoxMetzger left a comment •

edited

Loading

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

tristandeleu Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger Aug 28, 2021

FirefoxMetzger left a comment

FirefoxMetzger Aug 29, 2021

tristandeleu Aug 29, 2021

FirefoxMetzger Aug 29, 2021

FirefoxMetzger Aug 29, 2021

tristandeleu Aug 29, 2021

FirefoxMetzger Aug 29, 2021

FirefoxMetzger Aug 29, 2021

tristandeleu commented Sep 10, 2021

vwxyzjn commented Jan 25, 2022

jkterry1 commented Jan 25, 2022

vwxyzjn commented Jan 29, 2022

		"Trying to call function `{0}` with "
		"`_call`. Use `{0}` directly instead.".format(name)

	"for a pending call to `{0}` to complete.".format(self._state.value),
	"for a pending call to `{self._state.value}` to complete.",

	"for a pending call to `{0}` to complete.".format(self._state.value),
	f"for a pending call to `{self._state.value}` to complete.",

	`None`, the call to `step_wait` never times out.
	`None` (default), the call to `step_wait` never times out.

	assert len(values) == self.num_envs
	if len(values) == self.num_envs:
	raise ValueError("values must have one value for each environment or be a single (non-list) object.")

		"be one of {`reset`, `step`, `seed`, `close`, `_call`, "
		"`_setattr`, `_check_observation_space`}.".format(command)


		args, kwargs :
		Arguments and keyword arguments to apply to the method call.

	"The values must be a list of length the number "
	"Values must be a list with length equal to the number "

Add call, get_attr and set_attr methods to VectorEnv #1600

Add call, get_attr and set_attr methods to VectorEnv #1600

Conversation

tristandeleu commented Jul 12, 2019

namheegordonkim commented Feb 27, 2020

tristandeleu commented Aug 31, 2020

FirefoxMetzger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FirefoxMetzger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristandeleu commented Sep 10, 2021

vwxyzjn commented Jan 25, 2022

jkterry1 commented Jan 25, 2022

vwxyzjn commented Jan 29, 2022

FirefoxMetzger left a comment •

edited

Loading