Usage of an explicit overrides node

[omry]

Heya,

I think this not a great idea, happy to discuss why:

mbrl-lib/mbrl/algorithms/pets.py

Line 24 in c7a7425

max_horizon = cfg.overrides.get(

[luisenp]

Hey, curious to know why.

[omry]

The core idea behind Hydra is that it gives you the config you need.
The app should not worry about overrides and such, it should just get the config it needs to consume.

Is there a reason you prefer this over offloading this logic to Hydra's config composition?

[luisenp]

I'm still using Hydra composition, but maybe the name overrides is misleading?

The reason for this config group (overrides) is that I can give users default hyperparameter values that work well for each specific environment, in separate files. Maybe cfg.hyperparameters is a clearer name?

Of course, if there is a better pattern for this I'm happy to use it, I couldn't find one at the time I wrote this.

[natolambert]

I uploaded a version that uses nested defaults to get around overrides specifying algorithm and model
https://github.com/natolambert/conf-structure/tree/main/conf

[omry]

I'm still using Hydra composition, but maybe the name overrides is misleading?

The reason for this config group (overrides) is that I can give users default hyperparameter values that work well for each specific environment, in separate files. Maybe cfg.hyperparameters is a clearer name?

Of course, if there is a better pattern for this I'm happy to use it, I couldn't find one at the time I wrote this.

How you represent the configs in the config dir is one thing, what the application is getting is another.
The version uploaded by @natolambert is using nested config groups (not nested defaults) and this pattern which incidentally was created specifically for this use case :).

Hydra 1.1 will actually have nested default lists, which will probably offer an even cleaner solution for those.
It will also clean up the syntax of the pattern Nathan is using.
You can try Hydra 1.1 dev release, in fact as your project is still in development I recommend that you switch to it already.
The dev release is stable enough and there aren't going to be many changes before it gets to a release candidate.

[luisenp]

I'm a bit confused by this sentence:

"How you represent the configs in the config dir is one thing, what the application is getting is another."

What is this other thing the application is getting? I'm using exactly the configuration that I specify in my yaml file. Or, on second look, are you referring to the fact that I'm using another config path as a default (cfg.algorithm.agent.planning_horizon)? If so, then I can see why that's not desirable, I can revisit this.

[omry]

What I am going for is that the config the application is getting should be exactly what it needs.
The application needs a value, it should get it in the place it expects it.
it should not apply override logic to get the right value, Hydra can do it when it creates the config object.

If you buy into that, the question becomes: "How do we get Hydra to build the config object you need?"

(Of course, if your application actually needs both values for some reason what you are doing is fine, but I suspect you are using one or the other. never both).

[luisenp]

@natolambert Thanks for sharing. Keep in mind that the original reason I went with the current design is to avoid having a bunch of dynamics_model and optimizer folders replicated for each environment, since most of them will look exactly the same. I'd rather have some default configurations for model and planner, then for each experiment (i.e., combination of model+algorithm+env) I can just change some of the default parameters (for all groups) in a single file. This seems to me way easier to read.

My ideal scenario would be that Hydra lets me do something like the structure below. @omry is this already possible?

model_cfg
  -param1: default_model_p1
  -param2: default_model_p2

planner_cfg:
  -param1: default_planner_p1
  -param2: default_planner_p2

experiment_specific_cfg:
  -model_cfg.param1 = override_with_something
  -planner_cfg.param2 = override_with_something_else

PS: Writing this, I think cfg.experiment is probably a better name than cfg.overrides.

[omry]

Hydra 1. 1 offers several ways to avoid config repetition:

• Extending configs: https://hydra.cc/docs/next/patterns/extending_configs.
• Defining experiments: https://hydra.cc/docs/next/patterns/configuring_experiments
• Selecting a config to use based on the name of other choices: specializing config: https://hydra.cc/docs/next/patterns/specializing_config

Look at those 3 docs.

The last one is a cleaner form of what mbrl was using (that predated anything close to nested defaults, which enables the first two).
Reading your description of the problem, I think what you are really after is possible by extending configs.

[luisenp]

Thanks @omry, I'll take a look at those!

[omry]

Cool. They are all implementations of what is described in https://hydra.cc/docs/next/advanced/defaults_list, be sure to read it first.
(It's a bit more dense, but will give you a solid foundation).