--config should replace config, not merge them

[sophie-xhonneux]

What happened?

Overriding of configs doesn't work as expected.

When we try to override the default

...
### Example validation and training config for mask token modelling in physical space
validation_config: {"losses": {LossPhysical: {weight: 1.0, loss_fcts: [['mse', 1.0]]},}}
...

with a different validation_config

validation_config:
  losses:
    LossLatentSSLStudentTeacher: { 
      "weight": 1.0,
      "JEPA": {'weight': 5, "loss_extra_args": {}, "out_dim": 2048} }

We get the two configs appended

validation_config:
  losses:
    LossPhysical:
      weight: 1.0
      loss_fcts:
        - ["mse", 1.0]
    LossLatentSSLStudentTeacher:
      weight: 1.0
      JEPA:
        weight: 5
        loss_extra_args: {}
        out_dim: 2048

which is not the intended outcome

What are the steps to reproduce the bug?

No response

Hedgedoc link to logs and more information. This ticket is public, do not attach files directly.

No response

[kacpnowak]

Hi Sophie, what would be the intended outcome? Currently all configs (default, private, overwrite) are being merged together so it seems to me like the correct behavior.

[sophie-xhonneux]

Previously if you overrode something like num_samples_per_mini_epoch: 32 with num_samples_per_mini_epoch: 64, you would only retain the later, because that key can only exist once. So the expected behaviour would be (in the above example)

validation_config:
  losses:
    LossLatentSSLStudentTeacher: { 
      "weight": 1.0,
      "JEPA": {'weight': 5, "loss_extra_args": {}, "out_dim": 2048} }

Else we can never remove LossPhysical value from the default config with overrides, which would hamper certain things like integration-tests and others

[kacpnowak]

Omegaconf's merge uses update with "merge" set to True, which results in lists and dicts being merged rather than assigned, resulting in this behavior

[sophie-xhonneux]

Is there a workaround?

[kacpnowak]

We would have to do something like this:

base = OmegaConf.create({"a": 1, "subconfig": {"x": 1, "y": 2}})
override = OmegaConf.create({"subconfig": {"x": 99}})

if override in base:
   del base["subconfig"] 
base = OmegaConf.merge(base, override)

I think it would a good moment to think about how we want config to behave. Recently we changed time handling, so this change shouldn't be a problem. There was also a point raised by. @grassesi to get rid of lists, which IMO is a needed change

[sophie-xhonneux]

@clessig @tjhunter do you have thoughts on this?

[clessig]

The overrides are happening automatically so far since this are single values, not dicts or lists. With --options we so far always assumed ovrriding of existing keys, independent if this are values, dicts, lists. I am not sure what's the best way to implement it but it should happen. (We should have already gotten rid of all list so far).

[kacpnowak]

I'd be happy to take care of this as I already worked on time handling

[clessig]

@grassesi : this is a real issue and we should fix this as soon as possible. At the moment it also leads to a spurious dependence of the integration tests on the default_config.yml as it is in a users WeartherGenerator clone. In consequence, integration tests are not reproducible.

[clessig]

We should have an additional --override-config flag that specifies a diff config w.r.t the base config, which comes from --config and which itself defaults to the default_config.yml.

[tjhunter]

I put my thoughts in the SW dev channel, easier to have an informal discussion there. I think the current merging behavior is the most reasonable one when considering other options, based on my experience. I put a couple of alternatives for expressing the config.

[clessig]

I put my thoughts in the SW dev channel, easier to have an informal discussion there. I think the current merging behavior is the most reasonable one when considering other options, based on my experience. I put a couple of alternatives for expressing the config.

The merging behaviour is fine. But --config needs to overwrite the default config!