Skip to content

Allow passing a pre-initialized OptimWrapper to Learner#3843

Merged
jph00 merged 3 commits into
fastai:masterfrom
warner-benjamin:initialized_OptimWrapper
Dec 13, 2022
Merged

Allow passing a pre-initialized OptimWrapper to Learner#3843
jph00 merged 3 commits into
fastai:masterfrom
warner-benjamin:initialized_OptimWrapper

Conversation

@warner-benjamin

Copy link
Copy Markdown
Collaborator

The OptimWrapper documentation suggests that using a pre-defined optimizer with Learner's opt_func argument should work:

Or if you already have an existing one, pass in only opt:

opt = torch.optim.SGD([tensor([1,2,3])], lr=1e-2)
opt_func = OptimWrapper(opt=opt)

however, following this example and passing opt_func to Learner.opt_func results in an error, as Learner.create_opt only accepts callable methods and not initialized objects.

This PR modifies Learner to accept a pre-initialized OptimWrapper to the opt_func argument by modifying Learner.create_opt to set a pre-initialized OptimWrapper to Learner.opt. To mimic the current behavior of always initializing an optimizer from scratch, create_opt will call clear_state on a pre-initialized OptimWrapper.

This allows a straightforward method of using custom parameter groups in a PyTorch style optimizer:

param_groups = custom_param_group_method(model)
opt = OptimWrapper(opt=optim.AdamW(param_groups, ...))
learn = Learner(dls, model, opt_func=opt, ...)

To prevent a pre-initialized optimizer's state from being cleared on the first fit, users can use the current behavior of passing the optimizer directly to Learner.opt. However, in this case manually calling Learner.fit(..., reset_opt=True) will cause an error. Instead of using reset_opt, users will need to redefine the optimizer.

This PR also updates some of the existing Learner docments to better match the docment style and more accurately describe the argument behavior.

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@warner-benjamin

Copy link
Copy Markdown
Collaborator Author

When I run the tests locally, CI error does not occur. I also did not change anything related to default callbacks. Investigating.

@warner-benjamin

Copy link
Copy Markdown
Collaborator Author

@jph00 The CI failure appears to be random and due to a change made in the development branch of nbdev, which the fastai CI currently uses.

My updated passed the CI, but would have been safe to merge even if it did not, as I tested it with nbdev 2.3.9 and it passed every time.

@jph00 jph00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great @warner-benjamin . My only request is to avoid restricting types more than necessary. You've added some restrictions which are too tight -- and it's also made me realise some existing ones are also too tight!

Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
Comment thread fastai/learner.py Outdated
@warner-benjamin

Copy link
Copy Markdown
Collaborator Author

Will change any of the type hints to what you want. I tried to follow the convention set by #3639 and attempted to add some clarity. I added a few comments and questions on the code review.

@jph00

jph00 commented Dec 9, 2022

Copy link
Copy Markdown
Member

Many thanks @warner-benjamin -- let me know if you have any outstanding questions.

@jph00

jph00 commented Dec 12, 2022

Copy link
Copy Markdown
Member

Let me know which would be easier/preferred - I can merge this PR as is, and make the type changes on my end, or I you can change them in this PR.

@warner-benjamin

Copy link
Copy Markdown
Collaborator Author

I have most of them done already, just need some clarification on the Callback|list change.

@jph00

jph00 commented Dec 13, 2022

Copy link
Copy Markdown
Member

Many thanks!

@jph00 jph00 merged commit d72d758 into fastai:master Dec 13, 2022
@jph00

jph00 commented Dec 13, 2022

Copy link
Copy Markdown
Member

I'm replacing list in all types I can find with MutableSequence now FYI and will push shortly.

@warner-benjamin warner-benjamin deleted the initialized_OptimWrapper branch February 27, 2023 05:08
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.

2 participants