Introduce OptimizerInfos + add a test_errors #114178

janeyx99 · 2023-11-20T22:26:22Z

Introduce OptimizerInfos + use them to refactor out the error testing.

Why OptimizerInfos?

cleaner, easier way to test all configs of optimizers
would plug in well with devicetype to auto-enable tests for devices like MPS, meta
would allow for more granular testing. currently, lots of functionality is tested in _test_basic_cases and some of that should be broken down more.

What did I do for error testing?

I moved out some error cases from _test_basic_cases into a new test_errors parametrized test.
The new test has to live in TestOptimRenewed (bikeshedding welcome) because the parametrized tests need to take in device and dtype and hook correctly, and not all tests in TestOptim do that.
TestOptimRenewed also is migrating to the toplevel test/test_optim.py now because importing TestOptimRenewed does not work (because of test instantiation, TestOptimRenewed gets replaced with TestOptimRenewedDevice for CPU, CUDA, and whatever other device).

Is there any change in test coverage?

INCREASE: The error case where a single Parameter (vs a container of them) are passed in has now expanded to all optims instead of only LBFGS
DECREASE: Not much. The only thing is we no longer test two error cases for foreach=True AND foreach=False, which I think is redundant. (Highlighted in comments)

Possible but not urgent next step: test ALL possible error cases by going through all the constructors.

Stack from ghstack (oldest at bottom):

[ghstack-poisoned]

pytorch-bot · 2023-11-20T22:26:25Z

🔗 Helpful Links

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

📄 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 e6a3a73 with merge base 7f49603 ():
💚 Looks good so far! There are no failures yet. 💚

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

ghstack-source-id: b5742f3 Pull Request resolved: #114178

[ghstack-poisoned]

ghstack-source-id: 0ae75da Pull Request resolved: #114178

janeyx99 · 2023-11-23T03:06:49Z

test/optim/test_optim.py

@@ -1715,8 +1666,7 @@ def test_rmsprop(self):
            self._test_complex_optimizer(
                lambda param: RMSprop([param], maximize=True, foreach=foreach)
            )
-            with self.assertRaisesRegex(ValueError, "Invalid momentum value: -1.0"):
-                RMSprop(None, lr=1e-2, momentum=-1.0, foreach=foreach)


This is one place where coverage "decreased", since I no longer test this error case for BOTH foreach T and F. Which I believe is unnecessary anyway.

maybe this is unnecessary, but it's easy enough to do this:

class TestOptimRenewed: ... @onlyCPU @optims([optim for optim in optim_db if optim.optim_error_inputs_func is not None]) @parametrize("foreach", [False, True]) def test_errors(self, device, dtype, optim_info, foreach): ...

if there are error cases that are only expected to fail for e.g. foreach=True, then I'd consider allowing for this flexibility in ErrorOptimizerInput

Yea, thanks for the code pointer. I don't believe it is necessary though.

janeyx99 · 2023-11-23T03:07:07Z

test/optim/test_optim.py

@@ -1805,8 +1754,7 @@ def test_rprop(self):
                    [param], lr=0.001, maximize=True, foreach=foreach
                )
            )
-            with self.assertRaisesRegex(ValueError, "Invalid eta values: 1.0, 0.5"):
-                Rprop(None, lr=1e-2, etas=(1.0, 0.5), foreach=foreach)


And this is the other place where coverage "decreased", since I no longer test this error case for BOTH foreach T and F. Which I believe is unnecessary anyway.

Introduce OptimizerInfos + use them to refactor out the error testing. Why OptimizerInfos? - cleaner, easier way to test all configs of optimizers - would plug in well with devicetype to auto-enable tests for devices like MPS, meta - would allow for more granular testing. currently, lots of functionality is tested in `_test_basic_cases` and some of that should be broken down more. What did I do for error testing? - I moved out some error cases from `_test_basic_cases` into a new test_errors parametrized test. - The new test has to live in TestOptimRenewed (bikeshedding welcome) because the parametrized tests need to take in device and dtype and hook correctly, and not all tests in TestOptim do that. - TestOptimRenewed also is migrating to the toplevel test/test_optim.py now because importing TestOptimRenewed does not work (because of test instantiation, TestOptimRenewed gets replaced with TestOptimRenewedDevice for CPU, CUDA, and whatever other device). Is there any change in test coverage? - INCREASE: The error case where a single Parameter (vs a container of them) are passed in has now expanded to all optims instead of only LBFGS - DECREASE: Not much. The only thing is we no longer test two error cases for foreach=True AND foreach=False, which I think is redundant. (Highlighted in comments) [ghstack-poisoned]

