Skip to content
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

Move gym from habitat_baselines to habitat #864

Merged
merged 13 commits into from
May 10, 2022

Conversation

vincentpierre
Copy link
Contributor

@vincentpierre vincentpierre commented May 4, 2022

Migration Guide

Changes to imports :

When importing the gym definitions, you must replace :

import habitat_baselines.utils.gym_definitions as habitat_gym

with

import habitat.utils.gym_definitions as habitat_gym

When importing the render_wrapper, you must replace :

import habitat_baselines.utils.render_wrapper as habitat_gym

with

import habitat.utils.render_wrapper as habitat_gym

If you were using habitat_baselines.common.baseline_registry to register a RLEnv, you must now use habitat.registry to do so.


Changes to configuration files

The following configuration keys no longer belong to habitat_baselines configs :

GYM_AUTO_NAME
GYM_OBS_KEYS
GYM_ACTION_KEYS
GYM_DESIRED_GOAL_KEYS
GYM_ACHIEVED_GOAL_KEYS
GYM_FIX_INFO_DICT
REWARD_MEASURE
SUCCESS_MEASURE
SUCCESS_REWARD
SLACK_REWARD
END_ON_SUCCESS

Their respective replacements are now in the habitat task configs:

GYM:
    AUTO_NAME
    OBS_KEYS
    ACTION_KEYS
    DESIRED_GOAL_KEYS
    ACHIEVED_GOAL_KEYS
    FIX_INFO_DICT
TASK:
    REWARD_MEASURE
    SUCCESS_MEASURE
    SUCCESS_REWARD
    SLACK_REWARD
    END_ON_SUCCESS

In addition, a new key :

GYM: 
    CLASS_NAME

needs to be added to the task config file with the same value as the current ENV_NAME of the corresponding habitat_baselines config (default value is RearrangeRLEnv).

Motivation and Context

In order to separate habitat from habitat_baselines, we move the gym environments to habitat. This enables users to quickly start using the habitat environments without having to install the habitat_baselines trainers.
Reward and success definitions have been moved from baseline configuration to task configuration.
Modified configs creation script to allow modification of the task config from the baseline config.

How Has This Been Tested

Tested that training starts for all of the registered gym environments tasks
Added a unit tests that runs one episode of each gym environment

properly behaves.

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.

@vincentpierre vincentpierre requested review from ASzot and erikwijmans May 4, 2022 16:19
@vincentpierre vincentpierre self-assigned this May 4, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 4, 2022
@Skylion007
Copy link
Contributor

@vincentpierre Pre-commit is now working properly again. Feel free to pull form master to resolve.

habitat/core/registry.py Outdated Show resolved Hide resolved
test/test_gym.py Outdated Show resolved Hide resolved
done = True
return done

def get_info(self, observations):
return self.habitat_env.get_metrics()


@baseline_registry.register_env(name="NavRLEnv")
@habitat.registry.register_env(name="NavRLEnv")
class NavRLEnv(habitat.RLEnv):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This impacts all tasks, including non-rearrangement tasks (like PointNav/ObjectNav) are we okay with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How big is the impact do you think? How may users make a distinction between the baseline registry and the habitat.registry?
I am having a hard time figuring it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine. It'll impact users that are getting envs out of the baselines registry/registering their own envs, but it's a simple enough fix and this change overall makes sense.

dataset = make_dataset(
config.TASK_CONFIG.DATASET.TYPE, config=config.TASK_CONFIG.DATASET
)
if "TASK_CONFIG" in config:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this? Shouldn't this only be through config, not the TASK_CONFIG as with habitat baselines configurations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is because I want to allow the user to pass either a task config or a baseline config to make the environment. If the user passes a baseline config, this code will look at the task config and use it. Does that make sense ?

vincentpierre and others added 4 commits May 4, 2022 14:53
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the migration guide!

@vincentpierre vincentpierre merged commit 323f786 into facebookresearch:main May 10, 2022
dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024
* merging

* -

* Updating configs

* -

* -

* -

* fixing tests

* fixing tests

* changing configs

* Update habitat/tasks/rearrange/multi_task/composite_sensors.py

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

* Update habitat/tasks/rearrange/multi_task/composite_sensors.py

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

* addressing comments

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
HHYHRHY pushed a commit to SgtVincent/EMOS that referenced this pull request Aug 31, 2024
* merging

* -

* Updating configs

* -

* -

* -

* fixing tests

* fixing tests

* changing configs

* Update habitat/tasks/rearrange/multi_task/composite_sensors.py

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

* Update habitat/tasks/rearrange/multi_task/composite_sensors.py

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

* addressing comments

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants