Move schedulers to training from modules. #1801

krammnic · 2024-10-10T15:50:02Z

Context

What is the purpose of this PR? Is it to

add a new feature
fix a bug
update tests and/or documentation
other (please add here)

Please link to any issues this PR addresses.

Changelog

What are the changes made in this PR?
#1587

Test plan

Please make sure to do each of the following if applicable to your PR. If you're unsure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.

run pre-commit hooks and linters (make sure you've first installed via pre-commit install)
add unit tests for any new functionality
update docstrings for any new or updated methods or classes
run unit tests via pytest tests
run recipe tests via pytest tests -m integration_test
manually run any new or modified recipes with sufficient proof of correctness
include relevant commands and any other artifacts in this summary (pastes of loss curves, eval results, etc.)

UX

If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Here is a docstring example
and a tutorial example

I did not change any public API
I have added an example to docs or docstrings

pytorch-bot · 2024-10-10T15:50:06Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1801

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit a3e2ffe with merge base 5de5001 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

krammnic · 2024-10-10T15:51:27Z

@joecummings Require review

krammnic · 2024-10-11T09:53:19Z

Should include backward capability, no merge right now!

krammnic · 2024-10-11T10:03:44Z

@ebsmothers Fixed backward capability

ebsmothers

Can you also update the usage here and here?

ebsmothers · 2024-10-11T10:53:19Z

docs/source/api_ref_training.rst

    create_optim_in_bwd_wrapper
    register_optim_in_bwd_hooks

 .. _metric_logging_label:


Probably need to update this (otherwise it's duplicated below)

@krammnic in response to your question

Suggested change

.. _metric_logging_label:

.. _lr_scheduler_label:

ebsmothers

Oh actually I think the new version under torchtune/training/lr_schedulers.py is missing from the PR.

Also separately if you are running into issues with your local pre-commit not lining up with the CI in lint job, you can mimic these steps in our CI job more directly on your local machine to try to get the same setup (though I'd think that the usual flow in our CONTRIBUTING.md around pre-commit install should be sufficient)

krammnic · 2024-10-11T11:14:55Z

Yeah, will do some fix soon. I have broken something with my commits

krammnic · 2024-10-11T11:37:22Z

Oh actually I think the new version under torchtune/training/lr_schedulers.py is missing from the PR.

Also separately if you are running into issues with your local pre-commit not lining up with the CI in lint job, you can mimic these steps in our CI job more directly on your local machine to try to get the same setup (though I'd think that the usual flow in our CONTRIBUTING.md around pre-commit install should be sufficient)

Fixed. Yeah, probably I will mimic CI using instructions that you provided, because the way that is provided in CONTRIBUTING.md is pretty inconvenient for me.

ebsmothers · 2024-10-11T12:01:40Z

Oh actually I think the new version under torchtune/training/lr_schedulers.py is missing from the PR.
Also separately if you are running into issues with your local pre-commit not lining up with the CI in lint job, you can mimic these steps in our CI job more directly on your local machine to try to get the same setup (though I'd think that the usual flow in our CONTRIBUTING.md around pre-commit install should be sufficient)

Fixed. Yeah, probably I will mimic CI using instructions that you provided, because the way that is provided in CONTRIBUTING.md is pretty inconvenient for me.

Sounds good. By the way if CONTRIBUTING.md is not currently clear or giving the right setup you need, we should update it. We set it up a while back and many devs don't use it much day-to-day. But as someone onboarding to the project recently if there are certain areas where you're feeling friction let us know and we can hopefully make the process smoother.

krammnic · 2024-10-11T13:34:11Z

@ebsmothers Which label to put for schedulers section in docs? Will fix lint now and probably except this point it will be ready to merge.

ebsmothers · 2024-10-11T13:41:27Z

@ebsmothers Which label to put for schedulers section in docs? Will fix lint now and probably except this point it will be ready to merge.

Updated my inline comment here with the suggestion

krammnic · 2024-10-11T14:32:34Z

Tag is fixed.

krammnic · 2024-10-11T16:37:43Z

Yeah, docs is built. Now let me fix lint and it will be ready to merge

codecov-commenter · 2024-10-11T20:17:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.01%. Comparing base (54673b7) to head (0269dd0).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1801      +/-   ##
==========================================
+ Coverage   67.05%   69.01%   +1.95%     
==========================================
  Files         305      306       +1     
  Lines       15937    16024      +87     
==========================================
+ Hits        10687    11059     +372     
+ Misses       5250     4965     -285

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

krammnic · 2024-10-13T17:58:45Z

Last lint fix. Should be fine right now

RdoubleA · 2024-10-13T18:51:47Z

docs/source/api_ref_training.rst

+
+    get_cosine_schedule_with_warmup
+
+.. lr_scheduler_label:


this should swap with metric_logging_label above, and should have an underscore before: .. _lr_scheduler_label:. otherwise it gets interpreted as a comment (sphinx, I know)

RdoubleA · 2024-10-13T18:53:44Z

docs/source/api_ref_training.rst

 .. _metric_logging_label:

+Schedulers
+-----------------


the underline needs to match the length of the "Schedulers" title

krammnic · 2024-10-13T20:19:05Z

Finnaly! Can be merged...

krammnic · 2024-10-13T21:32:31Z

@RdoubleA As you already reviewed this maybe we can merge?

ebsmothers · 2024-10-13T23:57:42Z

docs/source/api_ref_training.rst

+
+    get_cosine_schedule_with_warmup
+
+.. _lr_scheduler_label:


Is this right? Seems to me like lr scheduler label and metric logging label need to be swapped

Are you speaking about 2 sections?(swap them) I'm not sure why you might want to swap specific labels

Currently the _lr_scheduler_label is just above the "Metric Logging" section, while the _metric_logging_label is just above the "Schedulers" section. I believe it should be the other way around. (I don't care too much whether we swap labels or section orders, but one of them needs to be swapped. Otherwise labels are pointing to the wrong sections, no?)

I seen now what was your idea, fixed!

joecummings · 2024-10-14T13:43:44Z

@RdoubleA and/or @ebsmothers can one of you sign off on this?

ebsmothers · 2024-10-14T13:53:21Z

@RdoubleA and/or @ebsmothers can one of you sign off on this?

@joecummings I have one more comment on the file api_ref_training.rst. After that I am good to merge this

ebsmothers

Thanks for making this change! Once CI is green I think we're good to merge here

krammnic and others added 3 commits October 10, 2024 18:43

move schedulers to training

cb69b5f

Add get_cosine_schedule_with_warmup in api_ref_training.rst

4b65d99

Remove get_cosine_schedule_with_warmup from api_ref_modules.rst

ed686fc

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2024

SalmanMohammadi requested a review from joecummings October 10, 2024 16:03

krammnic added 2 commits October 11, 2024 06:02

Add to modules due to backward cap reasons lr_schedulers.py

f0f0230

Return cosine schedulers due to backward cap reasons __init__.py

01e8ba2

ebsmothers reviewed Oct 11, 2024

View reviewed changes

krammnic added 3 commits October 11, 2024 14:21

Update add usage in knowledge_distillation_single_device.yaml

952712e

Fix ref in memory_optimizations.rst

a0bf430

Fix and return lr_schedulers.py

35d9bf1

Rename tag api_ref_training.rst

21b4cf7

Fix tag api_ref_training.rst

754d93c

krammnic added 3 commits October 11, 2024 15:31

fix lint lr_schedulers.py

ab1665b

fix lint lr_schedulers.py

fbf908e

fix lint lr_schedulers.py

0269dd0

fix lint lr_schedulers.py

a9f66e4

krammnic changed the title ~~[WIP] Move schedulers to training from modules.~~ Move schedulers to training from modules. Oct 13, 2024

fix lint __init__.py

afe5651

RdoubleA reviewed Oct 13, 2024

View reviewed changes

Fix docs api_ref_training.rst

7d605af

ebsmothers reviewed Oct 13, 2024

View reviewed changes

swap labels api_ref_training.rst

a3e2ffe

ebsmothers approved these changes Oct 14, 2024

View reviewed changes

ebsmothers merged commit 78ceee6 into meta-pytorch:main Oct 14, 2024
17 checks passed

mori360 pushed a commit to mori360/torchtune that referenced this pull request Oct 14, 2024

Move schedulers to training from modules. (meta-pytorch#1801)

760d2bb

joecummings mentioned this pull request Oct 15, 2024

Move cosine LR scheduler with warmup from modules/ to training/ #1587

Closed

Move schedulers to training from modules. #1801

Move schedulers to training from modules. #1801

Uh oh!

Conversation

krammnic commented Oct 10, 2024

Context

Changelog

Test plan

UX

Uh oh!

pytorch-bot bot commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1801

✅ No Failures

Uh oh!

krammnic commented Oct 10, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

ebsmothers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebsmothers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

ebsmothers commented Oct 11, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

ebsmothers commented Oct 11, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

krammnic commented Oct 11, 2024

Uh oh!

codecov-commenter commented Oct 11, 2024

Codecov Report

Uh oh!

krammnic commented Oct 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krammnic commented Oct 13, 2024

Uh oh!

krammnic commented Oct 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krammnic Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joecummings commented Oct 14, 2024

Uh oh!

ebsmothers commented Oct 14, 2024

Uh oh!

ebsmothers left a comment

Choose a reason for hiding this comment

pytorch-bot bot commented Oct 10, 2024 •

edited

Loading

ebsmothers left a comment •

edited

Loading

krammnic Oct 14, 2024 •

edited

Loading