ghstack-source-id: 2186040 Pull Request resolved: #114178

Introduce OptimizerInfos + use them to refactor out the error testing. Why OptimizerInfos? - cleaner, easier way to test all configs of optimizers - would plug in well with devicetype to auto-enable tests for devices like MPS, meta - would allow for more granular testing. currently, lots of functionality is tested in `_test_basic_cases` and some of that should be broken down more. What did I do for error testing? - I moved out some error cases from `_test_basic_cases` into a new test_errors parametrized test. - The new test has to live in TestOptimRenewed (bikeshedding welcome) because the parametrized tests need to take in device and dtype and hook correctly, and not all tests in TestOptim do that. - TestOptimRenewed also is migrating to the toplevel test/test_optim.py now because importing TestOptimRenewed does not work (because of test instantiation, TestOptimRenewed gets replaced with TestOptimRenewedDevice for CPU, CUDA, and whatever other device). Is there any change in test coverage? - INCREASE: The error case where a single Parameter (vs a container of them) are passed in has now expanded to all optims instead of only LBFGS - DECREASE: Not much. The only thing is we no longer test two error cases for foreach=True AND foreach=False, which I think is redundant. (Highlighted in comments) [ghstack-poisoned]

ghstack-source-id: 3c6401f Pull Request resolved: #114178

Introduce OptimizerInfos + use them to refactor out the error testing. Why OptimizerInfos? - cleaner, easier way to test all configs of optimizers - would plug in well with devicetype to auto-enable tests for devices like MPS, meta - would allow for more granular testing. currently, lots of functionality is tested in `_test_basic_cases` and some of that should be broken down more. What did I do for error testing? - I moved out some error cases from `_test_basic_cases` into a new test_errors parametrized test. - The new test has to live in TestOptimRenewed (bikeshedding welcome) because the parametrized tests need to take in device and dtype and hook correctly, and not all tests in TestOptim do that. - TestOptimRenewed also is migrating to the toplevel test/test_optim.py now because importing TestOptimRenewed does not work (because of test instantiation, TestOptimRenewed gets replaced with TestOptimRenewedDevice for CPU, CUDA, and whatever other device). Is there any change in test coverage? - INCREASE: The error case where a single Parameter (vs a container of them) are passed in has now expanded to all optims instead of only LBFGS - DECREASE: Not much. The only thing is we no longer test two error cases for foreach=True AND foreach=False, which I think is redundant. (Highlighted in comments) Possible but not urgent next step: test ALL possible error cases by going through all the constructors. [ghstack-poisoned]

albanD

Sounds good!

albanD · 2023-12-05T20:14:54Z

torch/testing/_internal/common_optimizers.py

+        self,
+        optimizer_error_input,
+        *,
+        error_on=OptimizerErrorEnum.CONSTRUCTION_ERROR,


Do you expect these to be default being used? If not we can just remove the default values here.

Yes, most of them are on construction

Is it true as well for the err type and regex below?

albanD · 2023-12-05T20:18:09Z

torch/testing/_internal/common_optimizers.py

+        optim_inputs_func,
+        # Implementation specific kwargs the optimizer supports, e.g., fused, foreach, differentiable
+        # We consider capturable to be a base constructor flag since it is implemented across the board.
+        supported_impls: Tuple[str] = ("foreach", "differentiable"),


It would be great for the comment above to mention every valid value for this flag.

wait it does do that

"kwargs the optimizer supports, e.g., fused, foreach, differentiable"
Is not the same as
"kwargs the optimizer supports, it must be a subset of fused, foreach, differentiable"

torch/testing/_internal/common_optimizers.py

janeyx99 · 2023-12-05T20:30:36Z

@pytorchbot merge

let's see if the 3 days ago is recent enough

pytorchmergebot · 2023-12-05T20:32:47Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

janeyx99 · 2023-12-05T20:37:08Z

@pytorchbot merge

pytorchmergebot · 2023-12-05T20:39:04Z

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

Introduce OptimizerInfos + use them to refactor out the error testing. Why OptimizerInfos? - cleaner, easier way to test all configs of optimizers - would plug in well with devicetype to auto-enable tests for devices like MPS, meta - would allow for more granular testing. currently, lots of functionality is tested in `_test_basic_cases` and some of that should be broken down more. What did I do for error testing? - I moved out some error cases from `_test_basic_cases` into a new test_errors parametrized test. - The new test has to live in TestOptimRenewed (bikeshedding welcome) because the parametrized tests need to take in device and dtype and hook correctly, and not all tests in TestOptim do that. - TestOptimRenewed also is migrating to the toplevel test/test_optim.py now because importing TestOptimRenewed does not work (because of test instantiation, TestOptimRenewed gets replaced with TestOptimRenewedDevice for CPU, CUDA, and whatever other device). Is there any change in test coverage? - INCREASE: The error case where a single Parameter (vs a container of them) are passed in has now expanded to all optims instead of only LBFGS - DECREASE: Not much. The only thing is we no longer test two error cases for foreach=True AND foreach=False, which I think is redundant. (Highlighted in comments) Possible but not urgent next step: test ALL possible error cases by going through all the constructors. Pull Request resolved: pytorch#114178 Approved by: https://github.com/albanD

[init] Add OptimizerInfos

a835403

[ghstack-poisoned]

janeyx99 added a commit that referenced this pull request Nov 20, 2023

[init] Add OptimizerInfos

6c99112

ghstack-source-id: b5742f3 Pull Request resolved: #114178

Update on "[init] Add OptimizerInfos"

71b83a8

[ghstack-poisoned]

Update on "[init] Add OptimizerInfos"

68a8292

[ghstack-poisoned]

Update on "[init] Add OptimizerInfos"

82b3214

[ghstack-poisoned]

janeyx99 mentioned this pull request Nov 22, 2023

[BE][SparseAdam] cleaner way to verify no sparse params #114425

Closed

Update on "[init] Add OptimizerInfos"

e33703c

[ghstack-poisoned]

janeyx99 added a commit that referenced this pull request Nov 23, 2023

Introduce OptimizerInfos + add a test_errors

31fb7dc

ghstack-source-id: 0ae75da Pull Request resolved: #114178

janeyx99 changed the title ~~[init] Add OptimizerInfos~~ Introduce OptimizerInfos + add a test_errors Nov 23, 2023

janeyx99 commented Nov 23, 2023

View reviewed changes

janeyx99 added a commit that referenced this pull request Nov 28, 2023

Introduce OptimizerInfos + add a test_errors

f545e4a

ghstack-source-id: 2186040 Pull Request resolved: #114178

janeyx99 added a commit that referenced this pull request Nov 29, 2023

Introduce OptimizerInfos + add a test_errors

1913660

ghstack-source-id: 3c6401f Pull Request resolved: #114178

janeyx99 mentioned this pull request Nov 29, 2023

Move test_multi_tensor_optimizers to use OptimizerInfos #114797

Closed

janeyx99 marked this pull request as ready for review November 29, 2023 21:10

This was referenced Nov 29, 2023

[BE] remove redundant _test_derived_optimizers by migrating more to OptimizerInfo #114802

Closed

Migrate test_peak_mem_multi_tensor_optimizers to OptimizerInfo #115023

Closed

janeyx99 mentioned this pull request Dec 3, 2023

Move tests depending on listed configs to OptimizerInfo #115025

Closed

albanD approved these changes Dec 5, 2023

View reviewed changes

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

pytorchmergebot added the merging label Dec 5, 2023

pytorchmergebot removed the merging label Dec 5, 2023

janeyx99 added the topic: not user facing topic category label Dec 5, 2023

pytorchmergebot added the merging label Dec 5, 2023

pytorchmergebot added the Merged label Dec 5, 2023

pytorchmergebot closed this in d78fe03 Dec 5, 2023

pytorchmergebot removed the merging label Dec 5, 2023

janeyx99 mentioned this pull request Dec 6, 2023

Move step is noop tests #115299

Closed

facebook-github-bot deleted the gh/janeyx99/108/head branch December 9, 2023 15:27

Introduce OptimizerInfos + add a test_errors #114178

Introduce OptimizerInfos + add a test_errors #114178

Uh oh!

Conversation

janeyx99 commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 20, 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/114178

✅ No Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbschlosser Nov 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albanD 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!

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!

Uh oh!

janeyx99 commented Dec 5, 2023

Uh oh!

pytorchmergebot commented Dec 5, 2023

Merge failed

Uh oh!

janeyx99 commented Dec 5, 2023

Uh oh!

pytorchmergebot commented Dec 5, 2023

Merge started

Uh oh!

Uh oh!

janeyx99 commented Nov 20, 2023 •

edited

Loading

pytorch-bot bot commented Nov 20, 2023 •

edited

Loading

jbschlosser Nov 27, 2023 •

edited

Loading