search

vturrisi / solo-learn

solo-learn: a library of self-supervised methods for visual representation learning powered by Pytorch Lightning
MIT License
1.39k stars 181 forks source link

Move from Argparse to Omegaconf #300

Closed vturrisi closed 1 year ago

vturrisi commented 1 year ago

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

turian commented 1 year ago

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

turian commented 1 year ago

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

vturrisi commented 1 year ago

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

turian commented 1 year ago

@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:

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?

turian commented 1 year ago

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.)

vturrisi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@vturrisi did CI/CD get disabled?

vturrisi commented 1 year ago

@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[bot] commented 1 year ago

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%)` | :arrow_down: | | | dali | `40.06% <42.70%> (ø)` | | Carriedforward from [8c5acfe](https://codecov.io/gh/vturrisi/solo-learn/commit/8c5acfeb453efe2e02eec8fef677bbb4b1554d08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi) | *This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi) to find out more. | [Impacted Files](https://codecov.io/gh/vturrisi/solo-learn/pull/300?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi) | Coverage Δ | | |---|---|---| | [solo/args/umap.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9hcmdzL3VtYXAucHk=) | `0.00% <0.00%> (ø)` | | | [solo/data/dali\_dataloader.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9kYXRhL2RhbGlfZGF0YWxvYWRlci5weQ==) | `1.88% <1.85%> (+0.12%)` | :arrow_up: | | [solo/args/knn.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9hcmdzL2tubi5weQ==) | `20.00% <20.00%> (ø)` | | | [solo/utils/auto\_umap.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby91dGlscy9hdXRvX3VtYXAucHk=) | `53.73% <70.37%> (-10.77%)` | :arrow_down: | | [solo/methods/swav.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9tZXRob2RzL3N3YXYucHk=) | `63.69% <75.00%> (-29.25%)` | :arrow_down: | | [solo/methods/vibcreg.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9tZXRob2RzL3ZpYmNyZWcucHk=) | `75.36% <75.00%> (-24.64%)` | :arrow_down: | | [solo/methods/wmse.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9tZXRob2RzL3dtc2UucHk=) | `73.56% <75.00%> (-26.44%)` | :arrow_down: | | [solo/utils/checkpointer.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby91dGlscy9jaGVja3BvaW50ZXIucHk=) | `67.56% <77.41%> (-12.15%)` | :arrow_down: | | [solo/methods/simclr.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9tZXRob2RzL3NpbWNsci5weQ==) | `67.10% <77.77%> (-32.90%)` | :arrow_down: | | [solo/methods/simsiam.py](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi#diff-c29sby9tZXRob2RzL3NpbXNpYW0ucHk=) | `65.75% <77.77%> (-34.25%)` | :arrow_down: | | ... and [46 more](https://codecov.io/gh/vturrisi/solo-learn/pull/300/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Victor+Turrisi) | |