Removing Habitat v1 style actions and move to v2 style #1251

vincentpierre · 2023-04-07T22:31:47Z

Motivation and Context

Following up on our discussion last Tuesday, I made this PR that moves SOME of our sim actions to lab.
Basically, I changed the logic so that actions no longer return observations (except when they do) and the actions no longer step the simulation physics (except when they do). They do when the action is implemented in sim (only move forward, turn left and turn right) because otherwise we cannot do pathfinding and others.
Overall the code for adding new actions is cleaner (see examples/new_actions.py ) and the stepping is also simpler. Please have a look.

Why not move all the actions to lab ? We need sim actions for navigation to calculate the number of actions needed to solve a navigation task in the episode generator. Hence, we need the sim actions to be used in the task.
Couldn't you just reimplement the actions in lab the same way they are in sim? Yes, but I would need to make sure the action configuration are the same in sim and lab, which can be problematic since it forces the sim config to know about the navigation action configs.
Why are we trying to change the way actions are used? We are moving towards having the lab actions as a default since they are easier to use, they are the only actions habitat2.0 uses.

How Has This Been Tested

Unit tests pass, plus I used a script to make sure the new actions (like tilt camera) do the same thing.

Types of changes

[Refactoring] Large changes to the code that improve its functionality or performance

Checklist

My code follows the code style of this project.
I have updated the documentation if required.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes if required.

vincentpierre · 2023-04-13T18:13:03Z

habitat-lab/habitat/core/embodied_task.py

-                task=self,
-                is_last_action=is_last_action,
-            )
+        return task_action.step(


Most of the time, task_action will return None, but for some legacy reason, it will sometimes return the observations. This is because sim.step(action) returns observations. In order to avoid rendering observations twice, we will use the observations computed in task_action when they are available.

vincentpierre · 2023-04-13T18:14:31Z

habitat-lab/habitat/core/embodied_task.py

+        observations: Any = None
        if isinstance(action_name, tuple):  # there are multiple actions
-            for i, a_name in enumerate(action_name):
-                self._step_single_action(
-                    observations,
+            for a_name in action_name:
+                observations = self._step_single_action(
                    a_name,
                    action,
                    episode,
-                    i == len(action_name) - 1,
                )
        else:
-            self._step_single_action(
-                observations, action_name, action, episode
+            observations = self._step_single_action(
+                action_name, action, episode
            )


After this code, if observations is None, then we will need to get the observations from the simulator and lab sensors. If it is not None, this means the environment was stepped by the action, so we DO NOT call sim.step()

vincentpierre · 2023-04-13T18:16:01Z

habitat-lab/habitat/core/registry.py

@@ -181,23 +181,6 @@ def register_dataset(cls, to_register=None, *, name: Optional[str] = None):
            "dataset", to_register, name, assert_type=Dataset
        )

-    @classmethod
-    def register_action_space_configuration(


We deprecate registering new action spaces for sim. If you want to add new actions, you can directly add them to the action space of lab. YOU CAN NO LONGER create new sim actions. The only sim actions we have will be hardcoded. They are move forward turn left and turn right. We use them for pathfinding and other tools.

vincentpierre · 2023-04-13T18:16:26Z

habitat-lab/habitat/sims/habitat_simulator/habitat_simulator.py

-            self.habitat_config.action_space_config
-        )(self.habitat_config).get()
+
+        agent_config.action_space = {


This is where the sim action space is hard coded.

vincentpierre · 2023-04-13T18:18:07Z

habitat-lab/habitat/tasks/rearrange/actions/actions.py

@@ -60,19 +60,14 @@ def reset(self, *args, **kwargs):
        super().reset(*args, **kwargs)
        self.does_want_terminate = False

-    def step(self, task, *args, is_last_action, **kwargs):
+    def step(self, task, *args, **kwargs):


Getting rid of is_last_action for simplicity.

vincentpierre · 2023-04-13T18:19:55Z

test/test_sensors.py

@@ -591,14 +583,6 @@ def test_noise_models_rgbd():
            "RedwoodDepthNoiseModel"
        )

-        config.habitat.simulator.action_space_config = "pyrobotnoisy"


Deprecating pyrobotnoisy and its test

ASzot · 2023-04-14T21:43:37Z

habitat-lab/habitat/core/embodied_task.py

            )

+        self._sim.step_physics(1.0 / 60.0)  # type:ignore


What is this call? Why is this using a fixed 60Hz? This didn't seem to be here before, why add it?

The simulator method always steps physics with a dt of 1/60 (see here)
Before, each action was calling sim.step which is INCREDIBLY INEFFICIENT WHEN THERE ARE MORE THAN ONE ACTION, so instead, the actions now just perform the action and so not step the simulator. The stepping of the simulator happens in EmbodiedTask. Since there is no action to perform and the simulator just the physics needs to be stepped, we do not call sim.step (since this method takes as input an action that does not exist) but instead, we manually step the physics with sim.step_physics.

Agreed this step size should be configured somewhere rather than locked at 60Hz.
True this is the default, but better that it is configured than a magic number.

ASzot · 2023-04-14T21:45:21Z

All the HabitatSim action registration code inhabitat-lab/habitat/tasks/rearrange/__init__.py should also be removed.

vincentpierre · 2023-04-14T22:50:53Z

All the HabitatSim action registration code in habitat-lab/habitat/tasks/rearrange/__init__.py should also be removed.

Thanks, I think I just forgot about those. I removed them now.

… I cannot read.

examples/new_actions.py

Skylion007 · 2023-05-03T19:13:16Z

habitat-lab/habitat/core/embodied_task.py

        )

    def step(self, action: Dict[str, Any], episode: Episode):
        action_name = action["action"]
        if "action_args" not in action or action["action_args"] is None:
            action["action_args"] = {}
