-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Feature/5555 add timm support #5898
Feature/5555 add timm support #5898
Conversation
Codecov Report
@@ Coverage Diff @@
## release/1.1.x #5898 +/- ##
=============================================
Coverage 94% 94%
=============================================
Files 134 134
Lines 10051 10051
=============================================
Hits 9407 9407
Misses 644 644 |
@potipot pls set the target 1.2 as there won't be any releases in 1.1 cc: @tchaton; see our suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not that familiar with the timm
library but I can see that it seems to be quite popular, so I think we should support it :]
Some other comments:
- this function needs to be updated:
https://github.com/PyTorchLightning/pytorch-lightning/blob/7f352cb69a8202e3f829419657597697ca5d99e2/pytorch_lightning/trainer/optimizers.py#L144 - the documentation needs to be updated:
https://github.com/PyTorchLightning/pytorch-lightning/blob/release/1.2-dev/docs/source/common/optimizers.rst - tests...
@@ -13,6 +13,7 @@ | |||
# limitations under the License. | |||
from pytorch_lightning.utilities import rank_zero_warn | |||
from pytorch_lightning.utilities.exceptions import MisconfigurationException | |||
from timm.scheduler.scheduler import Scheduler as TimmScheduler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As timm
is an extra feature, we need to make sure that the code can run even if the user does not have timm installed. Something like this should be done:
https://github.com/PyTorchLightning/pytorch-lightning/blob/0a50bb406fa41dfa6a0e2be52f531a9c81c87d00/pytorch_lightning/loggers/wandb.py#L30-L37
@@ -24,6 +24,7 @@ | |||
from pytorch_lightning.core.optimizer import LightningOptimizer | |||
from pytorch_lightning.utilities import rank_zero_warn | |||
from pytorch_lightning.utilities.exceptions import MisconfigurationException | |||
import timm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions. |
This pull request is going to be closed. Please feel free to reopen it create a new from the actual master. |
How is this request going? |
What does this PR do?
Fixes # (#5555) <- introduce support for timm scheduler and optimizer. Waiting for someone else I decided to try and do it myself. This is my first PR so looking forward for your comments.
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