-
Notifications
You must be signed in to change notification settings - Fork 285
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
[BugFix] Fix RoboHiveEnv tests #2062
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/2062
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 3 Unrelated FailuresAs of commit 772e893 with merge base 0d00748 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
LGTM |
The tests are still failing because of missing envs, e.g. DKittyOrientFixed-v0
|
I checked things at the RoboHive's end. All things line up well. Do you know where the problem is? (robohive) vikashplus@vikashplus-m3:~/Repos/robohive$ ipython
In [1]: import robohive
RoboHive:> Registering Arms Envs
RoboHive:> Registering Myo Envs
RoboHive:> Registering Hand Envs
RoboHive:> Registering Claw Envs
RoboHive:> Registering Appliances Envs
RoboHive:> Registering Multi-Task (2 subtasks) Envs
RoboHive:> Registering FrankaKitchen (FK1) Envs
RoboHive:> Registering Multi-Task (9 subtasks) Envs
RoboHive:> Registering Quadruped Envs
In [3]: import gymnasium as gym
In [4]: ee = gym.make("DKittyWalkFixed-v0") Here is another one (robohive) vikashplus@vikashplus-m3:~/Repos/robohive/robohive$ python tests/test_quads.py
RoboHive:> Registering Arms Envs
RoboHive:> Registering Myo Envs
RoboHive:> Registering Hand Envs
RoboHive:> Registering Claw Envs
RoboHive:> Registering Appliances Envs
RoboHive:> Registering Multi-Task (2 subtasks) Envs
RoboHive:> Registering FrankaKitchen (FK1) Envs
RoboHive:> Registering Multi-Task (9 subtasks) Envs
RoboHive:> Registering Quadruped Envs
=================================
Testing module:: Quadruped Suite
Testing env: DKittyOrientFixed-v0
ROBEL: RObotics BEnchmarks for Learning with low-cost robots
Michael Ahn, Henry Zhu, Kristian Hartikainen, Hugo Ponte
Abhishek Gupta, Sergey Levine, Vikash Kumar
CoRL-2019 | https://sites.google.com/view/roboticsbenchmarks/
Testing env: DKittyOrientRandom-v0
Testing env: DKittyOrientRandom_v2d-v0
WARNING:absl:ARB_clip_control unavailable while mjDEPTH_ZEROFAR requested, depth accuracy will be limited
Testing env: DKittyStandFixed-v0
Testing env: DKittyStandRandom-v0
Testing env: DKittyStandRandom_v2d-v0
Testing env: DKittyWalkFixed-v0
Testing env: DKittyWalkRandom-v0
Testing env: DKittyWalkRandom_v2d-v0
.
----------------------------------------------------------------------
Ran 1 test in 8.394s
OK |
Found a fix for the issue! The camera names for the
The colon in the camera name is causing some issue, I'm not sure what that is. But adding |
torchrl/envs/libs/robohive.py
Outdated
@@ -233,6 +233,7 @@ def register_visual_env(cls, env_name, cams): | |||
|
|||
if not len(cams): | |||
raise RuntimeError("Cannot create a visual envs without cameras.") | |||
cams = [i.replace("A:", "A_") for i in cams] |
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.
Let me know if this is something you think should be changed at RoboHive's end.
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.
Ah, I see your comment above. I believe A:trackingZ
is a valid string for MuJoCo as well as RoboHive.
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 would encourage using environment (and derived camera etc) names that are conventional and can be used for serialization. I can imagine someone using the camera name within the experiment name and use the experiment name to save things on disk
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 have name a note of this for the next release.
Should we merge this PR for now and remove the workaround after the camera names are updated in robohive in the next release? That seems like a reasonable approach. |
I'd vote that we go ahead with the proposed workaround. I'll make sure that we land this change in the next RoboHive release. |
test/test_libs.py
Outdated
@@ -3338,37 +3338,28 @@ class TestRoboHive: | |||
# In the CI, robohive should not coexist with other libs so that's fine. | |||
# Locally these imports can be annoying, especially given the amount of |
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.
RoboHive's prints can be contorled using an env variable
ROBOHIVE_VERBOSITY=ALL/INFO/(WARN)/ERROR/ONCE/ALWAYS/SILENT
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.
sure
The goal of this comment is for anyone running the tests and seeing loads of messages from imports. That could be a bit surprising and annoying. We can add that there's a strategy to silence them. I guess one could argue that importing a library should not print anything by default though...
I isolated the "info" in the corresponding entry in the output tensordict |
Co-authored-by: Vincent Moens <[email protected]> (cherry picked from commit bedd2b7)
I pulled the latest changes, upgraded robohive to 0.7, and replaced gym with gymnasium. The older failures are fixed, but a new set of tests are now failing.
Full traceback:
|
RoboHive is returning 2 values. See here - |
Nope it seems that in this case robohive is returning only one (at least torchrl cannot see two things coming out of reset) |
this line kinda does the same thing directly in the base class that uses
|
Here's my understanding of what is happening: the reset you pointed in env_base does indeed return 2 values but as you were asking just above the new method doesn't and @implement_for doesn't catch that class EnvBase:
@implement_for("gymnasium")
def _process_reset_data(self, reset_data):
if not isinstance(reset_data, tuple) or not (len(reset_data) == 2 and isinstance(reset_data[1], dict)):
return reset_data, {}
return reset_data
@implement_for("gym", None, 0.25) # or whatever other version
def _process_reset_data(self, reset_data):
if isinstance(reset_data, tuple) and len(reset_data) == 2 and isinstance(reset_data[1], dict):
# throw the dict away
return reset_data[0]
return reset_data
def reset(self):
# your code here
return self._process_reset_data(reset_data)
class OtherEnv(EnvBase):
def reset(self):
# your other code here
return self._process_reset_data(reset_data) Does that help? |
Thanks @vmoens for digging in. This indeed confirms the theory. I was also surprised by why it's not affecting other envs (e.g. 1. And there is a clever answer hidden there. Instead of returning the values, these other envs rely on the |
|
Perfect. Thanks for pairing with me to better understand this issue. |
Description
RoboHiveEnv
tests actually run correctly when executedMotivation and Context
Currently, the tests for
RoboHiveEnv
aren't actually executing correctly. When the env is instantiated, thefrom_pixels
key isn't actually used, and none of the envs are tested withfrom_pixels=True
.After the test code is fixed, I get 9 failures for the RoboHive tests, all of which seem to be caused by an issue with
DKitty
environments.Short Test log:
Full traceback: