Fixed batch spaces where the original space's seed was ignored. Issue 2680 #2727

pseudo-rnd-thoughts · 2022-04-02T18:31:56Z

Provides a fix for issue 2680 where the SyncVectorEnv ignores the action space seed

This required changes to the batch_space function for each of the spaces to pass the seed for the batched space
The space constructor seed parameter allows a seeding.RandomNumberGenerator that ignores the seed function if passed

…calars

…pr__ for the dictionary space string looks similar to an actual dictionary

…ify nvec typing to include np.ndarray

…g.RNG is provided then it is assigned to _np_random and .seed is not run

…ce's seeding

…seeding

…generator.state

…equivalent for testing

lebrice

Interesting stuff @pseudo-rnd-thoughts !

I left some comments here, hopefully they are useful to you.
I'm not a "real" contributor to Gym, so I'm not approving this PR or anything, just commenting.

Have a good one!

tests/vector/test_spaces.py

gym/spaces/dict.py

gym/spaces/box.py

gym/spaces/multi_discrete.py

gym/vector/utils/spaces.py

tests/vector/test_sync_vector_env.py

tests/vector/utils.py

…s not a complete copy of the original space

… value

…e original space doesn't sampling doesn't effect the batched_space

…t_each_index and test_deterministic

pseudo-rnd-thoughts · 2022-04-06T16:17:25Z

@lebrice I think I have made all of the changes that you requested, could you review again please?

jkterry1 · 2022-04-08T14:37:16Z

@pseudo-rnd-thoughts can you please fix tests?

jkterry1 · 2022-04-08T14:37:22Z

merge conflicts*

pseudo-rnd-thoughts · 2022-04-08T22:34:20Z

@jkterry1 I have fixed the merge conflicts and passed all of the tests.

However, running the tests locally (on mac) then I'm getting an error with tests/vector/test_shared_memory::test_read_from_shared_memory for all the spaces.
But the problem is that Im getting the problem on the master branch so I dont think that I have introduced an issue. Do you want me to solve this issue in this PR or a separate one?

Edit - The solution was much simpler than I thought and is solved in the commit below

…rocess_write function was unpicklable. Making the function a top-level function solves this error

lebrice

Thanks for the changes @pseudo-rnd-thoughts !

gym/spaces/box.py

gym/spaces/dict.py

gym/spaces/tuple.py

gym/utils/seeding.py

gym/vector/utils/spaces.py

tests/vector/test_spaces.py

tests/vector/test_sync_vector_env.py

….seed's typing

… the number of batched spaces n=1

RedTachyon

Added a bunch of comments. Overall this breaks my brain a little bit, but I think the angle of the solution is fine, so I left some nitpicks for the implementation.

And for the ease of clicking for future me: #2680

gym/spaces/dict.py

gym/utils/seeding.py

gym/vector/utils/spaces.py

tests/vector/test_spaces.py

tests/vector/test_sync_vector_env.py

tests/vector/utils.py

RedTachyon · 2022-04-10T17:51:22Z

Ah, and please name the PR in some more descriptive way.

lebrice · 2022-04-10T18:57:29Z

Hey @pseudo-rnd-thoughts just a small remark:
I'd rather mark the conversations as resolved myself when reviewing. This makes it easier to see if my comments are addressed.

Also, and this is perhaps a bit "extra", but I personally like to leave a comment with the hash of the commit that addresses the feedback, something like "fixed in abcdef123", and GitHub will automatically create a link to view the diff for that commit.

This makes the reviewing experience much better. Also, could you please un-resolve the discussions where I left a comment? I don't believe I can do it myself atm.

pseudo-rnd-thoughts · 2022-04-11T09:13:13Z

Thanks, this is my first time working on a project of this size so thanks for the recommendations, I will try to do all of them.

…` with `from numpy.testing import assert_array_equal`

….state` with `np_random`

pseudo-rnd-thoughts · 2022-04-14T19:02:24Z

@lebrice I think that you have made all of the changes necessary.
Would you be able to resolve all of the questions that you think have been solved?

tests/vector/test_spaces.py

tests/vector/test_sync_vector_env.py

tests/vector/utils.py

pseudo-rnd-thoughts · 2022-04-19T20:20:02Z

@lebrice Sorry I messed up, I have responded to all of your messages but had them as a review and pending and thought they had gone through but they hadn't. Just did that now, I hope my "old" answers are helpful

…46bc35ff3fad9f65979c693472a93a8#r850740825

RedTachyon · 2022-04-24T14:09:26Z

Looks good now

pseudo-rnd-thoughts added 14 commits April 2, 2022 17:01

Add a case for the Box shape where the low and high values are both s…

cbfb39d

…calars

Add seeding.RandomNumberGenerator parameter to Dict seed. Modify __re…

fd14bfc

…pr__ for the dictionary space string looks similar to an actual dictionary

Add seeding.RandomNumberGenerator parameter to Multi Binary seed

3afe353

Add seeding.RandomNumberGenerator parameter to Multi Binary seed. Mod…

c4163ce

…ify nvec typing to include np.ndarray

Space seed typing can be a seeding.RandomNumberGenerator. If a seedin…

6c4ccbb

…g.RNG is provided then it is assigned to _np_random and .seed is not run

Fixed the tuple seeding type as List[int] is not a valid Space seed type

7815e0c

Added typing to batch_space. The batch_space seed is equal to the spa…

240893f

…ce's seeding

Fixed the seeding type

c65fda1

Add test for batch space seeds are identical to the original space's …

dfdd7ff

…seeding

Add equivalence function for RandomNumberGenerator comparing the bit_…

118a281

…generator.state

The batch_space functions uses a copy of the seed for the original space

8f5a933

Set the action space seed for sync_vector_env seed testing

c68c9d1

Add test for the seeding of the sync vector environment

7e17b3f

Update the test_batch_space_seed to check the resulting sampling are …

462101d

…equivalent for testing

pseudo-rnd-thoughts mentioned this pull request Apr 2, 2022

[Bug Report] VectorEnv action space seed problem #2680

Closed

1 task

lebrice reviewed Apr 3, 2022

View reviewed changes

pseudo-rnd-thoughts added 10 commits April 3, 2022 15:33

Revert representation back to the original version

0568a8e

Remove additional Box shape initialisation

becf062

Remove additional typing of MultiDiscrete

33f116b

Fixed bug of Space batch space where the original space's np_random i…

e3ae3c8

…s not a complete copy of the original space

Add CustomSpace to the batched space seed test

a3a2ff5

Modify the CustomSpace sample to produce a random number not a static…

a5b6564

… value

Fix CustomSpace to reflect the sample function

5e47aff

Copy the space.np_random for the batched_space seed to ensure that th…

75bf30f

…e original space doesn't sampling doesn't effect the batched_space

Parameterized the batch_space_seed, added testing for rng_different_a…

2b94d97

…t_each_index and test_deterministic

Black and isort pre-commit changes

eec4b2d

Merge branch 'master' into issue-2680

852f016

pseudo-rnd-thoughts added 2 commits April 8, 2022 22:52

Merge branch 'master' into issue-2680

4b76cfa

Pre-commit fix

6fefc6b

pseudo-rnd-thoughts mentioned this pull request Apr 8, 2022

AutoResetWrapper integration with gym.make() #2728

Merged

7 tasks

MacOS, test_read_from_shared_memory throws an error that the inner _p…

1dc2789

…rocess_write function was unpicklable. Making the function a top-level function solves this error

lebrice reviewed Apr 9, 2022

View reviewed changes

pseudo-rnd-thoughts added 2 commits April 9, 2022 19:43

Fixed typing of seed where a space's seed function differs from Space…

05a4539

….seed's typing

Added check that the sample lengths are equal and explicitly provided…

884f0dc

… the number of batched spaces n=1

RedTachyon reviewed Apr 10, 2022

View reviewed changes

pseudo-rnd-thoughts changed the title ~~Issue 2680~~ Fixed batch spaces where the original space's seed was ignored. Issue 2680 Apr 11, 2022

pseudo-rnd-thoughts added 4 commits April 11, 2022 19:57

Removed relative imports for absolute imports

4f22472

Use deepcopy instead of copy

ebcdd9d

Replaces `from numpy.testing._private.utils import assert_array_equal…

c8e3100

…` with `from numpy.testing import assert_array_equal`

Using the seeding __eq__ function, replace `np_random.bit_generator…

cac98f4

….state` with `np_random`

lebrice reviewed Apr 14, 2022

View reviewed changes

tests/vector/test_spaces.py Outdated Show resolved Hide resolved

tests/vector/test_sync_vector_env.py Show resolved Hide resolved

tests/vector/utils.py Show resolved Hide resolved

Added docstrings and comments to the tests to explain their purpose

296cf50

pseudo-rnd-thoughts added 6 commits April 19, 2022 22:20

Merge branch 'master' into issue-2680

0cd88cd

Remove __eq__ from RandomNumberGenerator and add to tests/vector/utils

41a5683

Add sync vector determinism test for issue openai#2680

b7fc60f

Fixed bug for https://github.com/openai/gym/pull/2727/files/462101d38…

4fdd037

…46bc35ff3fad9f65979c693472a93a8#r850740825

Made the new seeds a list of integers

9ed9149

Merge branch 'master' into issue-2680

18f99c6

jkterry1 merged commit 3354451 into openai:master Apr 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed batch spaces where the original space's seed was ignored. Issue 2680 #2727

Fixed batch spaces where the original space's seed was ignored. Issue 2680 #2727

pseudo-rnd-thoughts commented Apr 2, 2022

lebrice left a comment

pseudo-rnd-thoughts commented Apr 6, 2022

jkterry1 commented Apr 8, 2022

jkterry1 commented Apr 8, 2022

pseudo-rnd-thoughts commented Apr 8, 2022 •

edited

Loading

lebrice left a comment

RedTachyon left a comment

RedTachyon commented Apr 10, 2022

lebrice commented Apr 10, 2022 •

edited

Loading

pseudo-rnd-thoughts commented Apr 11, 2022

pseudo-rnd-thoughts commented Apr 14, 2022

pseudo-rnd-thoughts commented Apr 19, 2022

RedTachyon commented Apr 24, 2022

Fixed batch spaces where the original space's seed was ignored. Issue 2680 #2727

Fixed batch spaces where the original space's seed was ignored. Issue 2680 #2727

Conversation

pseudo-rnd-thoughts commented Apr 2, 2022

lebrice left a comment

Choose a reason for hiding this comment

pseudo-rnd-thoughts commented Apr 6, 2022

jkterry1 commented Apr 8, 2022

jkterry1 commented Apr 8, 2022

pseudo-rnd-thoughts commented Apr 8, 2022 • edited Loading

lebrice left a comment

Choose a reason for hiding this comment

RedTachyon left a comment

Choose a reason for hiding this comment

RedTachyon commented Apr 10, 2022

lebrice commented Apr 10, 2022 • edited Loading

pseudo-rnd-thoughts commented Apr 11, 2022

pseudo-rnd-thoughts commented Apr 14, 2022

pseudo-rnd-thoughts commented Apr 19, 2022

RedTachyon commented Apr 24, 2022

pseudo-rnd-thoughts commented Apr 8, 2022 •

edited

Loading

lebrice commented Apr 10, 2022 •

edited

Loading