-        observations: Any = {}
+        observations: Any = None


Suggested change

observations: Any = None

observations: Optional = None

habitat-lab/habitat/tasks/nav/nav.py

Skylion007 · 2023-05-03T19:14:51Z

habitat-lab/habitat/tasks/nav/nav.py

+        for sensor_name in sensor_names:
+            sensor = self._sim.agents[0]._sensors[sensor_name].node  # type: ignore
+            sensor.rotation = sensor.rotation * mn.Quaternion.rotation(
+                mn.Rad(np.deg2rad(amount)), mn.Vector3.x_axis()


You can use Magnum degree to rad here

aclegg3

Overall looks good to me.

aclegg3 · 2023-05-04T15:13:16Z

habitat-lab/habitat/core/embodied_task.py

            )

+        self._sim.step_physics(1.0 / 60.0)  # type:ignore


Agreed this step size should be configured somewhere rather than locked at 60Hz.
True this is the default, but better that it is configured than a magic number.

…rch#1251) * no yet working * movement is working * moving camera * wip * oops * removing action code * Updating the new actions of the examples * removing more code and configurations * changes for bug fixes * not testing sim in lab * changing a few more configs * changing config * new approach * this is not going to work beause of pathfinding * CMD-Z * CMD-Z on configs * fixing tests * COMMAND - ZZZZZZ * simplify * simplify * edit README * readding tests * removing _try_register_rearrange_task` * readding code that I deleted in my previous commit because apparently I cannot read. * addressing comments * addressing Alex comment --------- Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>

vincentpierre added 5 commits April 6, 2023 08:57

no yet working

2756b8f

movement is working

026ad35

moving camera

2a15ab1

wip

af2e92c

oops

547cf0b

vincentpierre self-assigned this Apr 7, 2023

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 7, 2023

vincentpierre added 17 commits April 7, 2023 16:22

removing action code

d45d5c7

Updating the new actions of the examples

5b102fd

removing more code and configurations

658f88a

changes for bug fixes

36a869e

not testing sim in lab

514f329

changing a few more configs

0284ab7

changing config

fa416ea

new approach

6e221dc

this is not going to work beause of pathfinding

2ed17c9

CMD-Z

114f1a9

CMD-Z on configs

b6da3c8

fixing tests

1ce2ad5

COMMAND - ZZZZZZ

9d55fb3

simplify

9854d89

simplify

4fe7102

edit README

a9cf6bb

readding tests

1552cd1

vincentpierre commented Apr 13, 2023

View reviewed changes

vincentpierre marked this pull request as ready for review April 13, 2023 18:23

vincentpierre requested review from devendrachaplot, aclegg3 and ASzot April 13, 2023 20:39

ASzot reviewed Apr 14, 2023

View reviewed changes

removing _try_register_rearrange_task`

899a6ae

readding code that I deleted in my previous commit because apparently…

d81c1ad

… I cannot read.

vincentpierre requested a review from ASzot April 25, 2023 17:52

ASzot approved these changes Apr 25, 2023

View reviewed changes

Skylion007 reviewed May 3, 2023

View reviewed changes

examples/new_actions.py Outdated Show resolved Hide resolved

Skylion007 reviewed May 3, 2023

View reviewed changes

examples/new_actions.py Show resolved Hide resolved

Skylion007 reviewed May 3, 2023

View reviewed changes

habitat-lab/habitat/tasks/nav/nav.py Show resolved Hide resolved

Skylion007 reviewed May 3, 2023

View reviewed changes

addressing comments

a01a5ef

aclegg3 approved these changes May 4, 2023

View reviewed changes

vincentpierre added 2 commits May 5, 2023 14:31

addressing Alex comment

f100aeb

Merge branch 'main' into habitat1_lab_actions

2aee7f2

vincentpierre merged commit 04d9ed2 into facebookresearch:main May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing Habitat v1 style actions and move to v2 style #1251

Removing Habitat v1 style actions and move to v2 style #1251

vincentpierre commented Apr 7, 2023 •

edited

Loading

vincentpierre Apr 13, 2023

vincentpierre Apr 13, 2023

vincentpierre Apr 13, 2023

vincentpierre Apr 13, 2023

vincentpierre Apr 13, 2023

vincentpierre Apr 13, 2023

ASzot Apr 14, 2023

vincentpierre Apr 14, 2023

aclegg3 May 4, 2023

ASzot commented Apr 14, 2023

vincentpierre commented Apr 14, 2023

Skylion007 May 3, 2023

Skylion007 May 3, 2023

aclegg3 left a comment

aclegg3 May 4, 2023

Removing Habitat v1 style actions and move to v2 style #1251

Removing Habitat v1 style actions and move to v2 style #1251

Conversation

vincentpierre commented Apr 7, 2023 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

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

ASzot commented Apr 14, 2023

vincentpierre commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aclegg3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentpierre commented Apr 7, 2023 •

edited

Loading