Expose necessary interface to guide manual SGD process from Python by mitar · Pull Request #6238 · BVLC/caffe

mitar · 2018-02-14T16:17:18Z

This fixes #3959 and #2686.

EDIT by @Noiredd: do not merge until #6209 is merged first (or else terrible conflicts will arise during master to opencl pull).

Noiredd

Good addition, I was about to implement something like this myself but first we need to pull some Pycaffe changes from the OpenCL branch to synchronize the whole repository. For this reason I recommend holding this until #6209 is reviewed and merged.

In the meantime, please address a few questions I've left in the code.

mitar · 2018-02-14T16:45:27Z

Thanks for the quick code review!

Noiredd

Sorry for the confusion @mitar, I am afraid I accidentally removed your comment. Sorry about that, I only wanted to merge my review comments for brevity.

Noiredd · 2018-02-14T16:45:20Z

python/caffe/_caffe.cpp

    .add_property("display", &SolverParameter::display)
-    .add_property("layer_wise_reduce", &SolverParameter::layer_wise_reduce);
+    .add_property("layer_wise_reduce", &SolverParameter::layer_wise_reduce)
+    .add_property("base_lr", &SolverParameter::base_lr, &SolverParameter::set_base_lr);


This change is included in #6209 which will be merged prior to any other changes to _caffe.cpp.

Oho, looking at #6209 looks like everything in this pull request is done also there, no?

Noiredd · 2018-02-14T16:45:33Z

python/caffe/_caffe.cpp

    shared_ptr<SGDSolver<Dtype> >, boost::noncopyable>(
-        "SGDSolver", bp::init<string>());
+        "SGDSolver", bp::init<string>())
+        .def("get_learning_rate", &SGDSolver<Dtype>::GetLearningRate);


What about other solvers, will they get this property only due to the inheritance?

Other solvers extending from SGDSolver? I think only them will get this, but this is OK, no? this is on purpose.

So if I do a = caffe.AdamSolver("solver.prototxt") in my pycaffe script, will I be able to call a.get_learning_rate() or not?

From my understanding, the answer is yes, but I have not tested it.

Would be good to know this. Maybe consider implementing some tests for this change? See #4342, it is somewhat similar to this (a bit more clumsy) but also had some very simple tests.

No, this does not inherit correctly b.c. the bp::bases of the child SGDSolvers are set to Solver and not SGDSolver. Switching the NesteroveSolver and other children to have the SGDSolver base fixes the issue. I second incorporating the apply_update() test from c5b5b55, so could you cherry-pick it to give @nitnelave credit for it?

Noiredd · 2018-02-14T16:45:40Z

src/caffe/solver.cpp

-    // Increment the internal iter_ counter -- its value should always indicate
-    // the number of times the weights have been updated.
-    ++iter_;
-


Why did this have to be moved to SGDSolver?

Based on the comment: "its value should always indicate the number of times the weights have been updated."

Because now it is possible to update weights without having to call step, this has to be moved so that also calling directly ApplyUpdate increases it. I think that this is even cleaner from the design perspective. The code is next to the code which is updating weights. Not somewhere else.

Ideally the base Solver would enforce the increment to iter_, but since all Caffe solvers are SGD solvers and other solvers that depart from this mold aren't expected I think the move is a pragmatic choice that avoids unnecessary interface code.

Noiredd · 2018-02-14T16:52:59Z

Please run make lint and fix the style issues and we will resume the review.
Also, I'm not sure if you noticed but I accidentally removed your comment - please repost it if needed.

mitar · 2018-02-14T16:58:15Z

Good addition, I was about to implement something like this myself but first we need to pull some Pycaffe changes from the OpenCL branch to synchronize the whole repository. For this reason I recommend holding this until #6209 is reviewed and merged.

Thanks. I can rebase after that is merged.

Please run make lint and fix the style issues and we will resume the review.

Working on that.

BTW, I cannot see results from Travis CI run. Are they set as private?

mitar · 2018-02-14T17:02:27Z

Pushed linting fixes.

shelhamer · 2018-02-15T03:23:14Z

Thanks @mitar! I have been meaning to make a PR like this myself for some time. I'll take a look at both #6209 and this soon.

shelhamer

Thanks @mitar for exposing solver updating to pycaffe. This has come up many times and been done in forks and branches and so it certainly deserves to be done in master. I vote for merge once the inheritance issue is resolved.

Although #6209 came first and makes further improvements, this PR is simpler b.c. of its reduced scope and is ready for immediate use. #6209 can be adjusted once this is merged. Thanks @Noiredd for porting all those improvements and reviewing this PR too.

shelhamer · 2018-06-06T20:28:11Z

src/caffe/solver.cpp

-    // Increment the internal iter_ counter -- its value should always indicate
-    // the number of times the weights have been updated.
-    ++iter_;
-


Ideally the base Solver would enforce the increment to iter_, but since all Caffe solvers are SGD solvers and other solvers that depart from this mold aren't expected I think the move is a pragmatic choice that avoids unnecessary interface code.

shelhamer · 2018-06-06T20:52:24Z

python/caffe/_caffe.cpp

    shared_ptr<SGDSolver<Dtype> >, boost::noncopyable>(
-        "SGDSolver", bp::init<string>());
+        "SGDSolver", bp::init<string>())
+        .def("get_learning_rate", &SGDSolver<Dtype>::GetLearningRate);


No, this does not inherit correctly b.c. the bp::bases of the child SGDSolvers are set to Solver and not SGDSolver. Switching the NesteroveSolver and other children to have the SGDSolver base fixes the issue. I second incorporating the apply_update() test from c5b5b55, so could you cherry-pick it to give @nitnelave credit for it?

a sketch of `solver.step()` done out manually: 1. `solver.net.forward()` 2. `solver.net.backward()` 3. `solver.net.apply_update()` 4. `solver.net.clear_param_diffs()`

with update exposed it is important to increment the iteration when an update is made, whether by step or update alone. more fundementally, it's the update that defines an iterationa, so this is a natural place for the increment.

`solver.lr` is the effective learning rate in use while `solver.base_lr` is the configured learning rate at initialization. the solver parameter is now editable for setting fields that are in use throughout the lifetime of the solver, such as the maximum iteration.

shelhamer · 2018-06-08T02:54:27Z

@mitar I had time to do a pass so I updated this branch by rebasing on master, rewording for detail, and including a test for updating from pycaffe. I'm planning to merge once the tests pass to double-check my local results.

mitar · 2018-06-08T03:13:14Z

Oh, thanks!

mitar · 2018-06-08T04:32:20Z

What are plans for a release which would include this? Or do you have some daily build I could pip install without compiling the repo myself?

shelhamer · 2018-06-08T14:46:55Z

The current state of affairs with master is that you have to compile it yourself.

What are plans for a release which would include this?

There is no definite plan right now but I could imagine a release in early July to collect improvements since 1.0. I will bring it up with the core devs.

Or do you have some daily build I could pip install without compiling the repo myself?

No, we do not have a daily build, although it would obviously be useful. I've never rigged up such a system myself, but perhaps a daily linux build (specifically for the Ubuntu LTS) would be feasible.

mitar · 2018-06-08T16:23:41Z

No, we do not have a daily build, although it would obviously be useful. I've never rigged up such a system myself, but perhaps a daily linux build (specifically for the Ubuntu LTS) would be feasible.

You could even push artifacts from Travis CI.

I think you also publish a PyPi package like other libraries do. Using manylinux and if you need more space there for the package, you can request it here.

[pycaffe] expose interface for manual, step-by-step optimization

mitar mentioned this pull request Feb 14, 2018

Python manual sgd #3959

Closed

Noiredd reviewed Feb 14, 2018

View reviewed changes

BVLC deleted a comment from mitar Feb 14, 2018

Noiredd reviewed Feb 14, 2018

View reviewed changes

Noiredd mentioned this pull request Feb 14, 2018

Matcaffe: where is the update() method ? #4758

Open

Noiredd added in progress Python labels Feb 14, 2018

This was referenced Feb 14, 2018

Pycaffe improvements from OpenCL branch #6209

Open

Improve python interface for the solver #4342

Closed

Noiredd self-assigned this Feb 14, 2018

shelhamer requested changes Jun 6, 2018

View reviewed changes

mitar and others added 4 commits June 7, 2018 15:13

[pycaffe] expose solver update to do manual solving

cc1c8fb

a sketch of `solver.step()` done out manually: 1. `solver.net.forward()` 2. `solver.net.backward()` 3. `solver.net.apply_update()` 4. `solver.net.clear_param_diffs()`

increment iteration during update, not step

c74913d

with update exposed it is important to increment the iteration when an update is made, whether by step or update alone. more fundementally, it's the update that defines an iterationa, so this is a natural place for the increment.

[pycaffe] test solver update

1bdcb74

shelhamer force-pushed the manual-sgd branch from e9659a1 to 1bdcb74 Compare June 8, 2018 02:52

shelhamer approved these changes Jun 8, 2018

View reviewed changes

shelhamer added focus and removed in progress labels Jun 8, 2018

shelhamer merged commit 2a1c552 into BVLC:master Jun 8, 2018

sjb7749 pushed a commit to sjb7749/caffe that referenced this pull request Jul 2, 2018

Merge pull request BVLC#6238 from mitar/manual-sgd

0b2b145

[pycaffe] expose interface for manual, step-by-step optimization

XinYao1994 pushed a commit to XinYao1994/caffe that referenced this pull request Aug 29, 2018

Merge pull request BVLC#6238 from mitar/manual-sgd

eaa7493

[pycaffe] expose interface for manual, step-by-step optimization

pmgysel mentioned this pull request Dec 2, 2018

Merge branch 'master' of https://github.com/BVLC/caffe pmgysel/caffe#7

Merged

Conversation

mitar commented Feb 14, 2018 • edited by Noiredd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Noiredd left a comment

Choose a reason for hiding this comment

Uh oh!

mitar commented Feb 14, 2018

Uh oh!

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

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!

mitar Feb 14, 2018 • 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!

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!

Noiredd commented Feb 14, 2018

Uh oh!

mitar commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitar commented Feb 14, 2018

Uh oh!

shelhamer commented Feb 15, 2018

Uh oh!

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

shelhamer commented Jun 8, 2018

Uh oh!

mitar commented Jun 8, 2018

Uh oh!

mitar commented Jun 8, 2018

Uh oh!

shelhamer commented Jun 8, 2018

Uh oh!

mitar commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mitar commented Feb 14, 2018 •

edited by Noiredd

Loading

Noiredd left a comment •

edited

Loading

mitar Feb 14, 2018 •

edited

Loading

mitar commented Feb 14, 2018 •

edited

Loading