BVLC
De-duplicate Engine Tests
A not insignificant amount of the cuDNN integration diff was testing, but these tests are mostly mirrors of the Caffe standard tests with the engine swapped between CAFFE and CUDNN. These should be de-duplicated by a TestDtypesAndDevicesAndEngines scheme or simply a vector of the two layer objects for clarity and ease of maintenance.
The increased test time caused by the added (exploding) combination is perhaps not a big deal. But the smell of the code suggests that refactoring is in need. If completed by someone, #610 and #1064 may provide a more thorough solution.
There are two or three variants, i.e. CPU, CUDA and cuDNN, with the same functionalities for some layers. There will certainly be more in the future. To be flexible and easily extensible, the implementations of the same layer should be split into separate classes inheriting a common abstract base class. For example, AbstractConvolutionLayer is inherited by CPUConvolutionLayer, CUDAConvolutionLayer and CuDNNConvolutionLayer. The modularity of the layers can also be achieved by other designs.
Each family of layers could be produced by the corresponding CPU, CUDA and CuDNN LayerFactory. There are layers that only have CPU version or CPU plus CUDA version. To avoid having to manually track which layer have what versions which is cumbersome, the CuDNNLayerFactory had better subclass CUDALayerFactory which then inherits CPULayerFactory. The subclasses only override the methods to produce the layers that are implemented in the specific flavors.
While the full abstract class and inherited strategies design has its merits, and is actually how I first developed the engines, it was rejected because
- engines are not completely exchangeable, and a particular implementation might lack support for a given configuration e.g. cuDNN can't pool with padding
- by inheriting from the standard Caffe implementation, and always setting it up, the engine call fallback to Caffe as done by cuDNN layers in CPU mode.
I could imagine a factory and strategy design exists to handle this, but deferred it for this practicality. Good to keep in mind improved designs all the same.
For the tests, de-duplication could come through pulling the test method bodies out of the test classes and giving each a Layer arg for passing in the different engine (sub)classes.
On Saturday, September 20, 2014, kloudkl [email protected] wrote:
The increased test time caused by the added (exploding) combination is perhaps not a big deal. But the smell of the code suggests that refactoring is in need. If completed by someone, #610 https://github.com/BVLC/caffe/pull/610 and #1064 https://github.com/BVLC/caffe/pull/1064 may provide a more thorough solution.
There are two or three variants, i.e. CPU, CUDA and cuDNN, with the same functionalities for some layers. There will certainly be more in the future. To be flexible and easily extensible, the implementations of the same layer should be split into separate classes inheriting a common abstract base class. For example, AbstractConvolutionLayer is inherited by CPUConvolutionLayer, CUDAConvolutionLayer and CuDNNConvolutionLayer. The modularity of the layers can also be achieved by other designs.
Each family of layers could be produced by the corresponding CPU, CUDA and CuDNN LayerFactory. There are layers that only have CPU version or CPU plus CUDA version. To avoid having to manually track which layer have what versions which is cumbersome, the CuDNNLayerFactory had better subclass CUDALayerFactory which then inherits CPULayerFactory. The subclasses only override the methods to produce the layers that are implemented in the specific flavors.
— Reply to this email directly or view it on GitHub https://github.com/BVLC/caffe/issues/1108#issuecomment-56274226.
This is related to the issues that are brought up in "Set mode cpu fails?" #3317 (though, this of course, predates that by a lot). It's a shame that "Device Abstraction" #610 was abandoned, because it would resolve a lot of this.
I agree that test duplication across engine variants adds unnecessary maintenance overhead and obscures the core logic being tested. Refactoring toward a shared test harness that accepts layer instances as parameters seems like a pragmatic step forward. While full abstraction via factories and inheritance may be ideal long-term, even partial consolidation of test logic would improve clarity and reduce fragility. Happy to help explore a minimal working example if others are interested.
