Performance improvement on lab #1120

vincentpierre · 2023-02-03T23:04:32Z

Motivation and Context

It seems checking if a key is in a Hydra DictConfig is rather slow. We cache the agent index to avoid having to recompute it every time.

Findings: If you have to access a configuration field several times per second, please store the value somewhere instead of accessing the configuration. The configuration will not change, I promise.

1 process interact benchmark
Before regression of November 15 2022 : 168.11183922196446
After regression of November 15 2022 : 134.35855893433697
After first commit of this PR : 158.04943632647434
After second commit of this PR : 163.95664144856636

How Has This Been Tested

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

Skylion007 · 2023-02-03T23:39:17Z

@vincentpierre Another perf solution for this is to materialize the HydraConfig back into a slot enabled dataclass, and that would enable extremely fast access.

vincentpierre · 2023-02-03T23:50:30Z

Another perf solution for this is to materialize the HydraConfig back into a slot enabled dataclass, and that would enable extremely fast access.

You have a reference link on how to do this ?

Skylion007 · 2023-02-04T00:17:29Z

https://omegaconf.readthedocs.io/en/2.1_branch/usage.html#using-structured-config-mode

ASzot

There are measures everywhere that access the Hydra config. If we follow your suggestion of Findings: If you have to access a configuration field several times per second, please store the value somewhere instead of accessing the configuration. this will require many more changes? (for example see all the reads from the config in this file).

vincentpierre · 2023-02-04T01:43:17Z

@Skylion007
The documentation does not say one is faster than the other or what the trade offs are :

        :param structured_config_mode: Specify how Structured Configs (DictConfigs backed by a dataclass) are handled.
            - By default (``structured_config_mode=SCMode.DICT``) structured configs are converted to plain dicts.
            - If ``structured_config_mode=SCMode.DICT_CONFIG``, structured config nodes will remain as DictConfig.
            - If ``structured_config_mode=SCMode.INSTANTIATE``, this function will instantiate structured configs
              (DictConfigs backed by a dataclass), by creating an instance of the underlying dataclass.
              ```
 We often need to edit the configs at runtime and I am afraid converting the configs will cause issues within the code.

ASzot · 2023-02-04T02:00:07Z

Many of the actions also constantly read from the Hydra configs.

Skylion007 · 2023-02-04T22:09:39Z

@Skylion007 The documentation does not say one is faster than the other or what the trade offs are :

        :param structured_config_mode: Specify how Structured Configs (DictConfigs backed by a dataclass) are handled.
            - By default (``structured_config_mode=SCMode.DICT``) structured configs are converted to plain dicts.
            - If ``structured_config_mode=SCMode.DICT_CONFIG``, structured config nodes will remain as DictConfig.
            - If ``structured_config_mode=SCMode.INSTANTIATE``, this function will instantiate structured configs
              (DictConfigs backed by a dataclass), by creating an instance of the underlying dataclass.
              ```
 We often need to edit the configs at runtime and I am afraid converting the configs will cause issues within the code.

It will be faster because slot enabled dataclass (or attrs) access is the fastest lookup in Python (except for caching them as variables of course). It's not necessarily faster for one time access, and it will have some overhead, but amortized it will be faster than accessing a normal Python dictionary and therefore faster than accessing an OmegaConf one. See https://threeofwands.com/attrs-ii-slots/

Skylion007 · 2023-02-04T22:12:53Z

@vincentpierre Python also has some other caching mechanism implemented on Python classes to make lookup more efficient than a naive dict implementation so it will definitely be more efficient to access.

Skylion007 · 2023-02-06T19:11:04Z

habitat-lab/habitat/config/default_structured_configs.py

 class GymConfig(HabitatBaseConfig):
    auto_name: str = ""
    obs_keys: Optional[List[str]] = None
    action_keys: Optional[List[str]] = None
-    achieved_goal_keys: List = field(default_factory=list)
-    desired_goal_keys: List[str] = field(default_factory=list)
+    achieved_goal_keys: List = []


You still need to use an attrs.factory or the field alias unfortunately just like dataclasses: https://www.attrs.org/en/stable/api.html#attr.ib

Apparently, you can just attr.Factory as syntactic sugar: https://www.attrs.org/en/stable/examples.html#defaults

It doesn't look like I need to. I looked into the configuration that gets passed to run.py and it looks like all the fields (even lists) are correct.

@vincentpierre It works for now, but it's bugprone behvaior. We have rule against it explicitly, but they don't work on attrs:

B006: Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.

^ this also applies to dataclasses and attrs

@vincentpierre, it's less likely to be an issue since it would primarily be an issue with rewriting the configs, which we do sparingly, but it could lead to hard to spot bugs down the line.

Quote from this comment

Note that when OmegaConf uses the default value to create a ListConfig, it creates a separate container (i.e., adding element to the ListConfig will not add elements to the original Python list it was created from). As a result, it should actually be safe to provide a mutable list as default value (as long as you don't use that class elsewhere...)

@vincentpierre Still really yucky if you want to use the dataclasses without Hydra (like for UnitTesting). Also the attrs.slot thing doesn't buy us a whole lot of speed unless we are instantiating the hydra config back into the dataclass.

A better method for speedup is just to use instationate the config when storing it in the class. This converts it into the dataclass which will be faster to access than the DictConfig with or without slots.

@vincentpierre You can even just make it pass all the config keys to the class like so: https://hydra.cc/docs/advanced/instantiate_objects/overview/

Skylion007 · 2023-02-07T17:27:06Z

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

@@ -233,6 +242,9 @@ class ArmRelPosKinematicReducedActionStretch(RobotAction):
    def __init__(self, *args, config, sim: RearrangeSim, **kwargs):
        super().__init__(*args, config=config, sim=sim, **kwargs)
        self.last_arm_action = None
+        self._delta_pos_limit = self._config.delta_pos_limit


A lot of code refactoring could be safe here by just converting the omega config to the dataclass here and access those members, no?

I will leave this refactor in place, just because I think it makes more sense.
~~I instantiated the configs for the RobotActions, but leaving the measures and sensors as is because they all use the config differently~~ Trying this broke everything. I just want to merge this now.

Skylion007

Given the issues with attr.Factory, I would actually prefer if they remained dataclasses. Either way this looks god to me now.

* a few % * adding anoter percent or two * speedup grasp * optimizing rearrange_sim * adding the measures * moving to attr.s * converting the action classes to be faster * typo * resolving issues --------- Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>

…ch#1182)

* a few % * adding anoter percent or two * speedup grasp * optimizing rearrange_sim * adding the measures * moving to attr.s * converting the action classes to be faster * typo * resolving issues --------- Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>

…ch#1182)

a few %

001a8dc

vincentpierre self-assigned this Feb 3, 2023

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

adding anoter percent or two

afc74c5

vincentpierre added 2 commits February 3, 2023 15:41

speedup grasp

1215ae6

optimizing rearrange_sim

f345f2c

ASzot suggested changes Feb 4, 2023

View reviewed changes

adding the measures

6e8c13d

vincentpierre added 2 commits February 6, 2023 10:34

Merge branch 'main' into perf_01

25c03e4

moving to attr.s

d1a213b

Skylion007 reviewed Feb 6, 2023

View reviewed changes

converting the action classes to be faster

f6fa652

vincentpierre requested a review from ASzot February 6, 2023 19:53

vincentpierre added 2 commits February 6, 2023 13:55

typo

d4af8e4

resolving issues

ea9c00a

Skylion007 reviewed Feb 7, 2023

View reviewed changes

vincentpierre force-pushed the perf_01 branch from ceeda41 to ea9c00a Compare February 7, 2023 22:13

Skylion007 approved these changes Feb 8, 2023

View reviewed changes

ASzot approved these changes Feb 8, 2023

View reviewed changes

vincentpierre merged commit 93efc7b into facebookresearch:main Feb 8, 2023

ykarmesh mentioned this pull request Feb 22, 2023

Add continuous controller to Navigation tasks to be compatible with robot control #1129

Open

7 tasks

This was referenced Mar 1, 2023

@dataclass-style structured configs to @attrs-style #1172

Closed

Roll back moving to attr.s in #1120 #1182

Merged

rpartsey added a commit that referenced this pull request Mar 3, 2023

rolled back moving to attr.s in #1120 (#1182)

b63b024

dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024

rolled back moving to attr.s in facebookresearch#1120 (facebookresear…

8e8d782

…ch#1182)

HHYHRHY pushed a commit to SgtVincent/EMOS that referenced this pull request Aug 31, 2024

rolled back moving to attr.s in facebookresearch#1120 (facebookresear…

c5d7c75

…ch#1182)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvement on lab #1120

Performance improvement on lab #1120

vincentpierre commented Feb 3, 2023 •

edited by yvsriram

Loading

Skylion007 commented Feb 3, 2023

vincentpierre commented Feb 3, 2023

Skylion007 commented Feb 4, 2023

ASzot left a comment

vincentpierre commented Feb 4, 2023

ASzot commented Feb 4, 2023

Skylion007 commented Feb 4, 2023

Skylion007 commented Feb 4, 2023

Skylion007 Feb 6, 2023

Skylion007 Feb 6, 2023

vincentpierre Feb 6, 2023

Skylion007 Feb 6, 2023

Skylion007 Feb 6, 2023 •

edited

Loading

vincentpierre Feb 7, 2023

Skylion007 Feb 7, 2023

Skylion007 Feb 7, 2023

Skylion007 Feb 7, 2023

vincentpierre Feb 7, 2023 •

edited

Loading

Skylion007 left a comment

Performance improvement on lab #1120

Performance improvement on lab #1120

Conversation

vincentpierre commented Feb 3, 2023 • edited by yvsriram Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Skylion007 commented Feb 3, 2023

vincentpierre commented Feb 3, 2023

Skylion007 commented Feb 4, 2023

ASzot left a comment

Choose a reason for hiding this comment

vincentpierre commented Feb 4, 2023

ASzot commented Feb 4, 2023

Skylion007 commented Feb 4, 2023

Skylion007 commented Feb 4, 2023

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

Skylion007 Feb 6, 2023 • edited Loading

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

vincentpierre Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Skylion007 left a comment

Choose a reason for hiding this comment

vincentpierre commented Feb 3, 2023 •

edited by yvsriram

Loading

Skylion007 Feb 6, 2023 •

edited

Loading

vincentpierre Feb 7, 2023 •

edited

Loading