Fix backbone freezing with icevision models

[ethanwharris]

What does this PR do?

Fixes #1080

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests? [not needed for typos/docs]
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

• Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

[codecov]

Codecov Report

Merging #1163 (1e49be9) into master (4b12e02) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   89.24%   89.26%   +0.01%     
==========================================
  Files         286      286              
  Lines       13048    13048              
==========================================
+ Hits        11645    11647       +2     
+ Misses       1403     1401       -2     

Flag	Coverage Δ
unittests	89.26% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
flash/image/detection/model.py	95.45% <ø> (ø)
flash/core/integrations/icevision/backbones.py	93.33% <100.00%> (ø)
flash/image/detection/backbones.py	77.50% <0.00%> (+5.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b12e02...1e49be9. Read the comment docs.

[ligaz]

@ethanwharris Can you elaborate why the default learning rate has been changed here? Thanks.

[ethanwharris]

Hey, @ligaz sorry just an accidental change 😃 Woud you prefer the old default? Or something else? Please feel free to open a PR to revert / update it

[ligaz]

It was a general question what's the reason behind this change.

We are defaulting the optimizer to Adam so my suggestion is to use the default learning rate for it. From the PyTorch docs here:

learning rate (default: 1e-3)

[ethanwharris]

I think that makes sense! I wonder if there's a way we could just not provide the LR to the optimizer constructor if it is None? That way if you don't override the LR you would always just get the default for your chosen optimizer. What do you think?

[ligaz]

I was thinking about the same. It turns out that it defaults to the code in the core Task class which uses 5e-5 as a default value. This is the relevant code.

I'm personally not sure how we should proceed 😄

[ethanwharris]

It looks like we could just default the learning_rate to None, and then adjust the logic here: https://github.com/PyTorchLightning/lightning-flash/blob/4c624823d7992aa454b9f064d2714cae9b4a8893/flash/core/model.py#L463

If learning rate is None then the optimizer_kwargs should just be an empty dict. That would do it I think. Let me know if you'd like to try it otherwise I can take it 😃

[ligaz]

Feel free to take it. Thanks for your support 🙏