WIP: Support NCHW (for conv2d). #8021

TimZaman · 2017-09-29T17:40:29Z

Tensorflow (at least versions 1.3 and below) does not support NCHW computations of many ops when run on the CPU. That creates compatibility issues. These are originally mitigated in Keras, by doing a "NHWC roundtrip" and any op requested to be ran in "channels_first" (NCHW) would actually be transposed to- and from- NHWC.

NCHW often has performance benefits; for example, cuDNN is often fastest when the largest dimension is last (favouring NCHW for images).

The current implementation probes if NCHW is available by checking:

Whether NCHW (channels_first) is requested at all
If NCHW is supported by checking (1) if a GPU is available at all and (2) if it is not explicitly set on CPU.

If the op is set explicitly on the CPU, or there is no GPU device, and if NCHW is requested, we will proceed with the transpose roundtrip as we always did in Keras.

This PR only patches the conv2d op. There are more (and more significant) ops to be patched: any op that uses _postprocess_conv2d_output or _postprocess_conv3d_output ad bias_add too. The latter is actually Keras's bottleneck when it comes to NCHW. Lets save those for distinct PRs.

fchollet

Thanks for the PR!

fchollet · 2017-09-29T22:53:22Z

keras/backend/tensorflow_backend.py

+
+
+def get_current_tf_device():
+    """Return device string of current graph context that's explicitly, otherwise returns `None`."""


Please fix docstring typos

fchollet · 2017-09-29T22:54:38Z

keras/backend/tensorflow_backend.py

+        self.device = device
+
+
+def get_current_tf_device():


I think we should make this function private, as well as is_current_explicit_device, and get_available_gpus.

fchollet · 2017-09-29T22:54:58Z

keras/backend/tensorflow_backend.py

+
+def is_current_explicit_device(device_type):
+    """
+    Check if the current device is explicitly set on the device type.


Please put the docstring description on the first line

fchollet · 2017-09-29T22:56:36Z

keras/backend/tensorflow_backend.py

+    return [x.name for x in LOCAL_DEVICES if x.device_type == 'GPU']
+
+
+def has_nchw_support():


This sounds too specific. Why not something like "_running_on_gpu()" or something to that extent?

Hmm.. What it checks for is if the current scope is "not explicit on CPU, and has GPUs available". It's called like this to anticipate any TF 1.x's support that does not need the roundtrip so we can test for the tf version in the function.

Dref360 · 2017-10-01T17:40:40Z

Could we have some tests where we mock being on a gpu machine so that some tests run using NHWC and some run using NCWH? We could test with tf.device('/cpu:0'): etc.

TimZaman · 2017-10-01T18:39:10Z

Sure @Dref360 , that does require ignoring tests if people/travis does not have a GPU though. Do we already have tests in Keras that skip tests for environments where there's no GPU?

fchollet · 2017-10-02T22:00:39Z

keras/backend/tensorflow_backend.py

+    return op.device
+
+
+def is_current_explicit_device(device_type):


Please make this method private.

fchollet · 2017-10-02T22:00:44Z

keras/backend/tensorflow_backend.py

+    return (device is not None and device.device_type == device_type.upper())
+
+
+def get_available_gpus():


Please make this method private (unless there is a rationale for making it part of the public API).

fchollet · 2017-10-02T22:00:48Z

keras/backend/tensorflow_backend.py

+    return [x.name for x in LOCAL_DEVICES if x.device_type == 'GPU']
+
+
+def has_nchw_support():


Please make this method private.

fchollet · 2017-10-02T22:01:06Z

keras/backend/tensorflow_backend.py

@@ -44,6 +45,13 @@
 # Change its value via `manual_variable_initialization(value)`.
 _MANUAL_VAR_INIT = False

+# This map is for converting the keras data format string to that of TF.
+DATA_FORMAT_MAP = {'channels_first': 'NCHW', 'channels_last': 'NHWC'}


Please make this global variable private.

fchollet · 2017-10-02T22:01:10Z

keras/backend/tensorflow_backend.py

+
+# This list queries the devices.
+# We assume our devices don't change during our lifetime.
+LOCAL_DEVICES = device_lib.list_local_devices()


Please make this global variable private.

fchollet

LGTM, thanks!

fchollet · 2017-10-03T04:05:17Z

Please fix the docstrings and PEP8 issue reported in CI: https://travis-ci.org/fchollet/keras/builds/282490074

fchollet · 2017-10-03T19:46:46Z

keras/backend/tensorflow_backend.py

+
+    # Returns
+        bool: if the current device scope is explicitly set on the device type.
+    """


