Implement device abstraction for remaining classes

[robwhess]

There are more classes left to abstract to continue the work started by @kloudkl in #415. Here's a PR in which to do that work. I'll note when the PR is ready for review/merge.

I'm not sure if there will be more PRs into this branch after this, but we can take that as it comes.

[robwhess]

@shelhamer @jeffdonahue @sergeyk @longjon @kloudkl, etc.

I'd like to get your input on the direction I've taken with c489e3e8c77a92f2374a00b568bcf1fe438db292 (mostly in the new file src/caffe/devices/cpu_device.cpp). The goal here is to bring all the device-specific functions under the device classes so, for example, instead of calling caffe_cpu_axpy() or caffe_gpu_axpy(), you'd do something like this (in conjunction with the new function GetDevice() from src/caffe/device_factory.cpp):

GetDevice()->axpy(...);  // axpy on the device corresponding to the current mode

or, to specifically use the cpu or gpu version:

GetDevice(Caffe::CPU)->axpy(...);
GetDevice(Caffe::GPU)->axpy(...);

The end goal here would be to have all the cpu functions in src/caffe/devices/cpu_device.cpp, all the gpu functions in src/caffe/devices/gpu_device.cpp, etc., and to get rid of src/caffe/util/math_functions.{cpp,cu} and all the other files where device specific functions are defined (e.g. src/caffe/util/im2col.{cpp,cu}). Then, new device types would simply get a new source file in src/caffe/devices/.

I think the thing that's a bit tricky here is how to deal with layers that have layer-specific gpu kernels. Right now, you're using Forward_cpu(), Forward_gpu(), etc. to handle this, where the *_gpu() functions can launch kernels, etc., and the Layer class makes the decision about which one of these to use based on the current mode. My current idea to handle this going forward is to have each layer implement only Forward() and Backward(), and each layer's functions can explicitly decide (e.g. with an if or switch) whether to launch a kernel or call a gpu-specific function based on the current mode (or, down the line, based on a layer-specific device that can be assigned to a specific layer instance with a setter function, e.g. SetDevice(Caffe::GPU) or something).

Let me know if you think that's a useful way to continue here. If it is, I'll move forward with a similar consolidation of all the gpu functions in math_functions.{cpp,cu} into gpu_device.{cpp,cu}.

[robwhess]

Oh, I'll also work on rebasing on dev now that #555 is merged. That will interact with some of the changes I made in c489e3e8c77a92f2374a00b568bcf1fe438db292, and the two will have to be integrated.

[jeffdonahue]

Hey @robwhess, this looks great to me, but doesn't compile for me @ c489e3e (maybe I shouldn't I be able to?). I get these errors (using Ubuntu with gcc 4.6.3):

