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.

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

Move from Argparse to Omegaconf #300

Merged
merged 41 commits into from
Sep 27, 2022
Merged

Move from Argparse to Omegaconf #300

merged 41 commits into from
Sep 27, 2022

Conversation

vturrisi
Copy link
Owner

@vturrisi vturrisi commented Sep 8, 2022

Opening the PR now so that we can properly visualize the changes and eventually get the tests rolling.

@vturrisi vturrisi changed the title Adds support for Omegaconf Changes from Argparse to Omegaconf Sep 8, 2022
@vturrisi vturrisi changed the title Changes from Argparse to Omegaconf Move from Argparse to Omegaconf Sep 9, 2022
@turian
Copy link
Contributor

turian commented Sep 11, 2022

@vturrisi Why not use hydra from FAIR. It is a more powerful, and can easily convert to and from OmegaConf options?

@turian
Copy link
Contributor

turian commented Sep 11, 2022

I have used hydra in the past and could give feedback and code review

@vturrisi
Copy link
Owner Author

@turian yes, using hydra is another possibility. What would be the main advantages over omegaconf?

@turian
Copy link
Contributor

turian commented Sep 11, 2022

@vturrisi Hydra is a library that builds on Omegaconf, so it's more powerful. (Nonetheless, you can easily cast things to OmegaConf if need be.) But it's pretty easy to port to it.

TBH I am not super familiar with OmegaConf because I just just Hydra and it seems to handle the casts to OmegaConf automatically as necessary.

Here is what Hydra does that's cool, let me know if OmegaConf does all this:

  • You group your configs into different yaml files in config, maybe them easy to find.
  • You can compose the YAML files but easily override them.

Skim the intro: https://hydra.cc/docs/intro/
and you'll see what I mean.

There's also the Structured Configs which I haven't allowed, but I believe it allows you to statically type all your parameters and not need the YAML file. I haven't really needed this but super mission-critical services might want this.

BTW, Lightning-Hydra-Template which implements best practices also uses hydra?

solo/methods/barlow_twins.py Show resolved Hide resolved
solo/methods/mae.py Show resolved Hide resolved
@turian
Copy link
Contributor

turian commented Sep 11, 2022

One more benefit of hydra that I don't think omegaconf has.

Let's say you have two YAML files, one for a super large model and one for a tiny model.

hydra let's you switch configs using just the name of the YAML. Does omegaconf require you to

(I wanted to take a crack at this but first I'd like to discuss #303 i.e. which is the most common / most covering main file(s) to run on toy data.)

requirements.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@vturrisi
Copy link
Owner Author

vturrisi commented Sep 12, 2022

I'm thinking about just using hydra to parse the args and merge appropriate default files. Then, we can just call some script like:
python main_pretrain.py --config-path scripts/pretrain/imagenet --config-name barlow {extra parameters to modify} +{extra parameters to add}. I'm not a bit fan or relying on hydra for everything (e.g. initializing objs) nor adhering to the default "configs" structure that hydra uses as it seems like a nightmare to manage many settings.

@turian
Copy link
Contributor

turian commented Sep 12, 2022

I'm thinking about just using hydra to parse the args and merge appropriate default files. Then, we can just call some script like:
python main_pretrain.py --config-path scripts/pretrain/imagenet --config-name barlow {extra parameters to modify} +{extra parameters to add}.

Agreed

I'm not a bit fan or relying on hydra for everything (e.g. initializing objs) nor adhering to the default "configs" structure that hydra uses as it seems like a nightmare to manage many settings.

Agreed

Nonetheless, you need to add hydra-core to setup.py and requirements.txt. hydra is the wrong package.

@vturrisi
Copy link
Owner Author

Yes, I was in a hurry to commit and put the wrong packaged by mistake. Gonna fix it tomorrow together with moving all config files for pretrain from bash to the new setting. Hopefully I'll also fix the tests soon enough and merge everything this week.

@turian
Copy link
Contributor

turian commented Sep 16, 2022

@vturrisi did CI/CD get disabled?

@vturrisi
Copy link
Owner Author

@turian it's just failing in the background for some reason. I'm still fixing the tests so it's gonna take a while.

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #300 (3ae3e26) into main (be3de80) will decrease coverage by 7.12%.
The diff coverage is 76.66%.

Additional details and impacted files
Flag Coverage Δ *Carryforward flag
cpu 81.42% <86.03%> (-0.25%) ⬇️
dali 40.06% <42.70%> (ø) Carriedforward from 8c5acfe

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
solo/args/umap.py 0.00% <0.00%> (ø)
solo/data/dali_dataloader.py 1.88% <1.85%> (+0.12%) ⬆️
solo/args/knn.py 20.00% <20.00%> (ø)
solo/utils/auto_umap.py 53.73% <70.37%> (-10.77%) ⬇️
solo/methods/swav.py 63.69% <75.00%> (-29.25%) ⬇️
solo/methods/vibcreg.py 75.36% <75.00%> (-24.64%) ⬇️
solo/methods/wmse.py 73.56% <75.00%> (-26.44%) ⬇️
solo/utils/checkpointer.py 67.56% <77.41%> (-12.15%) ⬇️
solo/methods/simclr.py 67.10% <77.77%> (-32.90%) ⬇️
solo/methods/simsiam.py 65.75% <77.77%> (-34.25%) ⬇️
... and 46 more

@vturrisi vturrisi merged commit 6f228fd into main Sep 27, 2022
@vturrisi vturrisi deleted the omegaconf branch September 27, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants