habitat-lab icon indicating copy to clipboard operation
habitat-lab copied to clipboard

Better config stage 1

Open erikwijmans opened this issue 5 years ago • 3 comments

Motivation and Context

The current integration between habitat_baseline's and habitat's config is tenuous at best. One issue with the two is that both assume that their config begins at the root. This assumption makes it near impossible to make the two live in the same config. This PR puts habitat's config under a habitat node and habitat-baselines under a habitat_baseline node. This makes merging the two configs trivial and allows a single yaml file to modify both habitat and habitat_baseline's configs.

This PR also makes composition of configs the expected route. Composition is somewhat scary from a reproducibility standpoint, but we do save the config with the weights, so that source of truth is always there.

This PR also makes a slightly more controversial change and moves to lower case instead of upper case names in the config system. Lower case is more common, easier to read, and prettier IMO. I can revert this however.

There are a couple other minor changes that happened along the way. The most notable of these are

  1. Removal of the SENSORS field from habitat_baseline's config, although it is kinda long, it is easy to change agent sensors as habitat.simulator.agent_0.sensors now, so that field isn't needed.
  2. Renaming of NUM_PROCESSES to simulators_per_gpu. The old name is kinda confusing if you don't know that the separate simulator instances are ran in subprocesses. It also clashes with the naming that distributed RL (and DD-PPO) use for all the different worker processes (which are referred to as processes all the time).

How Has This Been Tested

All the tests pass!

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.

erikwijmans avatar Apr 15 '20 22:04 erikwijmans

Codecov Report

Merging #371 into master will decrease coverage by 0.55%. The diff coverage is 83.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
- Coverage   77.53%   76.98%   -0.56%     
==========================================
  Files         108      107       -1     
  Lines        7523     7425      -98     
==========================================
- Hits         5833     5716     -117     
- Misses       1690     1709      +19     
Impacted Files Coverage Δ
examples/benchmark.py 0.00% <0.00%> (ø)
examples/vln_benchmark.py 0.00% <0.00%> (ø)
examples/vln_reference_path_follower_example.py 0.00% <0.00%> (ø)
habitat/core/utils.py 52.00% <0.00%> (+0.68%) :arrow_up:
habitat/datasets/pointnav/pointnav_generator.py 91.11% <ø> (ø)
habitat/datasets/vln/r2r_vln_dataset.py 51.28% <0.00%> (ø)
habitat/tasks/nav/object_nav_task.py 62.29% <0.00%> (ø)
habitat_baselines/agents/slam_agents.py 0.00% <0.00%> (ø)
habitat_baselines/common/baseline_registry.py 100.00% <ø> (ø)
habitat_baselines/rl/ddppo/policy/resnet_policy.py 85.98% <ø> (-4.68%) :arrow_down:
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6cf86df...65eed5b. Read the comment docs.

codecov[bot] avatar Apr 15 '20 23:04 codecov[bot]

As I understand from the conversation in the issue the PR is blocked till:

  • recursive defaults implementation is in progress (this is essentially a near rewrite of the config composition logic).
  • support for controlling composition order in defaults (self) is coming as well (actually already working in the branch of the recursive defaults). will be supported in Hydra. Otherwise, we need to implement corresponding functionality on our side with references to Hydra core.

mathfac avatar Jan 14 '21 01:01 mathfac

As I understand from the conversation in the issue the PR is blocked till:

  • recursive defaults implementation is in progress (this is essentially a near rewrite of the config composition logic).
  • support for controlling composition order in defaults (self) is coming as well (actually already working in the branch of the recursive defaults). will be supported in Hydra. Otherwise, we need to implement corresponding functionality on our side with references to Hydra core.

This is still using yacs, just changing things around to make it friendlier. Those are things that were blocking the move to hydra. This never got really any attention and keeping it updated while other PRs are getting merged it a huge amount of work so I didn't.

erikwijmans avatar Jan 14 '21 01:01 erikwijmans