Skip to content
This repository was archived by the owner on Mar 20, 2026. It is now read-only.

Compatibility fix with Hydra 1.1#3722

Closed
omry wants to merge 2 commits intofacebookresearch:masterfrom
omry:hydra-1.1-compatibility
Closed

Compatibility fix with Hydra 1.1#3722
omry wants to merge 2 commits intofacebookresearch:masterfrom
omry:hydra-1.1-compatibility

Conversation

@omry
Copy link
Copy Markdown
Contributor

@omry omry commented Jul 19, 2021

One of the changes in Hydra 1.1 is that the default composition order is changing.
This is documented here.
In Hydra 1.1, a config is overriding values introduced by the defaults list while in Hydra 1.0 - the defaults list was overriding the values in the config.

fairseq is currently depending on the previous behavior:
The class FairseqConfig defines config values, and it's expecting them to be overridden by the defaults list.
This result in a different config being created when running fairseq_cli/hydra_train.py with Hydra 1.0 and with 1.1.

Hydra 1.1 introduced the _self_ keyword in the defaults list to control the composition order. In order to achieve the behavior of Hydra 1.0, _self_ should be added as the first item in the defaults list.

To allow for a smoother migration, Hydra 1.0 is ignoring _self_ starting from 1.0.7 (previous versions will issue an error).

This diff adds _self_ as the first item in the defaults list the fairseq config, and introduce a dependency a Hydra 1.0 version that is equal or newer to 1.0.7.

Testing:

I ensured that the following yield the same composed config:
Default config with Hydra 1.0.6, 1.0.7 and 1.1.0

examples/wav2vec/config/finetuning/base_10h.yaml with Hydra 1.0.6, 1.0.7 and 1.1.0.

This can be achieved by outputing the generated config using --cfg job and compating the outputs.

@omry omry force-pushed the hydra-1.1-compatibility branch from 96cb8ce to a1333f1 Compare July 19, 2021 22:26
)

super().__init__(cfg.optimizer)
super().__init__(getattr(cfg, "optimizer", None))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails in some tests on Hydra 1.1 due to an OmegaConf 2.1 change making access to non existing fields behave like it does for objects and dicts (raises an exception).

@alexeib
Copy link
Copy Markdown
Contributor

alexeib commented Jul 20, 2021

will this have any effect on external configs (--config-dir + --config-name)? if not lgtm

@omry
Copy link
Copy Markdown
Contributor Author

omry commented Jul 20, 2021

Not supposed to.
As I said, I tested examples/wav2vec/config/finetuning/base_10h.yaml and it's composing the same config.

@omry
Copy link
Copy Markdown
Contributor Author

omry commented Jul 23, 2021

@alexeib, can we get this merged soon?

@alexeib
Copy link
Copy Markdown
Contributor

alexeib commented Jul 24, 2021

sure, can you just make diff from this? merging happens through fbcode, not here

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@jieru-hu has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@jieru-hu merged this pull request in 53802e7.

@gegallego
Copy link
Copy Markdown
Contributor

gegallego commented Aug 19, 2022

Hi! This line could be changed to hydra-core>=1.0.7,<=1.1, right?
Also, omegaconf requirement should then be changed to omegaconf<=2.1 to satisfy this requirement from hydra.

@omry omry deleted the hydra-1.1-compatibility branch August 25, 2022 12:38
@rsxdalv
Copy link
Copy Markdown

rsxdalv commented Jul 5, 2023

@omry Could you please confirm if fairseq works with hydra-core==1.1?

@omry
Copy link
Copy Markdown
Contributor Author

omry commented Jul 7, 2023

@rsxdalv, Nope. I am no longer involved with either.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants