[optim] be explicit about CPU scalar tensor dtypes #111008

jon-chuang · 2023-10-11T00:39:15Z

Fixes #110940

cc @janeyx99 @crcrpar

pytorch-bot · 2023-10-11T00:39:20Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ No Failures

As of commit 18d6b92 with merge base b5dd37f ():
💚 Looks good so far! There are no failures yet. 💚

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

jon-chuang · 2023-10-11T00:47:39Z

@pytorchbot label "release notes: optim"

jon-chuang · 2023-10-11T05:45:59Z

test/optim/test_optim.py

@@ -736,7 +736,7 @@ def _test_derived_optimizers_varying_tensors(self, optimizer_with_kwargs, kwarg)
                    actual = mt_p_state[k]
                    self.assertEqual(st_p_state[k], actual, rtol=rtol, atol=atol)

-    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag):
+    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag, reduced_precision=False):


Don't think this tests sparseadam. I think it would have previously failed too.

this doesn’t test sparseadam nor lbfgs since they do not have foreach/multitensor impls.

moreover, it looks like sparseadam doesn’t have its step as a tensor like the majority of our optimizers. moving step to be a tensor is a separate change that includes adding warnings and tests (as it’s BC breaking) and I would recommend moving the sparseadam changes to a separate PR and only focusing on the foreach impls in this one.

If you’re up for it, RMSProp and Rprop also were accidentally left out of the step to Tensor migration (done by @mikaylagawarecki almost 2 yrs ago) and should be brought into the fold. 😄

this doesn’t test sparseadam nor lbfgs since they do not have foreach/multitensor impls.

moreover, it looks like sparseadam doesn’t have its step as a tensor like the majority of our optimizers. moving step to be a tensor is a separate change that includes adding warnings and tests (as it’s BC breaking) and I would recommend moving the sparseadam changes to a separate PR and only focusing on the foreach impls in this one.

If you’re up for it, Adadelta, RMSProp and Rprop also were accidentally left out of the step to Tensor migration (done by @mikaylagawarecki almost 2 yrs ago) and should be brought into the fold. 😄

Sure, I recall this is actually a high prio as well. Let me migrate Adadelta, RMSProp and Rprop next.

torch/optim/adagrad.py

janeyx99

thanks for taking this on!

janeyx99 · 2023-10-11T13:02:44Z

test/optim/test_optim.py

@@ -736,7 +736,7 @@ def _test_derived_optimizers_varying_tensors(self, optimizer_with_kwargs, kwarg)
                    actual = mt_p_state[k]
                    self.assertEqual(st_p_state[k], actual, rtol=rtol, atol=atol)

-    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag):
+    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag, reduced_precision=False):


this doesn’t test sparseadam nor lbfgs since they do not have foreach/multitensor impls.

moreover, it looks like sparseadam doesn’t have its step as a tensor like the majority of our optimizers. moving step to be a tensor is a separate change that includes adding warnings and tests (as it’s BC breaking) and I would recommend moving the sparseadam changes to a separate PR and only focusing on the foreach impls in this one.

If you’re up for it, RMSProp and Rprop also were accidentally left out of the step to Tensor migration (done by @mikaylagawarecki almost 2 yrs ago) and should be brought into the fold. 😄

janeyx99 · 2023-10-11T13:03:24Z

test/optim/test_optim.py

@@ -736,7 +736,7 @@ def _test_derived_optimizers_varying_tensors(self, optimizer_with_kwargs, kwarg)
                    actual = mt_p_state[k]
                    self.assertEqual(st_p_state[k], actual, rtol=rtol, atol=atol)

-    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag):
+    def _test_derived_optimizers(self, optimizer_pairs_with_flags, flag, reduced_precision=False):


this doesn’t test sparseadam nor lbfgs since they do not have foreach/multitensor impls.

moreover, it looks like sparseadam doesn’t have its step as a tensor like the majority of our optimizers. moving step to be a tensor is a separate change that includes adding warnings and tests (as it’s BC breaking) and I would recommend moving the sparseadam changes to a separate PR and only focusing on the foreach impls in this one.

If you’re up for it, Adadelta, RMSProp and Rprop also were accidentally left out of the step to Tensor migration (done by @mikaylagawarecki almost 2 yrs ago) and should be brought into the fold. 😄

test/optim/test_optim.py

janeyx99

hey--I am back. @jon-chuang How can I help get this over the finish line? Would want this in sooner rather than later!

jon-chuang · 2023-11-15T21:38:47Z

Hello @janeyx99, great! Apologies for the lack of movement on this. Let me prioritize this today.

jon-chuang · 2023-11-16T21:10:59Z

@janeyx99 should be as requested now

janeyx99

thanks! let's ship this thing :D

janeyx99 · 2023-11-21T19:22:09Z

@pytorchbot merge -r

pytorchmergebot · 2023-11-21T19:23:54Z

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