Per the failing docstring test, this docstring needs a Raises section mentioning the ValueError: https://travis-ci.org/fchollet/keras/jobs/282558708

fchollet

LGTM, many thanks!

TimZaman · 2017-10-04T18:08:29Z

OK! We're not done yet; we need to patch other ops too, most importantly (biggest bottleneck) is the add_bias's tranpose on NCHW. I'll get to that.

This reverts commit 80dcdcd.

…-outputs * master: (68 commits) Change default value of shuffle parameter of Sequential.fit_generator() from True to False. (keras-team#8075) Fix off-by-one bug in predict/evaluate progress bar (keras-team#8071) Revert "Faster sequence" (keras-team#8060) Support NCHW for conv2d. (keras-team#8021) Change compute_accuracy() argument order and names (keras-team#8049) Replace literal constant 10 with variable num_classes in example/ (keras-team#8041) Faster sequence (keras-team#8039) Improve RNN docs. Enable accuracy reporting during training in examples/mnist_siamese_graph.py (keras-team#7997) Bug fix: Models with shared layers shouldn't be considered Sequential like (keras-team#8025) Add 'subtract' merge layer documentation (keras-team#8038) Update inference in seq2seq script to be more efficient Remove lstm_benchmark from examples/README.md (keras-team#8024) Add shuffle to the Model API (keras-team#8023) Add seq2seq example script. fix travis failure (keras-team#8014) Improve TF backend's Switch function (keras-team#7958) Added support for dynamic noise_shape in Dropout (keras-team#7999) Make on_epoch_end optional (keras-team#8007) Incremental tests speed ups. ...

TimZaman force-pushed the tzaman/nchw-conv2d branch 3 times, most recently from 32b0c6f to 2a12f9d Compare September 29, 2017 17:44

fchollet reviewed Sep 29, 2017

View reviewed changes

TimZaman force-pushed the tzaman/nchw-conv2d branch from 2a12f9d to ecbae72 Compare October 1, 2017 18:38

fchollet reviewed Oct 2, 2017

View reviewed changes

TimZaman force-pushed the tzaman/nchw-conv2d branch from ecbae72 to ee13a1b Compare October 2, 2017 23:17

fchollet previously approved these changes Oct 3, 2017

View reviewed changes

TimZaman dismissed fchollet’s stale review via 0a1063b October 3, 2017 04:11

TimZaman force-pushed the tzaman/nchw-conv2d branch from ee13a1b to 0a1063b Compare October 3, 2017 04:11

fchollet reviewed Oct 3, 2017

View reviewed changes

Support NCHW for conv2d.

87df398

TimZaman force-pushed the tzaman/nchw-conv2d branch from 0a1063b to 87df398 Compare October 4, 2017 03:36

fchollet approved these changes Oct 4, 2017

View reviewed changes

fchollet merged commit 80dcdcd into keras-team:master Oct 4, 2017

Dref360 pushed a commit to Dref360/keras that referenced this pull request Oct 5, 2017

Revert "Support NCHW for conv2d. (keras-team#8021)"

f73c64c

This reverts commit 80dcdcd.

bdwyer2 added a commit to bdwyer2/keras that referenced this pull request Oct 5, 2017

Revert "Support NCHW for conv2d. (keras-team#8021)"

407eb62

This reverts commit 80dcdcd.

datumbox mentioned this pull request Nov 3, 2017

Remove unintended session initialization on TF backend #8377

Merged

jonilaserson mentioned this pull request Jan 29, 2018

NCHW vs NHWC on pretrained network #9234

Closed



		def get_current_tf_device():
		"""Return device string of current graph context that's explicitly, otherwise returns `None`."""

		return [x.name for x in LOCAL_DEVICES if x.device_type == 'GPU']


		def has_nchw_support():

		return op.device


		def is_current_explicit_device(device_type):

		return (device is not None and device.device_type == device_type.upper())


		def get_available_gpus():

WIP: Support NCHW (for conv2d). #8021

WIP: Support NCHW (for conv2d). #8021

Uh oh!

Conversation

TimZaman commented Sep 29, 2017

Uh oh!

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

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!

Dref360 commented Oct 1, 2017

Uh oh!

TimZaman commented Oct 1, 2017

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!

fchollet left a comment

Choose a reason for hiding this comment

Uh oh!

fchollet commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fchollet left a comment

Choose a reason for hiding this comment

Uh oh!

TimZaman commented Oct 4, 2017

Uh oh!

Uh oh!