src/caffe/devices/cpu_device.cpp: In member function ‘virtual void caffe::CPUDevice<Dtype>::copy_from_cpu(int, const Dtype*, Dtype*)’:
src/caffe/devices/cpu_device.cpp:96:13: error: expected primary-expression before ‘>’ token
src/caffe/devices/cpu_device.cpp: In member function ‘virtual void caffe::CPUDevice<Dtype>::set(int, Dtype, Dtype*)’:
src/caffe/devices/cpu_device.cpp:102:12: error: ‘Y’ was not declared in this scope
src/caffe/devices/cpu_device.cpp:106:5: error: ‘Y’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: In member function ‘virtual void caffe::CPUDevice<Dtype>::add_scalar(int, Dtype, Dtype*)’:
src/caffe/devices/cpu_device.cpp:120:5: error: ‘Y’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: In member function ‘void caffe::CPUDevice<Dtype>::sqr(int, const Dtype*, Dtype*) [with Dtype = float]’:
src/caffe/devices/cpu_device.cpp:141:9: error: ‘n’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: In member function ‘void caffe::CPUDevice<Dtype>::sqr(int, const Dtype*, Dtype*) [with Dtype = double]’:
src/caffe/devices/cpu_device.cpp:146:9: error: ‘n’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: At global scope:
src/caffe/devices/cpu_device.cpp:198:6: error: template-id ‘powx<>’ for ‘void caffe::CPUDevice<float>::powx(int, const float*, const float*, float*)’ does not match any template declaration
src/caffe/devices/cpu_device.cpp:199:41: note: saw 1 ‘template<>’, need 2 for specializing a member function template
src/caffe/devices/cpu_device.cpp:204:6: error: template-id ‘powx<>’ for ‘void caffe::CPUDevice<double>::powx(int, const double*, const double*, double*)’ does not match any template declaration
src/caffe/devices/cpu_device.cpp:205:42: note: saw 1 ‘template<>’, need 2 for specializing a member function template
src/caffe/devices/cpu_device.cpp: In member function ‘void caffe::CPUDevice<Dtype>::hamming_distance(int, const Dtype*, const Dtype*, uint32_t*) [with Dtype = float, uint32_t = unsigned int]’:
src/caffe/devices/cpu_device.cpp:327:23: error: ‘n’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: At global scope:
src/caffe/devices/cpu_device.cpp:335:6: error: template-id ‘hamming_distance<>’ for ‘void caffe::CPUDevice<double>::hamming_distance(int, const double*, const double*, uint64_t*)’ does not match any template declaration
src/caffe/devices/cpu_device.cpp:336:76: note: saw 1 ‘template<>’, need 2 for specializing a member function template
src/caffe/devices/cpu_device.cpp: In member function ‘virtual void caffe::CPUDevice<Dtype>::col2im(const Dtype*, int, int, int, int, int, int, Dtype*)’:
src/caffe/devices/cpu_device.cpp:464:40: error: ‘ksize’ was not declared in this scope
src/caffe/devices/cpu_device.cpp: At global scope:
src/caffe/devices/cpu_device.cpp:483:6: error: specializing member ‘caffe::CPUDevice<float>::col2im’ requires ‘template<>’ syntax
src/caffe/devices/cpu_device.cpp:486:6: error: specializing member ‘caffe::CPUDevice<double>::col2im’ requires ‘template<>’ syntax
src/caffe/devices/cpu_device.cpp: In member function ‘void caffe::CPUDevice<Dtype>::copy_from_cpu(int, const Dtype*, Dtype*) [with Dtype = float]’:
src/caffe/devices/cpu_device.cpp:490:1:   instantiated from here
src/caffe/devices/cpu_device.cpp:96:3: warning: left operand of comma operator has no effect [-Wunused-value]
src/caffe/devices/cpu_device.cpp:96:3: warning: right operand of comma operator has no effect [-Wunused-value]
src/caffe/devices/cpu_device.cpp: In member function ‘void caffe::CPUDevice<Dtype>::copy_from_cpu(int, const Dtype*, Dtype*) [with Dtype = double]’:
src/caffe/devices/cpu_device.cpp:490:1:   instantiated from here
src/caffe/devices/cpu_device.cpp:96:3: warning: left operand of comma operator has no effect [-Wunused-value]
src/caffe/devices/cpu_device.cpp:96:3: warning: right operand of comma operator has no effect [-Wunused-value]

It does compile @ the previous commit: 92b98f7

[kloudkl]

@robwhess, thanks for the refactoring efforts!

To keep backward compatibility, the original math functions were not directly moved into the device classes in #415. This is a very important design decision considering that there are hundreds of users that have forked, git cloned or downloaded the code and developed their own algorithms or applications. Some of them must have explicitly used the math functions which can't be removed without breaking their codes.

Note that #633 enables #555 to run on CPU.

[robwhess]

@jeffdonahue, woops, I completely forgot to run make before committing. Will fix tomorrow.

