Skip to content

[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimizer#34957

Closed
anjali411 wants to merge 11 commits intopytorch:masterfrom
anjali411:merge_optim
Closed

[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimizer#34957
anjali411 wants to merge 11 commits intopytorch:masterfrom
anjali411:merge_optim

Conversation

@anjali411
Copy link
Copy Markdown
Contributor

@anjali411 anjali411 commented Mar 18, 2020 •
edited
Loading

  1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
  2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
  3. BC-compatibility serialization test for LBFGS
  4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
  5. Made defaults_ optional argument in all optimizers except SGD

TODO: add BC-breaking notes for this PR

@override-unit-failures

@anjali411 anjali411 added the module: cpp Related to C++ API label Mar 18, 2020
@anjali411 anjali411 requested a review from yf225 March 18, 2020 16:23
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented Mar 18, 2020 •
edited
Loading

💊 CircleCI build failures summary and remediations

As of commit f8297f9 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_xenial_py3_6_gcc5_4_test (1/1)

Step: "Test" (full log | pattern match details) <confirmed not flaky by 2 failures>

Mar 26 22:31:08 [E request_callback_impl.cpp:94] Received error while processing request type 2: PickleError: ScriptModules cannot be deepcopied using copy.deepcopy or saved using torch.save. Mixed serialization of script and non-script modules is not supported. For purely script modules use my_script_module.save() instead. 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(86): serialize 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(135): serialize 
Mar 26 22:31:08  
Mar 26 22:31:08 [E request_callback_impl.cpp:94] Received error while processing request type 2: PickleError: ScriptModules cannot be deepcopied using copy.deepcopy or saved using torch.save. Mixed serialization of script and non-script modules is not supported. For purely script modules use my_script_module.save(<filename>) instead. 
Mar 26 22:31:08  
Mar 26 22:31:08 At: 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/jit/__init__.py(1779): __getstate__ 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(86): serialize 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(135): serialize 
Mar 26 22:31:08  
Mar 26 22:31:08 [E request_callback_impl.cpp:94] Received error while processing request type 2: PickleError: ScriptModules cannot be deepcopied using copy.deepcopy or saved using torch.save. Mixed serialization of script and non-script modules is not supported. For purely script modules use my_script_module.save(<filename>) instead. 
Mar 26 22:31:08  
Mar 26 22:31:08 At: 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/jit/__init__.py(1779): __getstate__ 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(86): serialize 
Mar 26 22:31:08   /opt/conda/lib/python3.6/site-packages/torch/distributed/rpc/internal.py(135): serialize 
Mar 26 22:31:08  
Mar 26 22:31:08 ok (1.118s) 
Mar 26 22:31:09   test_unexepected_kwarg_is_specified (__main__.JitRpcTestWithSpawn) ... ok (1.117s) 
Mar 26 22:31:10   test_user_rrefs_confirmed (__main__.JitRpcTestWithSpawn) ... ok (1.118s) 
Mar 26 22:31:11   test_user_rrefs_confirmed_remote (__main__.JitRpcTestWithSpawn) ... ok (1.118s)
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 177 times.

yf225
yf225 reviewed Mar 18, 2020
View reviewed changes
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/optim.cpp Outdated
Comment thread test/cpp/api/serialize.cpp Outdated
Comment thread test/cpp/api/serialize.cpp Outdated
Comment thread torch/csrc/api/include/torch/optim/optimizer.h Outdated
Comment thread torch/csrc/api/src/optim/optimizer.cpp Outdated
facebook-github-bot
facebook-github-bot reviewed Mar 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@anjali411 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yf225 yf225 added the module: bc-breaking Related to a BC-breaking change label Mar 18, 2020
yf225
yf225 approved these changes Mar 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yf225 yf225 left a comment

Choose a reason for hiding this comment

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

Thanks so much for the awesome work!

@anjali411 anjali411 closed this Mar 18, 2020
@anjali411 anjali411 reopened this Mar 18, 2020
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Mar 19, 2020

@anjali411 merged this pull request in b8e043a.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Mar 19, 2020

This broke Windows

[3111/3267] C:\Users\circleci\project\build\win_tmp\bin\sccache-cl.exe   /TP -DAT_PARALLEL_OPENMP=1 -DIDEEP_USE_MKL -DMAGMA_V2 -DMINIZ_DISABLE_ZIP_READER_CRC32_CHECKS -DONNX_ML=1 -DONNX_NAMESPACE=onnx_torch -DTH_BLAS_MKL -DUSE_CUDA -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE=1 -D_OPENMP_NOFORCE_MANIFEST -Iaten\src -I..\aten\src -I. -I..\ -I..\cmake\..\third_party\benchmark\include -Icaffe2\contrib\aten -I..\third_party\onnx -Ithird_party\onnx -I..\third_party\foxi -Ithird_party\foxi -I..\caffe2\..\torch\..\aten\src -Icaffe2\..\aten\src -Icaffe2\..\aten\src\ATen -I..\caffe2\..\torch\csrc\api -I..\caffe2\..\torch\csrc\api\include -I..\c10\.. -Ithird_party\ideep\mkl-dnn\include -I..\third_party\ideep\mkl-dnn\src\..\include -I"C:\Program Files\NVIDIA Corporation\NvToolsExt\include" -I..\c10\cuda\..\.. -I..\cmake\..\third_party\googletest\googlemock\include -I..\cmake\..\third_party\googletest\googletest\include -I..\third_party\protobuf\src -Iwin_tmp\mkl\include -I..\third_party -I..\cmake\..\third_party\eigen -IC:\Jenkins\Miniconda3\include -IC:\Jenkins\Miniconda3\lib\site-packages\numpy\core\include -I..\cmake\..\third_party\pybind11\include -I\opt\rocm\hip\include -I\include -I..\cmake\..\third_party\cub -Iwin_tmp\magma\include -I"C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v10.1\include" -I..\third_party\ideep\mkl-dnn\include -I..\third_party\ideep\include -I..\third_party\googletest\googletest\include -I..\third_party\googletest\googletest /DWIN32 /D_WINDOWS /GR  /w /EHa /bigobj -openmp -DNDEBUG -DUSE_FBGEMM -DHAVE_AVX_CPU_DEFINITION -DHAVE_AVX2_CPU_DEFINITION /MD /O2 /Ob2 /DNDEBUG /w /EHa /bigobj -DNDEBUG   -DCUDA_HAS_FP16=1 -DUSE_GCC_GET_CPUID -DUSE_AVX -DUSE_AVX2 -DTH_HAVE_THREAD /Z7 /EHa /DNOMINMAX /wd4267 /wd4251 /wd4522 /wd4838 /wd4305 /wd4244 /wd4190 /wd4101 /wd4996 /wd4275 /bigobj -openmp -std:c++14 /showIncludes /Fotest_api\CMakeFiles\test_api.dir\optim.cpp.obj /Fdtest_api\CMakeFiles\test_api.dir\ /FS -c ..\test\cpp\api\optim.cpp
FAILED: test_api/CMakeFiles/test_api.dir/optim.cpp.obj 
C:\Users\circleci\project\build\win_tmp\bin\sccache-cl.exe   /TP -DAT_PARALLEL_OPENMP=1 -DIDEEP_USE_MKL -DMAGMA_V2 -DMINIZ_DISABLE_ZIP_READER_CRC32_CHECKS -DONNX_ML=1 -DONNX_NAMESPACE=onnx_torch -DTH_BLAS_MKL -DUSE_CUDA -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE=1 -D_OPENMP_NOFORCE_MANIFEST -Iaten\src -I..\aten\src -I. -I..\ -I..\cmake\..\third_party\benchmark\include -Icaffe2\contrib\aten -I..\third_party\onnx -Ithird_party\onnx -I..\third_party\foxi -Ithird_party\foxi -I..\caffe2\..\torch\..\aten\src -Icaffe2\..\aten\src -Icaffe2\..\aten\src\ATen -I..\caffe2\..\torch\csrc\api -I..\caffe2\..\torch\csrc\api\include -I..\c10\.. -Ithird_party\ideep\mkl-dnn\include -I..\third_party\ideep\mkl-dnn\src\..\include -I"C:\Program Files\NVIDIA Corporation\NvToolsExt\include" -I..\c10\cuda\..\.. -I..\cmake\..\third_party\googletest\googlemock\include -I..\cmake\..\third_party\googletest\googletest\include -I..\third_party\protobuf\src -Iwin_tmp\mkl\include -I..\third_party -I..\cmake\..\third_party\eigen -IC:\Jenkins\Miniconda3\include -IC:\Jenkins\Miniconda3\lib\site-packages\numpy\core\include -I..\cmake\..\third_party\pybind11\include -I\opt\rocm\hip\include -I\include -I..\cmake\..\third_party\cub -Iwin_tmp\magma\include -I"C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v10.1\include" -I..\third_party\ideep\mkl-dnn\include -I..\third_party\ideep\include -I..\third_party\googletest\googletest\include -I..\third_party\googletest\googletest /DWIN32 /D_WINDOWS /GR  /w /EHa /bigobj -openmp -DNDEBUG -DUSE_FBGEMM -DHAVE_AVX_CPU_DEFINITION -DHAVE_AVX2_CPU_DEFINITION /MD /O2 /Ob2 /DNDEBUG /w /EHa /bigobj -DNDEBUG   -DCUDA_HAS_FP16=1 -DUSE_GCC_GET_CPUID -DUSE_AVX -DUSE_AVX2 -DTH_HAVE_THREAD /Z7 /EHa /DNOMINMAX /wd4267 /wd4251 /wd4522 /wd4838 /wd4305 /wd4244 /wd4190 /wd4101 /wd4996 /wd4275 /bigobj -openmp -std:c++14 /showIncludes /Fotest_api\CMakeFiles\test_api.dir\optim.cpp.obj /Fdtest_api\CMakeFiles\test_api.dir\ /FS -c ..\test\cpp\api\optim.cpp
C:\Users\circleci\project\torch\csrc\api\include\torch/optim/optimizer.h(62): error C2201: 'const torch::optim::OptimizerCloneableOptions<`private: virtual void __cdecl OptimTest_OldInterface_Test::TestBody(void)'::`2'::MyOptimizerOptions>::`vftable'': must have external linkage in order to be exported/imported
..\test\cpp\api\optim.cpp(191): note: see reference to class template instantiation 'torch::optim::OptimizerCloneableOptions<OptimTest_OldInterface_Test::TestBody::MyOptimizerOptions>' being compiled
C:\Users\circleci\project\c10/core/MemoryFormat.h(56): note: see reference to class template instantiation 'c10::ArrayRef<int64_t>' being compiled
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27038 for x64

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Mar 19, 2020

https://app.circleci.com/pipelines/github/pytorch/pytorch/142026/workflows/2c905e40-dbcf-4f36-ab96-adf171e2da84/jobs/4867720

@anjali411 anjali411 reopened this Mar 19, 2020
@anjali411 anjali411 removed the merged label Mar 19, 2020
@anjali411 anjali411 force-pushed the merge_optim branch 3 times, most recently from 1ef20ab to 026b53f Compare March 20, 2020 14:46
@yf225 yf225 mentioned this pull request Mar 20, 2020
Closed
@anjali411 anjali411 mentioned this pull request Mar 23, 2020
Closed
@yf225 yf225 force-pushed the merge_optim branch 5 times, most recently from 2f1fdab to 46ea71d Compare March 24, 2020 18:45
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Mar 26, 2020

@yf225 merged this pull request in efbd6b8.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Mar 26, 2020

This is still breaking Windows. ..\test\cpp\api\optim.cpp(193): error C2440: 'return': cannot convert from 'OptimTest_OldInterface_Test::TestBody::MyOptimizerOptions' to 'OptimTest_OldInterface_Test &'

https://app.circleci.com/pipelines/github/pytorch/pytorch/146720/workflows/1b89c2c9-b198-4902-8d67-72551af981db/jobs/4956748

@yf225 yf225 reopened this Mar 26, 2020
facebook-github-bot
facebook-github-bot reviewed Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yf225 yf225 removed the merged label Mar 27, 2020
@yf225 yf225 added the merged label Mar 27, 2020
yf225 pushed a commit that referenced this pull request Mar 27, 2020
@anjali411
[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimiz…
5c0b91b 
…er (#34957)

Summary:
1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
3. BC-compatibility serialization test for LBFGS
4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
5. Made defaults_ optional argument in all optimizers except SGD

**TODO**: add BC-breaking notes for this PR

Pull Request resolved: #34957

Test Plan: Imported from GitHub, without a `Test Plan:` line.

Differential Revision: D20678162

Pulled By: yf225

fbshipit-source-id: 74e062e42d86dc118f0fbaddd794e438b2eaf35a
gchanan pushed a commit that referenced this pull request Mar 27, 2020
@anjali411 @gchanan
[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimiz…
2a789cd 
…er (#34957)

Summary:
1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
3. BC-compatibility serialization test for LBFGS
4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
5. Made defaults_ optional argument in all optimizers except SGD

**TODO**: add BC-breaking notes for this PR

Pull Request resolved: #34957

Test Plan: Imported from GitHub, without a `Test Plan:` line.

Differential Revision: D20678162

Pulled By: yf225

fbshipit-source-id: 74e062e42d86dc118f0fbaddd794e438b2eaf35a
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@anjali411 @facebook-github-bot
[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimiz…
12aa2b8 
…er (pytorch#34957)

Summary:
1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
3. BC-compatibility serialization test for LBFGS
4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
5. Made defaults_ optional argument in all optimizers except SGD
Pull Request resolved: pytorch#34957

Test Plan: Imported from GitHub, without a `Test Plan:` line.

Differential Revision: D20518647

Pulled By: anjali411

fbshipit-source-id: 4760d1d29df1784e2d01e2a476d2a08e9df4ea1c
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@anjali411 @facebook-github-bot
[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimiz…
bca1fe6 
…er (pytorch#34957)

Summary:
1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
3. BC-compatibility serialization test for LBFGS
4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
5. Made defaults_ optional argument in all optimizers except SGD

**TODO**: add BC-breaking notes for this PR

Pull Request resolved: pytorch#34957

Differential Revision: D20645945

Pulled By: yf225

fbshipit-source-id: 383588065bf1859b38f0ad0a25d93d41e153c96e
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@anjali411 @facebook-github-bot
[C++ API Parity] [Optimizers] Merged Optimizer and LossClosureOptimiz…
dd758d0 
…er (pytorch#34957)

Summary:
1. Removed LossClosureOptimizer, and merged Optimizer into OptimizerBase (and renamed the merged class to Optimizer)
2. Merged the LBFGS-specific serialize test function and the generic test_serialize_optimizer function.
3. BC-compatibility serialization test for LBFGS
4. Removed mentions of parameters_ in optimizer.cpp, de-virtualize all functions
5. Made defaults_ optional argument in all optimizers except SGD

**TODO**: add BC-breaking notes for this PR

Pull Request resolved: pytorch#34957

Test Plan: Imported from GitHub, without a `Test Plan:` line.

Differential Revision: D20678162

Pulled By: yf225

fbshipit-source-id: 74e062e42d86dc118f0fbaddd794e438b2eaf35a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebetica ebetica Awaiting requested review from ebetica

@goldsborough goldsborough Awaiting requested review from goldsborough

2 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@yf225 yf225 yf225 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: bc-breaking Related to a BC-breaking change module: cpp Related to C++ API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@anjali411 @facebook-github-bot @ezyang @yf225 @mruberry