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

Add CNTK as keras backend #6800

Merged
merged 101 commits into from Jun 7, 2017

Conversation

Projects
None yet
10 participants
@souptc
Contributor

souptc commented May 30, 2017

This is the Beta version of CNTK as keras backend. Those changes are suppose to work with CNTK v2.0 GA. Because the CNTKv2.0 is not officially published, you could use the wheel below to have a first try:

http://cntk.ai/PythonWheel/ForKeras/cntk-2.0rc3-cp35-cp35m-linux_x86_64.whl
http://cntk.ai/PythonWheel/ForKeras/cntk-2.0rc3-cp27-cp27mu-linux_x86_64.whl

Most of the keras features are supported, except:

  1. Performance optimization on CPU device.
    CNTK performance optimization on cpu device is not finished, so if you want better performance, please try to run CNTK_Keras with GPU device. 
     
  2. Gradient as symbolic ops.
    This is not include in CNTK_Keras beta release, and we will support it in CNTK_Keras GA release. 
     
  3. Stateful recurrent layer.
    This is not include in CNTK_Keras beta release. 
     
  4. Masking on recurrent layer.
    This is not include in CNTK_Keras beta release, and we will support it in CNTK_Keras GA release. 
     
  5. Padding with non-specified shape. 
    Padding with non-speicfied shape is not supported now. To using cntk backend in keras with padding, please specified the concrete input shape. 
     
  6. Convolution with dilation.
    This is not include in CNTK_Keras beta release, and we will support it in CNTK_Keras GA release. 
     
  7. Randomness op across batch axis. 
    This is not include in CNTK_Keras beta release, and we will support it in CNTK_Keras GA release. 
     
  8. Some Keras backend apis: reverse / top_k / ctc / map / foldl / foldr. 
    These are not include in CNTK_Keras beta release, and we will support it in CNTK_Keras GA release. 
@souptc

This comment has been minimized.

Contributor

souptc commented Jun 3, 2017

@fchollet , I just update the implementation which resolve the most comments. For the "bias_shape", I explained the purpose of the change, do you have any suggestion about a better solution?

@souptc

This comment has been minimized.

Contributor

souptc commented Jun 3, 2017

And for the maintainance, yes, CNTK team will definitely commit to maintain the cntk backend in the future.

chentaMS and others added some commits Jun 5, 2017

