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.

Sign up for GitHub

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

Jump to bottom

Relative recursive defaults take3 #1170

Merged
merged 91 commits into from
Jan 1, 2021
Merged

Relative recursive defaults take3 #1170

merged 91 commits into from
Jan 1, 2021

Conversation

omry
Copy link
Collaborator

@omry omry commented Nov 26, 2020 •
edited
Loading

Design document here

This is a from-scratch implementation.
The biggest change in requirements is that we are now aiming to support relocating sub-trees with packages overrides, which means both config groups and package overrides are relative.

In terms of implementation, this version goes through an intermediate tree that is being converted to a list with a DFS walk.
This simplifies the implementation and will also be great for visualization purposes.

Closes #171 (reimplements)
Closes #326 (reimplements)
Closes #1080 (reimplements)
Closes #1102
Closes #956
Closes #1107
Closes #354

Followup tasks:

To-do list

  • Test with simple config group overrides
  • Test computed package when there are no package overrides in package header
  • test with config group overrides overriding config groups @pkg
  • test overriding config group choices with non-default packages
    • tests for _global_
    • tests for _global_.foo
    • tests for _name_
  • test with config header package override
  • test with both config header and defaults list pkg override
  • Support overriding config group values from the defaults list
    • reconsider support for overriding as before, DECISION: Not happening.
    • Support marked overrides in primary config only
    • Support marked override in all configs
    • Test overriding of config groups with a specified package (@pkg)
    • Overriding of config groups with a specified package (@pkg) when there are multiple choices from same group
    • Handle hydra overrides
  • Experiment use case: test the following from a config in an experiment group
    • Override user config group with and without an external override of the same config group
    • Experiment specified in primary defaults
    • Append experiment file from external overrides
    • Override hydra config group from experiment [+ external override]
    • Include config with an absolute path
    • Test final defaults list with an experiment file
    • Test experiment config as a primary (must have @Package global and absolute addressing of config groups)
  • package header:
    • Consider making relative.
      Decision: package header will remain absolute.
    • Consider deprecating completely.
      Decision: package will not be deprecated for now
  • Test use cases for config extension:
    • Extension from the same config group
    • Extension from absolute config group
    • Extension from a nested config group
  • Test missing ('???') in Defaults List
  • Interpolation support
    • Simple + override
    • Forward + override
    • Interpolate with nested groups
    • Interpolate with group@pkg1
    • Test interpolated config with a defaults list
    • Error if interpolated config defaults list has an overrides
    • Support and deprecate legacy defaults list interpolation style
  • delete support from overrides
    • override to null from defaults list
    • Support delete by group
    • Support delete by group=value
    • Test deletion with a specific package
    • Deletion test with final defaults list
  • Error handling:
    • Error handling for entries that failed to override anything
    • Duplicate self error
    • test handling missing configs mentioned in defaults list (with and without optional)
    • Ambiguous overrides should provide valid override keys for group
    • Test deprecation message when attempting to override hydra configs without override: true
    • Should duplicate entries in results list be an error? (same override key)
  • Integrate with Hydra
    • replace old defaults list computation
    • enable --info=defaults output
    • ensure all tests are passing
    • implement --info=defaults-tree output
  • Drop rename support:
  • Cleanup
    • Remove previous implementation of recursive defaults list and rename new_defaults_list to defaults_list etc.
    • Delete tests and test data of old defaults list impl
    • Clean up package logic from config sources
    • Clean up Hydra 1.0 warnings related to package header
  • TODO: Followup items
    • Consider retaining the final choices in the hydra config node to allow interpolation with their values.
    • Enforce that overrides are at the end of the defaults list
      (with the exception of self that can be after them)
    • Consider override style of: - override hydra/launcher: submitit
    • Profile and optimize default tree composition: speed in line with 1.0
    • Fix error message when overriding a non-existing config group from the command line to not say "append with +"
    • Improve --info defaults-tree output
    • Test cases when config group name matches a keyword (optional, override)
  • Documentation
  • File follow-up tasks

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 26, 2020
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2020

This pull request introduces 1 alert when merging bfecf9f into 143d060 - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 26, 2020

This pull request introduces 1 alert when merging bde870c into 143d060 - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a call

@omry omry force-pushed the relative-recursive-defaults-take3 branch from bde870c to 8a24c63 Compare December 1, 2020 02:39
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 3, 2020

This pull request fixes 1 alert when merging c5bc56a into c1eb243 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 3, 2020

This pull request fixes 1 alert when merging 5700988 into c1eb243 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 5700988 to d8b840c Compare December 3, 2020 23:28
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 3, 2020

This pull request fixes 1 alert when merging d8b840c into c1eb243 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 4, 2020

This pull request fixes 1 alert when merging 83eaebb into c1eb243 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@omry omry mentioned this pull request Dec 5, 2020
Closed
@odelalleau odelalleau mentioned this pull request Dec 7, 2020
Merged
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 8, 2020

This pull request fixes 1 alert when merging 5388f5f into 96d232d - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 5388f5f to 6d43679 Compare December 11, 2020 04:05
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 11, 2020

This pull request introduces 4 alerts when merging 6d43679 into 522431d - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 12, 2020

This pull request introduces 4 alerts when merging 62ad0a5 into b2d5310 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 12, 2020

This pull request introduces 4 alerts when merging 3e802dc into b2d5310 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unused import
  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 3e802dc to fa0c7aa Compare December 13, 2020 02:40
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2020

This pull request introduces 1 alert when merging fa0c7aa into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from fa0c7aa to b27e6c9 Compare December 13, 2020 03:18
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2020

This pull request introduces 1 alert when merging b27e6c9 into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2020

This pull request introduces 1 alert when merging e04ce9c into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from e04ce9c to 97e5c83 Compare December 13, 2020 06:14
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2020

This pull request introduces 1 alert when merging 97e5c83 into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 97e5c83 to 0da4dd3 Compare December 15, 2020 01:21
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 15, 2020

This pull request introduces 2 alerts when merging 0da4dd3 into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 0da4dd3 to 30a057f Compare December 15, 2020 01:49
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 15, 2020

This pull request introduces 2 alerts when merging 30a057f into 9122b03 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from 30a057f to a7318cf Compare December 15, 2020 02:34
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 15, 2020

This pull request introduces 2 alerts when merging a7318cf into 79127d5 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Variable defined multiple times

@omry omry force-pushed the relative-recursive-defaults-take3 branch from a7318cf to 245f731 Compare December 15, 2020 04:00
@omry omry force-pushed the relative-recursive-defaults-take3 branch 8 times, most recently from 7ccd023 to 43da2b8 Compare December 29, 2020 23:45
@omry omry mentioned this pull request Dec 30, 2020
Closed
@omry omry force-pushed the relative-recursive-defaults-take3 branch 3 times, most recently from 04f1b7d to b6a6ef1 Compare December 30, 2020 20:58
@omry omry mentioned this pull request Dec 31, 2020
Closed
@omry omry force-pushed the relative-recursive-defaults-take3 branch 2 times, most recently from 2bda47d to 2f7e3b1 Compare January 1, 2021 05:09
@omry omry mentioned this pull request Jan 1, 2021
Merged
10 tasks
omry added 4 commits January 1, 2021 13:34
@omry
Documentation update
b36ecb8 
- Defaults List
- Packages
- Extending configs
- Configuring experiments
@omry
minor updates to error messages
337ee1a
@omry
news fragments updated
f86a90e
@omry
updated todos.
cbd055f
@omry omry force-pushed the relative-recursive-defaults-take3 branch from 2f7e3b1 to cbd055f Compare January 1, 2021 21:38
@omry omry merged commit ea8d435 into master Jan 1, 2021
@omry omry deleted the relative-recursive-defaults-take3 branch January 1, 2021 22:17
@omry omry mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
2 participants
@omry @facebook-github-bot