habitat-lab
habitat-lab copied to clipboard
Better config stage 1
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
- 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.sensorsnow, so that field isn't needed. - 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.
Codecov Report
Merging #371 into master will decrease coverage by
0.55%. The diff coverage is83.07%.
@@ 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 dataPowered by Codecov. Last update 6cf86df...65eed5b. Read the comment docs.
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.
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.