Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"weights" added to solver parameters, "snapshot_prefix" field default initialization #6123

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

IlyaOvodov
Copy link
Contributor

@IlyaOvodov IlyaOvodov commented Dec 21, 2017

This pull request implements parts 1&2 of feature request #6110.

"weights" parameter is added to SolverParameters class. It has quite the same meaning as "--weights" command line parameter of caffe executable and is used if "--weights" command line parameter is not specified. It makes it possible to specify initial weights in a solver.prototxt file instead of command line and make it unnecessary to change command line or command file.

It is aimed to make it possible to localize all training parameters in a solver.prototxt file. See #6110 for more detailed motivation.

If a "--snapshot" command line parameter is specified, new "weights" parameter in a solver.prototxt file is ignored. So there is no need to modify this file to continue previously interrupted training process from a snapshot.

EDIT by @Noiredd: second important contribution of this PR is a default initialization of the "solver_prefix" parameter. Previously Caffe would fail on parsing the solver if a snapshot was requested but no prefix given.

@Noiredd
Copy link
Member

Noiredd commented Jan 3, 2018

I will review this today/tomorrow. In the meantime, please squash your commits to maintain a simpler history.

If you want to keep all changes related to #6110 in this PR (which would be great IMO), I would suggest having one commit for each point. This would make review much easier.

@IlyaOvodov
Copy link
Contributor Author

IlyaOvodov commented Jan 8, 2018

Thank you! I am not shure it's a good idea to keep all changes related to #6110 in this one PR because 1) they are absolutely independent and 2) I'm not shure that I'll have possibility to implement all them at once. So shep-by-step approach looks better.
But I realized that it is worth adding python support of "weights" property to SolverParams to this PR, so I ask to wait with review until I'll do it as well as I'll squash commits (sorry, my fault).

@IlyaOvodov
Copy link
Contributor Author

IlyaOvodov commented Jan 12, 2018

I've squashed commits i this PR. I've found that python support of specific "weights" property is quite useless until python interface to solver will be improved in total. So this PR looks to be completed and ready for review.

But some tests failed with messages like these:
W: The repository 'http://dl.google.com/linux/chrome/deb stable Release' does not have a Release file.
W: Failed to fetch http://www.apache.org/dist/cassandra/debian/dists/39x/InRelease Unexpected type excepted 21 != 5
W: Failed to fetch http://ppa.launchpad.net/chris-lea/redis-server/ubuntu/dists/trusty/InRelease Could not connect to ppa.launchpad.net:80 (91.189.95.83), connection timed out
W: Failed to fetch http://ppa.launchpad.net/couchdb/stable/ubuntu/dists/trusty/InRelease Unable to connect to ppa.launchpad.net:http:

It seems not to be connected to my changes but rather to som network or configuration problems of build server. I have no idea what to do with it, even I have no possibility to retry them. Can you suggest?

@IlyaOvodov IlyaOvodov changed the title The "weights" parameter is added to solver parameters Improvements for issue #6110: The "weights" parameter is added to solver parameters, "snapshot_prefix" defailt initialization Jan 13, 2018
@IlyaOvodov
Copy link
Contributor Author

Along the way I've added implementation of improvement No.2 from issue #6110.
Now this PR is fully completed.

Copy link
Member

@Noiredd Noiredd left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, @IlyaOvodov! I left one comment in the code - let's discuss that and merge this soon.

// If command line --weights parameter if specified, it has higher priority
// and owerwrites this one.
// If --snapshot command line parameter is specified, this one is ignored.
optional string weights = 42;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better (or at least more Caffe-style) to do this as a repeated field? I don't mind having an option to pass comma-separated values, but being able to use separate entries for several models will help avoid chaos (especially when using long absolute paths).

@Noiredd Noiredd changed the title Improvements for issue #6110: The "weights" parameter is added to solver parameters, "snapshot_prefix" defailt initialization "weights" added to solver parameters, "snapshot_prefix" field default initialization Jan 30, 2018
Loading weights is moved from caffe.exe to solver class, so new "weights" solver parameter is used not only from command line but when caffe is used as library (including python)

corrected formatting

fixed line length

more formatting corrected
…points to a directory. See issue BVLC#6110 proposed improvement No.2
@IlyaOvodov
Copy link
Contributor Author

@Noiredd , I agree with your idea and decided that to make appropriate changes is better than to discuss it.
Modifications are squashed to the 1st commit here.

@Noiredd
Copy link
Member

Noiredd commented Feb 12, 2018

Looks good. Thank you for the improvement, @IlyaOvodov!

@Noiredd Noiredd merged commit a44c444 into BVLC:master Feb 12, 2018
IlyaOvodov pushed a commit to IlyaOvodov/caffe that referenced this pull request Feb 12, 2018
"weights" added to solver parameters, "snapshot_prefix" field default initialization
@IlyaOvodov
Copy link
Contributor Author

compiling would give error like:

There are these 2 reasons:

  1. You didn't update caffe.proto file. Check that you have line "repeated string weights = 42; " (c326294#diff-e0bd8d028edf0f88febfebe3bf703904R253) in it
  2. You didn't fully rebuild the project, so that caffe.proto file was not recompiled.

XinYao1994 pushed a commit to XinYao1994/caffe that referenced this pull request Aug 29, 2018
"weights" added to solver parameters, "snapshot_prefix" field default initialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants