Unit test for HITL tool and/or SandboxDriver #1654
Conversation
facebook-github-bot
added
the
CLA Signed
| @patch("utils.gui.replay_gui_app_renderer.ReplayRenderer", spec=True) | ||
| @patch("utils.gui.replay_gui_app_renderer.ImageFramebufferDrawer", spec=True) | ||
| @patch("utils.gui.replay_gui_app_renderer.TextDrawer", spec=True) |
There was a problem hiding this comment.
Patching these objects until https://app.asana.com/0/home/1205684818278375/1205279290945037 is done.
| @@ -108,6 +108,11 @@ def __init__( | |||
| ) | |||
| self._count_obj_collisions = self._config.count_obj_collisions | |||
|
|
|||
| self._fixed_starting_position = ( | |||
There was a problem hiding this comment.
We need to get a review from someone more familiar with rearrange_task.py on the changes in this file.
| args=default_args, | ||
| config=default_config, | ||
| gui_input=gui_input, | ||
| line_render=app_renderer._replay_renderer.debug_line_render(0), |
There was a problem hiding this comment.
Would a solution like I did here perhaps be simpler and more elegant, if our goal is only to test SandboxDriver?
There was a problem hiding this comment.
Agree, thanks for the suggestion. Advantage of DummyClass over the patching is that this way testing SandboxDriver doesn't require importing and instantiating ReplayRenderer.
Updated code.
| from habitat_baselines.config.default import get_config as get_baselines_config | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
I'm curious if we can run this as part of CI. I started a slack thread here.
There was a problem hiding this comment.
As Andrew replied the Slack tread this assets are actually already being downloaded in the multi-agent trainer CI.
There was a problem hiding this comment.
Cool. Can you link to a successful CI run of this file here as a reply? It's not clear to me at a glance if this file is getting included in the tests as part of CI install_and_test_ubuntu.
| with open("examples/siro_sandbox/test_sandbox_app_keyframe.txt", "r") as f: | ||
| expected_keyframe = f.read() | ||
|
|
||
| assert post_sim_update_dict["keyframes"][0] == expected_keyframe |
There was a problem hiding this comment.
Can you provide instructions here (as a code comment) for how to update examples/siro_sandbox/test_sandbox_app_keyframe.txt if/when this test breaks due to desired changes to the app behavior?
| with open("examples/siro_sandbox/test_sandbox_app_keyframe.txt", "r") as f: | ||
| expected_keyframe = f.read() | ||
|
|
||
| assert post_sim_update_dict["keyframes"][0] == expected_keyframe |
There was a problem hiding this comment.
In addition to this equality check on the entire keyframe contents (which I like), can you also sanity-check the contents of the keyframe? There should be at least one keyframe, one load, one creation, and one stateUpdate, and the comment for that set of checks should be "the first keyframe should create and pose at least one render asset instance".
| from habitat_baselines.config.default import get_config as get_baselines_config | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Cool. Can you link to a successful CI run of this file here as a reply? It's not clear to me at a glance if this file is getting included in the tests as part of CI install_and_test_ubuntu.
| @@ -243,6 +248,10 @@ def reset(self, episode: Episode, fetch_observations: bool = True): | |||
| self._is_episode_active = True | |||
|
|
|||
| if self._should_place_articulated_agent: | |||
| if self._fixed_agent_starting_pose: | |||
There was a problem hiding this comment.
Are you happy with this implementation? I see two old comments that suggest this wasn't quite working correctly yet:
From your PR description:
slightly different agents positions even though we force them to be fixed by adding "fixed_agent_starting_pose" config argument (for the context if "fixed_agent_starting_pose" is in config we fix numpy and sim.pathfinder seeds). probably because float numbers representation? This causes test to fail sometimes.
From this Slack thread:
So, I would expect that setting env.seed(my_seed) would be enough to have deterministic behaviour, but it is not, agent positions are set to different positions every time. Is it working as expected @Andrew Szot @Alex Clegg?
Motivation and Context
Initial version of the SandboxDriver unit test.
Known issues:
How Has This Been Tested
Running
pytest examples/siro_sandbox/test_sandbox_app.pylocally (requires hab3_bench_assets assets).Types of changes
Checklist