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

Consolidate tools. #789

Merged
merged 6 commits into from
Jul 26, 2014
Merged

Consolidate tools. #789

merged 6 commits into from
Jul 26, 2014

Conversation

Yangqing
Copy link
Member

This is to consolidate the various tools that caffe has into one single binary ("caffe.bin"), and deprecate the multiple binaries hard to remember. In the meantime, we will also switch to the gflags library and get rid of all the argv[4] type hard-coding.

As a result, if you want to do device query, do:

build/tools/caffe.bin devicequery --device_id=0

if you want to do e.g. training with an option of finetuning, do

build/tools/caffe.bin train --solver_proto_file=solver.prototxt [--pretrained_net_file=pretrained.binaryproto]

I have currently deprecated device_query, train_net, finetune_net, net_speed_benchmark. Others could be potentially deprecated in the same way.

Yangqing Jia added 3 commits July 25, 2014 09:08
@Yangqing
Copy link
Member Author

Evan, Jeff, Jon - this is the idea we discussed about over emails. Please see if such direction seems right to you.

@shelhamer @jeffdonahue @longjon

@jeffdonahue
Copy link
Contributor

Nice, this will be great! I have a couple subjective suggestions:

  • Could the binary name just be "caffe" rather than "caffe.bin"? I'd like to be able to think of running caffe like git, and somehow having an extension makes that seem less natural. (Of course one could always create an alias or any other type of shortcut, but it would be more convenient not to have to do that.)
  • Could we just have the "train" verb require a single argument, the solver prototxt, to always be passed in rather than having a flag for it? Given that the solver prototxt is required for training, the flag seems unnecessary.

@Yangqing
Copy link
Member Author

The ".bin" extension is added by the makefile, so that would be easy to change.

I am a little inclined to not have unnamed parameters - since then one has to remember for each command what is needed, and in the code we will have to do argv[2] type stuff again. having a name prepending each parameter will be a little longer but will make every parameter (other than the action verb) more semantically reasonable. Also, if we want to change parameters in the future, hard-coded argv will be hard to maintain.

@jeffdonahue
Copy link
Contributor

K, your reasoning on not using unnamed parameters makes sense. On the .bin extension, maybe we should leave it in the Makefile since otherwise people will try to run train_net.bin and not get the deprecation error message? We could just softlink the extension-less version.

@Yangqing
Copy link
Member Author

Makes sense. I am leaving the .bin as is since if we want, we can simply add

ln -s build/tools/caffe.bin build/tools/caffe

and have it solved.

Also, we will need to change our various documentations and start using "caffe action" type command.

@shelhamer @longjon any opinions? I'll merge for now if it looks OK.

@jeffdonahue
Copy link
Contributor

The example scripts (*train*.sh and *finetune*.sh under examples/) should probably be updated to use the new consolidated binary before merge.

LOG(INFO) << "Optimization Done.";

LOG(ERROR) << "Deprecated. Use caffe.bin train --solver_proto_file=... "
"[--resume_point_file=...] instead.";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe these should be LOG(FATAL)s?

Copy link
Contributor

Choose a reason for hiding this comment

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

(obviously this doesn't matter much because the tool no longer does anything anyway, but I think it might be good to have the script fail by returning 1 and/or LOG(FATAL)ing in case someone actually makes use of those exit codes to see if their script calling train_net.bin succeeded)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for consolidating like we talked about @Yangqing! Looks good to
merge once the bundled examples are updated and Jeff's comments are
addressed.

I too would rather have "caffe" instead of "caffe.bin" but for deprecation
let's just add the symlink to the Makefile, then in the following
release drop the .bin.

On Friday, July 25, 2014, Jeff Donahue notifications@github.com wrote:

In tools/train_net.cpp:

  • SolverParameter solver_param;

- ReadProtoFromTextFileOrDie(argv[1], &solver_param);

  • LOG(INFO) << "Starting Optimization";
  • SGDSolver solver(solver_param);
  • if (argc == 3) {
  • LOG(INFO) << "Resuming from " << argv[2];
  • solver.Solve(argv[2]);
  • } else {
  • solver.Solve();
  • }

- LOG(INFO) << "Optimization Done.";

  • LOG(ERROR) << "Deprecated. Use caffe.bin train --solver_proto_file=... "
  •            "[--resume_point_file=...] instead.";
    

(obviously this doesn't matter much because the tool no longer does
anything anyway, but I think it might be good to have the script fail by
returning 1 and/or LOG(FATAL)ing in case someone actually makes use of
those exit codes to see if their script calling train_net.bin succeeded)


Reply to this email directly or view it on GitHub
https://github.com/BVLC/caffe/pull/789/files#r15431553.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll change the examples. Not doing the symlinking for now - maybe let's explicitly switch when all the tools are in one binary.

@Yangqing
Copy link
Member Author

Done updating and passing travis, merging :)

Yangqing added a commit that referenced this pull request Jul 26, 2014
@Yangqing Yangqing merged commit d5f1a50 into BVLC:dev Jul 26, 2014
@shelhamer
Copy link
Member

@Yangqing or other gflag / glog experts: what's the simplest way to make the consolidated tools produce (normal, successful) output by default? For instance, caffe devicequery requires prepending GLOG_logtostderr=1 to print anything as well as other tools noted elsewhere.

I'd like to prepare a release but before that the consolidated tool should be friendly and the examples should use it.

@shelhamer shelhamer mentioned this pull request Aug 7, 2014
5 tasks
@shelhamer
Copy link
Member

Sorry, I missed it in the glog documentation before. It's just FLAGS_logtostderr = 1;

@shelhamer shelhamer mentioned this pull request Aug 7, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
RazvanRanca pushed a commit to RazvanRanca/caffe that referenced this pull request Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants