-
Notifications
You must be signed in to change notification settings - Fork 505
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
Environment Factory+Habitat 2.0 Code Cleanup #1401
Conversation
…pr.py Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com>
…pr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com>
…pr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com>
@dataclass | ||
class VectorEnvFactoryConfig(HabitatBaselinesBaseConfig): | ||
""" | ||
`_target_` points to the `VectorEnvFactory` to setup the vectorized | ||
environment. Defaults to the Habitat vectorized environment setup. | ||
""" | ||
|
||
_target_: str = ( | ||
"habitat_baselines.common.habitat_env_factory.HabitatEnvFactory" | ||
) | ||
|
||
|
||
@dataclass | ||
class HydraCallbackConfig(HabitatBaselinesBaseConfig): | ||
""" | ||
Generic callback option for Hydra. Used to create the `_target_` class or | ||
call the `_target_` method. | ||
""" | ||
|
||
_target_: Optional[str] = None | ||
|
||
|
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.
Should these two be merged into a single class? Like InstantiationConfig
? It seems they have the same attribute.
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 don't think so because I want the default to be different.
@@ -413,6 +437,8 @@ class HabitatBaselinesConfig(HabitatBaselinesBaseConfig): | |||
log_file: str = "train.log" | |||
force_blind_policy: bool = False | |||
verbose: bool = True | |||
# Creates the vectorized environment. | |||
vector_env_factory: VectorEnvFactoryConfig = VectorEnvFactoryConfig() |
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.
why not have this be a string? Is it because we need to have _target_
to get Hydra boilerplate?
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.
Yes, it is so hydra.utils.instantiate
will work.
@@ -430,6 +456,11 @@ class HabitatBaselinesConfig(HabitatBaselinesBaseConfig): | |||
load_resume_state_config: bool = True | |||
eval: EvalConfig = EvalConfig() | |||
profiling: ProfilingConfig = ProfilingConfig() | |||
should_log_single_proc_infos: bool = False |
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.
docstring
logger.warn( | ||
"Using the debug Vector environment interface. Expect slower performance." | ||
) | ||
vector_env_cls = ThreadedVectorEnv |
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.
Should this be a separate VectorEnvFactory?
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 don't think so because this is controlling the VectorEnv implementation returned.
from omegaconf import DictConfig | ||
|
||
|
||
class VectorEnvFactory(ABC): |
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.
VectorEnvFactory seem to only have one implementation. Is this overengineered?
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 agree it only has 1 implementation in the current setup. However, this makes it much easier to (1) integrate with external environments that have a custom vectorized environment implementation, (2) create custom vector env logic.
config: "DictConfig", env_class: Union[Type[Env], Type[RLEnv]] | ||
config: "DictConfig", | ||
env_class: Union[Type[Env], Type[RLEnv]], | ||
dataset=None, |
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.
Why does the make env function need to have a dataset specified?
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.
This is to allow passing a dataset to the environment setup so it's not recreated every time. However, by default, it is recreated and the default behavior is unchanged from what was there before.
* Fixed HRL bug * Allowing actions * Allowing None for rnn hidden states * Fixed bug with constraints * Nav improvements * Pddl fixes * Updated printing * Removed comment * Profiling * More HRL fixes * Fixed bugs * Added arm init * Performance profiling system * PR comments * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_predicate.py Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> * doc strings * CI * Comments * Update habitat-baselines/habitat_baselines/rl/hrl/hierarchical_policy.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Comments * CI * Fixup to fixed policy * Fixed tests * Fixed tests * Dataset uprade * PDDL setup * merged * Fixed problem with grasping and open cab * Config options * Env factory * CI * Removed prints * obj sampling * Obj sampler fixes * Better kinematic mode * CI * Fixed stats reporting * removed dataset v2 * removing bad print statements * precommit * pre commit * Sub in clone for robo state * Not always updating RNN state during eval * Added save callback * More docs * Addressed PR comments * HL and LL hidden states * Fixed hidden state issue * More flexibility around RNN hidden state size * Visual encoder flexibility * Fixed some logging * Added GPU device ID to the dataset generator * CI * Tests * Fixed rnn hidden state issue * Removed bad assert * Storage cleanup * Fixed storage args * Rerun CI? * Maybe fixed CI * Fixed sensor overflow problem * Updated logger statement * Removed sort --------- Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com>
* Fixed HRL bug * Allowing actions * Allowing None for rnn hidden states * Fixed bug with constraints * Nav improvements * Pddl fixes * Updated printing * Removed comment * Profiling * More HRL fixes * Fixed bugs * Added arm init * Performance profiling system * PR comments * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_predicate.py Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> * doc strings * CI * Comments * Update habitat-baselines/habitat_baselines/rl/hrl/hierarchical_policy.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Update habitat-lab/habitat/tasks/rearrange/multi_task/pddl_logical_expr.py Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com> * Comments * CI * Fixup to fixed policy * Fixed tests * Fixed tests * Dataset uprade * PDDL setup * merged * Fixed problem with grasping and open cab * Config options * Env factory * CI * Removed prints * obj sampling * Obj sampler fixes * Better kinematic mode * CI * Fixed stats reporting * removed dataset v2 * removing bad print statements * precommit * pre commit * Sub in clone for robo state * Not always updating RNN state during eval * Added save callback * More docs * Addressed PR comments * HL and LL hidden states * Fixed hidden state issue * More flexibility around RNN hidden state size * Visual encoder flexibility * Fixed some logging * Added GPU device ID to the dataset generator * CI * Tests * Fixed rnn hidden state issue * Removed bad assert * Storage cleanup * Fixed storage args * Rerun CI? * Maybe fixed CI * Fixed sensor overflow problem * Updated logger statement * Removed sort --------- Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com> Co-authored-by: Vincent-Pierre BERGES <28320361+vincentpierre@users.noreply.github.com>
Motivation and Context
env_factory
abstraction. This is necessary for custom vectorized environments.ignore_skills
key to hierarchical policy.How Has This Been Tested
Training policies end-to-end.
Types of changes