[feat] OSS: Sync all attributes

[blefaudeux]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
• Did you read the contributor guideline?
• Did you make sure to update the docs?
• Did you write any new necessary tests? (lr scheduler test)

What does this PR do?

Fixes #66

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[blefaudeux]

cc @mannatsingh

[msbaines]

Please add a test.

Does this fix a bug? The only attribute that is common to all optimizers is learning rate.

[blefaudeux]

Please add a test.

Does this fix a bug? The only attribute that is common to all optimizers is learning rate.

see for instance https://github.com/facebookresearch/ClassyVision/blob/master/classy_vision/optim/classy_optimizer.py#L162
in classy the user is free to pass any callback that he/she wants to adjust any setting over time, can be LR but can also be something else, the "lr" key is not specific as you can see in the code snippet. Not syncing them means silent failure for the end user when using oss, not very nice.

I'll add a non-lr unit test, good point, I was a bit lazy and thought that lr could be enough but that's wrong

[msbaines]

Please add a test.
Does this fix a bug? The only attribute that is common to all optimizers is learning rate.

see for instance https://github.com/facebookresearch/ClassyVision/blob/master/classy_vision/optim/classy_optimizer.py#L162
in classy the user is free to pass any callback that he/she wants to adjust any setting over time, can be LR but can also be something else, the "lr" key is not specific as you can see in the code snippet. Not syncing them means silent failure for the end user when using oss, not very nice.

I'll add a non-lr unit test, good point, I was a bit lazy and thought that lr could be enough but that's wrong

It would be nice to capture some of this discussion in the commit message or via a code comment.

[blefaudeux]

Please add a test.
Does this fix a bug? The only attribute that is common to all optimizers is learning rate.

see for instance https://github.com/facebookresearch/ClassyVision/blob/master/classy_vision/optim/classy_optimizer.py#L162
in classy the user is free to pass any callback that he/she wants to adjust any setting over time, can be LR but can also be something else, the "lr" key is not specific as you can see in the code snippet. Not syncing them means silent failure for the end user when using oss, not very nice.
I'll add a non-lr unit test, good point, I was a bit lazy and thought that lr could be enough but that's wrong

It would be nice to capture some of this discussion in the commit message or via a code comment.

ah, the intent was to have this documented in the linked issue (#66), but if it's easier I can elaborate more in the commit messages or PR ? Alrighty, learning by doing, there are some hidden assumptions

[msbaines]

Please add a test.
Does this fix a bug? The only attribute that is common to all optimizers is learning rate.

see for instance https://github.com/facebookresearch/ClassyVision/blob/master/classy_vision/optim/classy_optimizer.py#L162
in classy the user is free to pass any callback that he/she wants to adjust any setting over time, can be LR but can also be something else, the "lr" key is not specific as you can see in the code snippet. Not syncing them means silent failure for the end user when using oss, not very nice.
I'll add a non-lr unit test, good point, I was a bit lazy and thought that lr could be enough but that's wrong

It would be nice to capture some of this discussion in the commit message or via a code comment.

ah, the intent was to have this documented in the linked issue (#66), but if it's easier I can elaborate more in the commit messages or PR ? Alrighty, learning by doing, there are some hidden assumptions

The linked issue is two levels of indirection from the code. It's nice to have a useful summary in the commit message. The commit message is one level of indirection away from the code. When maintaining code, it is common to git log or git blame a file. Understanding the rationale for a change from the code or the commit message is very handy. You have all the information available in the log. Reading the issue requires you to move your attention away from the log.

https://chris.beams.io/posts/git-commit/#why-not-how

[msbaines]

Please add more context to the commit message before landing.