Adding support for Big Endian in decode_raw_op_test

[Nayana-ibm]

Adding support for Big Endian in decode_raw_op_test

[tensorflow-jenkins]

Can one of the admins verify this patch?

[Nayana-ibm]

Decode_raw_op test fails on big endian with an error "Unimplemented support for little_endian=true".
As per comments, we have made changes in decode_raw and created a new PR for ongoing discussion on this.

[rohan100jain]

I took a quick look at this PR and my only concern is that there doesn't seem to be any handling of the little_endian_ attribute in the OP code. What if little_endian_ is false (indicating that the data is little endian) but the system is different (say big endian). I think that case isn't perhaps handled. If the type of the data and system match then I think this code should do the job.

Adding Andrew as well.

[Nayana-ibm]

@rohan100jain @drpngx Other solution we could think is to add check for little endian platform.
Below is the patch:

--- a/tensorflow/core/kernels/decode_raw_op.cc
+++ b/tensorflow/core/kernels/decode_raw_op.cc
@@ -69,6 +69,15 @@ class DecodeRawOp : public OpKernel {
         context, context->allocate_output("output", out_shape, &output_tensor));
     auto out = output_tensor->flat_inner_dims<T>();
     DCHECK_EQ(flat_in.size(), out.dimensions()[0]);
+
+    if (::tensorflow::port::kLittleEndian) {
      OP_REQUIRES(
        context,
        little_endian_ == ::tensorflow::port::kLittleEndian || sizeof(T) == 1,
        errors::Unimplemented("Unimplemented support for little_endian=",
                              little_endian_ ? "true" : "false"));
+    }
   // Endianness matches, so just copy each string byte-for-byte.

[drpngx]

Patch look strange, there's a sizeof = 1 that's not used because of the if conditional?

[Nayana-ibm]

@drpngx
Decode_raw_op test was failing on big endian due to following assert:

OP_REQUIRES(
        context,
        little_endian_ == ::tensorflow::port::kLittleEndian || sizeof(T) == 1,
        errors::Unimplemented("Unimplemented support for little_endian=",
                              little_endian_ ? "true" : "false"));

With our PR, we tried to remove the above assert assuming that type of data and the system match.
However, as @rohan100jain commented, we could think of above patch. Through if condition, assert will be skipped only on big endian and would not hamper original functionality.

Can you think of better way to handle the above scenario?

[drpngx]

The test basically says that it's not implemented. Maybe it's stale. Does it pass all tests?

[Nayana-ibm]

@drpngx All tests are passing at our end with above mentioned patch.
Should we modify PR so that you could trigger a build?

[drpngx]

Jenkins, test this please.

[drpngx]

Not sure if @aselle has more context. The test was written to mean that anything other than little endian doesn't work, but I guess if it works for you, then I am for removing the check altogether, if it also has a reasonable chance of working with middle endian.

[Nayana-ibm]

@aselle @drpngx Please let me know your comments.

[drpngx]

Sorry for the delay. Thinking about this, it's the right thing to delete the test since you have tested it and it's obsolete. The case we don't cover is middle endian but I don't think we need to worry about this now.