Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactored scheduler callbacks (epoch-based/step-based warmup) #568

Merged
merged 44 commits into from
Jan 20, 2023
Merged

Refactored scheduler callbacks (epoch-based/step-based warmup) #568

merged 44 commits into from
Jan 20, 2023

Conversation

BloodAxe
Copy link
Contributor

@BloodAxe BloodAxe commented Dec 13, 2022 •
edited
Loading

Changes:

warmup_mode: linear_step will emit deprecation warning. But will continue to work without any changes.

Two new modes with explicit meaning:

  • warmup_mode: linear_batch_step
  • warmup_mode: linear_epoch_step

Per-batch warmup:

warmup_mode: linear_batch_step
num_warmup_steps: 100

Old Per-epoch warmup:

warmup_mode: linear_batch_step
num_warmup_epochs: 4

@BloodAxe
Refactored scheduler callbacks to have clear names & updated config f…
44d3d85 
…iles to respect new config names (Keep the old name for BC as well but emit a warning when used).
@dagshub
Copy link

dagshub bot commented Dec 13, 2022

Join the discussion on DagsHub!

shaydeci
shaydeci suggested changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@shaydeci shaydeci left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work (:
There are some comments inline., mostly discussing some edge cases and mildly (possible) inaccuracies.

Its important IMO we add unit tests for scheduling. Please see the different use cases I had in tests/unit_tests/lr_warmup_test.py.

src/super_gradients/training/utils/callbacks/callbacks.py Outdated Show resolved Hide resolved
src/super_gradients/training/utils/callbacks/callbacks.py Show resolved Hide resolved
src/super_gradients/training/utils/callbacks/callbacks.py Outdated Show resolved Hide resolved
src/super_gradients/training/utils/callbacks/callbacks.py Outdated Show resolved Hide resolved
src/super_gradients/training/utils/callbacks/callbacks.py Outdated Show resolved Hide resolved
shaydeci and others added 26 commits December 13, 2022 15:48
@shaydeci
Merge branch 'master' into feature/SG-525-step-based-warmup
25b2201
@BloodAxe
Merge branch 'master' into feature/SG-525-step-based-warmup
d584c23
@BloodAxe
New callbacks API
0513b72
@BloodAxe
New callbacks API
4427ac6
@BloodAxe
New callbacks API
f1b3ebf
@BloodAxe
Merge branch 'master' into feature/SG-581
03bfd97
@BloodAxe
Merge branch 'master' into feature/SG-581
4a7f6e7
@BloodAxe
Fix imports
727bd88
@BloodAxe
Uncomment sanity check
f067964
@BloodAxe
Fix callbacks order
23ed76b
@BloodAxe
Fix callbacks order
a4bf08d
@BloodAxe
Fix callbacks order
ee4e579
@BloodAxe
Fix wrong import
1e55891
@BloodAxe
Fix wrong import
e5a19cf
@BloodAxe
Merge branch 'master' into feature/SG-581
41f5b12
@BloodAxe
Call update_context
88c1e55
@BloodAxe
Call update_context
1cbef94
@BloodAxe
Fix on_train_batch_start timing
2893e2c
@BloodAxe
Merge branch 'master' into feature/SG-525-step-based-warmup
7f2f997
@BloodAxe
Merge branch 'feature/SG-581' into feature/SG-525-step-based-warmup
5d178c5
@BloodAxe
Batch-step warmup callback (WIP)
16efeae
@BloodAxe
Added docs clarifying mapping of new events to old Phase enum values
627f764
@BloodAxe
Instantiate context after _prep_for_test
6a2d46f
@BloodAxe
Merge branch 'master' into feature/SG-581
173f65c
@BloodAxe
Merge branch 'feature/SG-581' into feature/SG-525-step-based-warmup
1664ae3
@BloodAxe
Update callback implementation
d671b49
@BloodAxe
Merge branch 'master' into feature/SG-525-step-based-warmup
98432fa 
# Conflicts:
#	src/super_gradients/training/utils/callbacks/callbacks.py
#	tests/unit_tests/lr_warmup_test.py
@BloodAxe BloodAxe marked this pull request as ready for review January 16, 2023 13:27
@BloodAxe BloodAxe requested a review from ofrimasad as a code owner January 16, 2023 13:27
shaydeci
shaydeci suggested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@shaydeci shaydeci left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks awesome!
I just have one final suggestion for the unit test, see my comment there.
Please also update the docs in Trainer.train() (we have there the long list of training params docs), and shortly in default_training_hyperparams.yaml.

tests/unit_tests/lr_warmup_test.py Outdated Show resolved Hide resolved
shaydeci
shaydeci approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

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

LGTM

@BloodAxe BloodAxe merged commit 4016831 into master Jan 20, 2023
@BloodAxe BloodAxe deleted the feature/SG-525-step-based-warmup branch January 20, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaydeci shaydeci shaydeci approved these changes

@ofrimasad ofrimasad Awaiting requested review from ofrimasad ofrimasad is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BloodAxe @shaydeci