Add sparse_top_k_categorical_accuracy and test code (#6840)
* Add top_k_sparse_categorical_accuracy and test_top_k_sparse_categorical_accuracy

* Rename top_k_sparse_categorical_accuracy and sparse_top_k_categorical_accuracy
@fchollet

The file cntk_backend will require some cleanups and a style normalization. E.g. it mixes different quote characters for string delimitation, and other small style issues.

LICENSE Outdated
@@ -8,6 +8,10 @@ All contributions by Google:
Copyright (c) 2015, Google, Inc.
All rights reserved.
All contributions by Microsoft:
Copyright (c) 2015, Microsoft, Inc.

This comment has been minimized.

@fchollet

fchollet Jun 5, 2017

Collaborator

This should say "2017", I believe

This comment has been minimized.

@souptc

souptc Jun 6, 2017

Contributor

thanks, fixed.

@@ -3334,13 +3334,17 @@ def pool3d(x, pool_size, strides=(1, 1, 1), padding='valid',
return _postprocess_conv3d_output(x, data_format)
def bias_add(x, bias, data_format=None):
def bias_add(x, bias, data_format=None, bias_shape=None):

This comment has been minimized.

@fchollet

fchollet Jun 5, 2017

Collaborator

Wouldn't the bias argument already have a shape, that you could just read?

@souptc

This comment has been minimized.

Contributor

souptc commented Jun 6, 2017

clean the cntk_backend file with style issue.

@souptc

This comment has been minimized.

Contributor

souptc commented Jun 6, 2017

fix the bias_shape issue.

@fchollet

Some style nitpicks. Almost ready to merge!

NAME_SCOPE_STACK = []
@contextmanager

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

^M?

@@ -3346,28 +3346,41 @@ def bias_add(x, bias, data_format=None):
Output tensor.
# Raises
ValueError: In case of invalid `data_format` argument.
ValueError: In case of invalid `data_format` argument, or the input bias dimension is not expect

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Can you rephrase this? The meaning is unclear. Additionally, please make sure to introduce line breaks to keep line length reasonable.

if ndim(x) == 5:
if data_format == 'channels_first':
x += reshape(bias, (1, int_shape(bias)[0], 1, 1, 1))
shape = (bias_shape[0], 1, 1, 1) if len(bias_shape) == 1 else (bias_shape[3],) + bias_shape[:3]

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Introduce an if block to avoid a very long line.

elif data_format == 'channels_last':
x += reshape(bias, (1, 1, 1, 1, int_shape(bias)[0]))
shape = (1, 1, 1, bias_shape[0]) if len(bias_shape) == 1 else bias_shape

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Same.

elif ndim(x) == 4:
if data_format == 'channels_first':
x += reshape(bias, (1, int_shape(bias)[0], 1, 1))
shape = (bias_shape[0], 1, 1) if len(bias_shape) == 1 else (bias_shape[2],) + bias_shape[:2]

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Same.

elif ndim(x) == 4:
if data_format == 'channels_first':
x += reshape(bias, (1, bias.shape[0], 1, 1))
shape = (bias_shape[0], 1, 1) if ndim(bias) == 1 else (bias_shape[2],) + bias_shape[:2]

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Line too long

elif data_format == 'channels_last':
x += reshape(bias, (1, 1, 1, bias.shape[0]))
shape = (1, 1, bias_shape[0]) if ndim(bias) == 1 else bias_shape

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Line too long

elif ndim(x) == 3:
if data_format == 'channels_first':
x += reshape(bias, (1, bias.shape[0], 1))
shape = (bias_shape[0], 1) if ndim(bias) == 1 else (bias_shape[1],) + bias_shape[:1]

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Line too long

elif data_format == 'channels_last':
x += reshape(bias, (1, 1, bias.shape[0]))
shape = (1, bias_shape[0]) if ndim(bias) == 1 else bias_shape

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Line too long

output = K.reshape(output,
(self.output_row, self.output_col, -1, filters))
output = K.permute_dimensions(output, (2, 0, 1, 3))
output = K.local_conv2d(inputs, self.kernel, self.kernel_size, self.strides, (self.output_row, self.output_col), self.data_format)

This comment has been minimized.

@fchollet

fchollet Jun 6, 2017

Collaborator

Line too long

@fchollet

This comment has been minimized.

Collaborator

fchollet commented Jun 7, 2017

The failing test is a flake.

@fchollet fchollet merged commit 82832a3 into keras-team:master Jun 7, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@fchollet

This comment has been minimized.

Collaborator

fchollet commented Jun 7, 2017

I have applied a few style fixes to the backend source and merged. Thanks a lot! 👍

While going through the backend code, I have noticed that a lot of the error messages raised were not as helpful and as clear as they should be. I suggest you go over the error messages and improve them. Typically you want to tell the user: 1) what they did (e.g. print the arguments they passed), 2) why that was wrong (e.g. something not supported), and 3) what they should do instead. Currently you are mostly doing 2) only.

This will greatly improve the user experience for CNTK Keras users, and improve the usability / ease of debugging of Keras models on CNTK.

@souptc

This comment has been minimized.

Contributor

souptc commented Jun 7, 2017

Thanks François! Sure, I will go though the message tomorrow and improve them.

@ebarsoumMS

This comment has been minimized.

ebarsoumMS commented Jun 7, 2017

Thanks, everybody...

@fchollet

This comment has been minimized.

Collaborator

fchollet commented Jun 7, 2017

Huge thanks to everyone who contributed to this project! It's a big milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment