-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pickle tests #53
Add pickle tests #53
Changes from 31 commits
7f90eb8
b30e99a
6a0e2f2
9e3dc4e
e8f9d87
ae08a50
1bc5a35
2e97a55
9f0760d
f0afeef
706db5f
dc9df9c
5917bd5
55e13a7
29a0e4f
86fc8bc
efac866
45de236
6be6af8
e6a1c88
ad564f8
320f498
4992a4c
084d9b2
ef0f81a
246c8e1
9413b02
60f7482
0240502
492a364
aa0bb38
746a823
ca39529
cce032f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
"""Tests the functionality of the BSuiteCompatibilityV0 on bsuite envs.""" | ||
import pickle | ||
import warnings | ||
|
||
import bsuite | ||
|
@@ -109,3 +110,58 @@ def test_seeding(env_id): | |
|
||
env_1.close() | ||
env_2.close() | ||
|
||
|
||
# Without EzPickle:_register_bsuite_envs.<locals>._make_bsuite_env cannot be pickled | ||
# With EzPickle: maximum recursion limit reached | ||
FAILING_PICKLE_ENVS = [ | ||
"bsuite/bandit_noise-v0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I have found this before. It is generally because one of the classes implemented I suspect the issue is that the environment only defines a variable on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ll go check that individual env and see. But what would we be able to do to fix it, besides submitting a PR to their repo? I guess we could in the compatibility wrapper specifically check if it’s that env or if an env has that specific variable not defined in init, and then do whatever modifications are required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In short, we can't fix it. The environments failing use a wrapper that contains def __getattr__(self, attr):
return getattr(self._env, attr) The problem exists when The solution is simple def __getattr__(self, attr):
if "_env" in self.__dict__:
return getattr(self._env, attr)
else:
return super().__getattribute__(attr) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you put a PR up by any chance? Even if they don't' end up merging it I feel like we might as well try, you seem to understand this stuff better than me though, I'm not sure I'd be able to explain it well or respond to any questions about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"bsuite/bandit_scale-v0", | ||
"bsuite/cartpole-v0", | ||
"bsuite/cartpole_noise-v0", | ||
"bsuite/cartpole_scale-v0", | ||
"bsuite/cartpole_swingup-v0", | ||
"bsuite/catch_noise-v0", | ||
"bsuite/catch_scale-v0", | ||
"bsuite/mnist_noise-v0", | ||
"bsuite/mnist_scale-v0", | ||
"bsuite/mountain_car_noise-v0", | ||
"bsuite/mountain_car_scale-v0", | ||
] | ||
|
||
PASSING_PICKLE_ENVS = [ | ||
"bsuite/mnist-v0", | ||
"bsuite/umbrella_length-v0", | ||
"bsuite/discounting_chain-v0", | ||
"bsuite/deep_sea-v0", | ||
"bsuite/umbrella_distract-v0", | ||
"bsuite/catch-v0", | ||
"bsuite/memory_len-v0", | ||
"bsuite/mountain_car-v0", | ||
"bsuite/memory_size-v0", | ||
"bsuite/deep_sea_stochastic-v0", | ||
"bsuite/bandit-v0", | ||
] | ||
|
||
|
||
@pytest.mark.parametrize("env_id", PASSING_PICKLE_ENVS) | ||
def test_pickle(env_id): | ||
"""Test that pickling works.""" | ||
env_1 = gym.make(env_id, **BSUITE_ENV_SETTINGS[env_id]) | ||
env_2 = pickle.loads(pickle.dumps(env_1)) | ||
|
||
obs_1, info_1 = env_1.reset(seed=42) | ||
obs_2, info_2 = env_2.reset(seed=42) | ||
assert data_equivalence(obs_1, obs_2) | ||
assert data_equivalence(info_1, info_2) | ||
for _ in range(100): | ||
actions = int(env_1.action_space.sample()) | ||
obs_1, reward_1, term_1, trunc_1, info_1 = env_1.step(actions) | ||
obs_2, reward_2, term_2, trunc_2, info_2 = env_2.step(actions) | ||
assert data_equivalence(obs_1, obs_2) | ||
assert reward_1 == reward_2 | ||
assert term_1 == term_2 and trunc_1 == trunc_2 | ||
assert data_equivalence(info_1, info_2) | ||
|
||
env_1.close() | ||
env_2.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had solved the seeding issue in dm-lab @jjshoots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure but I think this should be a separate PR with the other dm-lab fixes, this is just for adding pickle tests and turns out the pickle tests for dm lab don't seem to work and are blocked so can just update the seed/pickle stuff for dm-lab later