pytorchmergebot · 2023-11-21T19:23:58Z

Successfully rebased jon-chuang/explicit-float32-step-optim onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout jon-chuang/explicit-float32-step-optim && git pull --rebase)

pytorchmergebot · 2023-11-21T19:23:59Z

The merge job was canceled. If you believe this is a mistake, then you can re trigger it through pytorch-bot.

janeyx99 · 2023-11-21T19:29:10Z

@pytorchbot merge

pytorchmergebot · 2023-11-21T19:30:57Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Rely on built in bash conditionals for doing the if statement rather than relying on $? To avoid issues observed in #111008 (comment) Signed-off-by: Eli Uriegas <eliuriegasmeta.com> [ghstack-poisoned]

Rely on built in bash conditionals for doing the if statement rather than relying on $? To avoid issues observed in #111008 (comment) Signed-off-by: Eli Uriegas <eliuriegas@meta.com> Pull Request resolved: #114295 Approved by: https://github.com/huydhn, https://github.com/malfet

jon-chuang requested review from albanD and janeyx99 as code owners October 11, 2023 00:39

pytorch-bot bot added the release notes: optim label Oct 11, 2023

jon-chuang changed the title ~~[optim] be explicit about scalar tensor dtypes to respect contract~~ [optim] be explicit about scalar tensor dtypes Oct 11, 2023

pytorchbot added the open source label Oct 11, 2023

jon-chuang changed the title ~~[optim] be explicit about scalar tensor dtypes~~ [optim] be explicit about CPU scalar tensor dtypes Oct 11, 2023

jon-chuang commented Oct 11, 2023

View reviewed changes

cpuhrsch added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 11, 2023

vadimkantorov reviewed Oct 11, 2023

View reviewed changes

torch/optim/adagrad.py Outdated Show resolved Hide resolved

janeyx99 reviewed Oct 11, 2023

View reviewed changes

jon-chuang commented Oct 11, 2023

View reviewed changes

test/optim/test_optim.py Outdated Show resolved Hide resolved

jon-chuang mentioned this pull request Oct 15, 2023

fix(dynamo): Optimizer._init_group did not handle return value #110709

Closed

janeyx99 mentioned this pull request Oct 19, 2023

Tensors in different devices #111573

Closed

janeyx99 reviewed Nov 15, 2023

View reviewed changes

albanD removed their request for review November 16, 2023 14:25

janeyx99 approved these changes Nov 21, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 21, 2023

jon-chuang added 4 commits November 21, 2023 19:23

be explicit about float32 step

31e6869

minor

4ff4a24

move set into try

ac2d38c

done

18d6b92

pytorchmergebot force-pushed the jon-chuang/explicit-float32-step-optim branch from 7f17207 to 18d6b92 Compare November 21, 2023 19:24

pytorchmergebot added the merging label Nov 21, 2023

seemethere mentioned this pull request Nov 21, 2023

ci: Clean up logic for merge -r #114295

Closed

pytorchmergebot added Merged and removed merging labels Nov 21, 2023

pytorchmergebot closed this in 62de29d Nov 21, 2023

jon-chuang mentioned this pull request Dec 14, 2023

Dynamo'ing Rprop, RMSprop, and Adadelta misses incrementing step due to skipping _init_group #115679

Closed

[optim] be explicit about CPU scalar tensor dtypes #111008

[optim] be explicit about CPU scalar tensor dtypes #111008

Uh oh!

Conversation

jon-chuang commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 11, 2023 • 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/111008

✅ No Failures

Uh oh!

jon-chuang commented Oct 11, 2023

Uh oh!

jon-chuang Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

janeyx99 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

janeyx99 Oct 11, 2023 • 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!

jon-chuang Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janeyx99 left a comment

Choose a reason for hiding this comment

Uh oh!

janeyx99 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

janeyx99 Oct 11, 2023 • 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!

Uh oh!

Uh oh!

janeyx99 left a comment

Choose a reason for hiding this comment

Uh oh!

jon-chuang commented Nov 15, 2023

Uh oh!

jon-chuang commented Nov 16, 2023

Uh oh!

janeyx99 left a comment

Choose a reason for hiding this comment

Uh oh!

janeyx99 commented Nov 21, 2023

Uh oh!

pytorchmergebot commented Nov 21, 2023

Uh oh!

pytorchmergebot commented Nov 21, 2023

Uh oh!

pytorchmergebot commented Nov 21, 2023

Uh oh!

janeyx99 commented Nov 21, 2023

Uh oh!

pytorchmergebot commented Nov 21, 2023

Merge started

Uh oh!

Uh oh!

jon-chuang commented Oct 11, 2023 •

edited

Loading

pytorch-bot bot commented Oct 11, 2023 •

edited

Loading

janeyx99 Oct 11, 2023 •

edited

Loading

janeyx99 Oct 11, 2023 •

edited

Loading