@kloudkl is this a major concern? This is still a very young software package. I feel like at this point in the project's life, it'd be best to let the interface evolve into what it should be (and it's still an open question as to whether this particular change is what Caffe should be) rather than worrying a lot about backwards compatibility. I think that's especially true in this case because the resolution of this change would be a simple find-and-replace operation. I suspect there have already been tougher changes to integrate back into peoples' forks than this one.

If folks find this change worthwhile but believe backwards incompatibility is a real problem here, maybe as an intermediate step we can keep the actual implementation of the device-specific functionality in the Device classes' methods, replace the implementations in the caffe_*() functions with calls to the corresponding Device methods, and then deprecate the caffe_*() functions.

My personal preference is to skip deprecation and get rid of the caffe_*() functions altogether because I think the abstraction into Device classes is cleaner and will make it easier to separate out the compilation of cuda-specific code going forward.

[jeffdonahue]

I'm inclined to agree with all of @robwhess's remarks in regards to backwards compatibility. I think we should make a reasonable effort to maintain backwards compatibility for very public interfaces like prototxts, etc., but I'm less inclined to worry about people who have implemented their own layers in their private branches having their rebases be effortless. Especially in the case of a change like this where a bit of effort in the short term makes your life easier in the long run.

In lieu of the level of deprecation this PR proposes, @robwhess 's proposal to replace the implementations to the existing caffe_*() math functions with calls to the Device methods sounds reasonable and wouldn't bother me if it existed, but I personally don't think even that's necessary -- maybe others can chime in with their opinions...

[shelhamer]

I'm down with @robwhess's and @jeffdonahue's assessment. On with the new
and improved interface!

Le mardi 8 juillet 2014, Jeff Donahue [email protected] a écrit :

I'm inclined to agree with all of @robwhess https://github.com/robwhess's
remarks in regards to backwards compatibility. I think we should make a
reasonable effort to maintain backwards compatibility for very public
interfaces like prototxts, etc., but I'm less inclined to worry about
people who have implemented their own layers in their private branches
having their rebases be effortless. Especially in the case of a change like
this where a bit of effort in the short term makes your life easier in the
long run.

In lieu of the level of deprecation this PR proposes, @robwhess
https://github.com/robwhess 's proposal to replace the implementations
to the existing caffe_*() math functions with calls to the Device methods
sounds reasonable and wouldn't bother me if it existed, but I personally
don't think even that's necessary -- maybe others can chime in with their
opinions...

—
Reply to this email directly or view it on GitHub
#587 (comment).

[kloudkl]

I mistakenly applied some of the large scale software engineering principles and should stop worrying too much about the backward compatibility.

[jeffdonahue]

I don't want to discourage you from raising such issues in the future -- all other things being equal it's of course better to keep all public facing methods backwards compatible rather than arbitrarily breaking them all the time. We just have to weigh the drawbacks of breaking a particular interface (other devs have to put in some amount of extra effort if they want to use their unofficial extensions with the latest version of Caffe) against the benefits (maintaining backward compatibility creates an additional burden for future development; breaking it in this case encourages other devs to learn how the new interface works, in turn speeding up their future development). For me personally I have quite a bit of non-public Caffe-extending code that this will break, and yet I'm almost excited to rebase that code once this is integrated due to the massive simplification that will result.

In generally we might prioritize backwards compatibility lower than some open source projects because none of us (the core Caffe devs/maintainers at Berkeley) are able to focus on software engineering issues associated with Caffe as a full time job; rather we are all using it mainly to accomplish research objectives. I'd guess (or hope) that as the project matures we'll converge closer and closer to having "ideal" interfaces and begin to see far fewer of these major breaking changes that touch huge parts of the codebase, at which point maintaining backwards compatibility will likely create less maintenance burden and therefore potentially become a higher priority.

[robwhess]

Note that this PR's history is now somewhat messy because I rebased on dev, but the device-abstraction branch is based off an older dev.

@shelhamer, @jeffdonahue, would it make sense to just close out this PR and start a new one directly into dev? I'm ending up doubting that I'll divide my work here into multiple PRs into device-abstraction anyway, and that way I can avoid relying on a BVLC maintainer rebasing device-abstraction on dev to avoid an ugly commit history here each time I rebase on dev.

[shelhamer]

The workflow is you rebase on dev at your fork then we force push the
rebase to BVLC:device-abstraction and in so doing update the PR.

Make sense?

I commented somewhere with the details earlier. Sorry I can't find it out.
Thanks for working on this!

Le mercredi 23 juillet 2014, Rob Hess [email protected] a écrit :

Note that this PR's history is now somewhat messy because I rebased on dev,
but the device-abstraction branch is based off an older dev.

@shelhamer https://github.com/shelhamer, @jeffdonahue
https://github.com/jeffdonahue, would it make sense to just close out
this PR and start a new one directly into dev? I'm ending up doubting
that I'll divide my work here into multiple PRs into device-abstraction
anyway, and that way I can avoid relying on a BVLC maintainer rebasing
device-abstraction on dev to avoid an ugly commit history here each time
I rebase on dev.

—
Reply to this email directly or view it on GitHub
#587 (comment).

[robwhess]

@shelhamer Oh, right, in #610: #610 (comment). That's where we'll have a clean history into dev.

[robwhess]

@shelhamer @jeffdonahue the rebase on dev is complete, and I think this would be a good checkpoint to merge back into device-abstraction.

As an aside, are we wedded to using rebase to pull changes to dev back into a feature branch instead of merge? When larger changes have been incorporated into dev (as seems to happen frequently with Caffe given its large, active development community), rebasing a feature branch can be incredibly, and in my opinion needlessly, complex.

Case in point: I spent 3 days sussing out this rebase, and when I finished and went back and checked the diff with dev, I realized using merge to reincorporate dev instead of rebase would have taken just a few hours (or less) because the changes to dev from my feature branch were, as a whole, conceptually very simple. The difficulty of the rebase lay in the sequencing of the commits. This has been my experience with rebase in general, both in this project and elsewhere.

Is it worth the significant extra work of a rebase to maintain what seems to me like a marginally cleaner commit history? I'd argue that a few extra merge entries in the history is very much worth the time and effort saved by just using merge to incorporate changes. I'd be curious to hear your thoughts on this though. @Yangqing, I'd be curious to hear yours, too, if you want to weigh in.

[bhack]

I'm for rebase in small and medium impact PR and for merge for large PR that often become feature branches in Caffe pipeline. See also some pro and cons here

[jeffdonahue]

@robwhess I definitely sympathize with your comments on rebase -- I don't know if I've ever spent 3 days on one, but certainly several hours and probably a day.

Personally my feeling is that the cleaner linear history is worth a bit of effort, but almost certainly not worth the amount you had to put in. I wouldn't mind having a couple merge commits in a PR this size if it makes your life that much easier, but @shelhamer is our resident git expert and I'm not sure he would agree.

There are a couple things you can do to make your life substantially easier when rebasing. One is to "squash" your history into a smaller number of commits (reordering related commits to combine together, when necessary), so that you're not repeatedly resolving what's essentially the same conflict. This is pretty easy to do with git interactive rebase (git rebase -i), see [1] for more info.

Another thing you can do starting now is use git rerere [2] to remember your conflict resolutions so that the next time you rebase you don't have to resolve the same conflicts.

[1] http://git-scm.com/book/en/Git-Tools-Rewriting-History
[2] http://git-scm.com/blog/2010/03/08/rerere.html

[jeffdonahue]

(and thanks for all the laborious work! force pushed to BVLC device-abstraction.)

[robwhess]

Thanks @jeffdonahue. I'll read those references. I think there were two main issues with this rebase: it'd been quite a while since the last rebase, so dev had diverged quite a bit from where it had been; and there were some conflicting architectural changes between my branch and dev (mainly the UVA stuff merged in #555 and #633). I'll try to stay more on top of things so the rebases aren't this big in the future.

[shelhamer]

@robwhess thanks for all of your effort and the involved rebase!

As the local git fanatic and survivor of many rebases myself, let me explain our policy and give a little advice:

• Rebasing is a chance to clarify, and not only integrate. Interactive rebasing and rewriting commit messages makes life easier on the future.
• Rebasing encourages closely tracking development, since only a few changes need to be resolved at a time. This lets the feature coordinate with the rest of the code in development.
• Merges from divergent branches can be overwhelming and hide complexity. The diff when resolving conflicts all at once can be a bit much. Rebasing commit by commit offers closer inspection of the changes as they develop. In my opinion this is a helpful reminder of the code as a whole and lets me make more informed local decisions.
• The linear history does make it easier to understand the story. Circular merges make the history confusing. In a project like ours with so many threads of development I find a lot of value in keeping the different branches clear.
• @jeffdonahue has already given two excellent suggestions for the rebase lifestyle. I'd like to second that PR time is a the perfect point to do an interactive rebase to streamline your contribution before a rebase to dev for integration.

As @jeffdonahue suggested and you concluded, the best course is to closely track development and routinely rebase. Granted the current pace of Caffe hacking can make this interesting, and sometimes changes like my #555 come along that throw a further wrench into it. Thanks for pushing through it.

That said, rebases can be painful when there is divergence or several branches are integrated all at once. See #167 for my personal favorite. If a particularly mind-bending rebase comes up again, we can discuss the best way to resolve it.