Apache Ignite Dataset #22210

dmitrievanthony · 2018-09-11T10:10:26Z

This is a proposal to add IgniteDataset that allows to work with Apache Ignite.

Apache Ignite is a memory-centric distributed database, caching, and processing platform for transactional, analytical, and streaming workloads, delivering in-memory speeds at petabyte scale. This proposal is a part of a more global initiative to implement so called "TensorFlow on Apache Ignite" (IGNITE-8335, Design Document).

The integration is based on Apache Ignite Binary Client Protocol and TensorFlow tf.data.Dataset. More information about supported features you can find in README.md of this module.

Tests have also been added. They use docker to hide configuration complexity, so that the implemented functionality can be tested quite simply by manual run.

It's a copy of #21853, because previous request has accidentally been closed because I cleaned up merge commits from the branch.

…e variables to satisty code style, use pointers instead of references.

dmitrievanthony · 2018-09-11T10:21:45Z

Hello @mrry, @martinwicke, @perfinion. Sorry for confusion, I've recreated #21853 here. The current state is:

I've fixed all comments from @mrry.
During last CI run only Windows builds and Clang checks failed.
I've fixed Clang checks.

Regarding Windows builds I see the same issues I had and asked here: https://groups.google.com/a/tensorflow.org/forum/#!topic/build/ePYss0Kxcu4. It looks like we need to add copt=-DWIN32_LEAN_AND_MEAN on CI server for Windows builds.

Guys, let me ask you to continue review.

mrry

Note that many of the style/performance/correctness comments apply generally across the code, but I've only commented at the first instance.

mrry · 2018-09-11T14:20:30Z

tensorflow/contrib/ignite/__init__.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+"""Apache Ignite is a memory-centric distributed database, caching, and


Style nit: all module, class, and function docstrings should begin with a one-line summary.

I updated docs and added a short description in the first line.

mrry · 2018-09-11T14:21:49Z

tensorflow/contrib/ignite/kernels/ignite_binary_object_parser.cc

+limitations under the License.
+==============================================================================*/
+
+#include "ignite_binary_object_parser.h"


Style nit: use the full absolute path to included headers in the same project.

mrry · 2018-09-11T14:23:40Z

tensorflow/contrib/ignite/kernels/ignite_binary_object_parser.cc

+
+Status BinaryObjectParser::Parse(uint8_t** ptr,
+                                 std::vector<Tensor>* out_tensors,
+                                 std::vector<int32_t>* types) {


The types argument seems to be unused, except in what seems to be a recursive call to this method. Delete it if it's unused.

Yes, excellent point, I completely forgot about it. The reason of types to be here is to build a schema of every object that we receive and check it.

I added this logic today, so it affects this code and CheckTypes method in IgniteDatasetIterator. Please take a look.

mrry · 2018-09-11T14:25:00Z

tensorflow/contrib/ignite/kernels/ignite_binary_object_parser.h

+==============================================================================*/
+
+#include <vector>
+#include "tensorflow/core/framework/dataset.h"


This header seems to be unused. If you depend on other headers indirectly, include them directly here or (preferably) in the .cc file.

Updated, look like the following two headers are enough:

#include "tensorflow/core/framework/tensor.h" #include "tensorflow/core/lib/core/status.h"

mrry · 2018-09-11T14:26:51Z

tensorflow/contrib/ignite/kernels/ignite_binary_object_parser.cc

+      int32_t length = *((int32_t*)*ptr);
+      *ptr += 4;
+      Tensor tensor(cpu_allocator(), DT_STRING, {});
+      tensor.scalar<std::string>()() = std::string((char*)*ptr, length);


Use tensorflow::string instead of std::string everywhere. Some platforms use a different string implementation.

mrry · 2018-09-11T15:43:25Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+    """
+    if self.fields is None:
+      object_type = types[self.type_id]
+      if object_type is not None:


object_type can never be None, because None is not a value in types.

Good point, I added check like:

if self.type_id in types:

Done.

mrry · 2018-09-11T15:44:07Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+        if is_array:
+          return tensor_shape.TensorShape([None])
+        return tensor_shape.TensorShape([])
+      raise Exception("Unsupported type [type_id=%d]" % self.type_id)


Use ValueError instead of Exception.

Replaced (where it's appropriate).

mrry · 2018-09-11T15:44:48Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+    return self.to_flat_rec([])
+
+  def to_permutation(self):
+    """Returns a permutation that should be applied to order object leafs."""


Typo: s/leafs/leaves/

Thanks, fixed.

mrry · 2018-09-11T15:48:55Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+        name="page_size")
+    self.username = ops.convert_to_tensor("" if username is None else username,\
+        dtype=dtypes.string, name="username")
+    self.password = ops.convert_to_tensor("" if password is None else password,\


This (and self.cert_password) will encode potentially sensitive information in the GraphDef and send it over insecure channels. Is it necessary to include this information here?

It's not necessary, but it's the simple way to start using.

We provide two ways to specify these parameters: via parameters of dataset or via environment variables on the nodes where dataset will be actually instantiated. As far as I understand, in the second case sensitive information won't be included into GraphDef and passed via insecure channels.

Perhaps we should only support the environment variable, in that case? I'm concerned that novice users won't understand the distinction, take the simple route, and leak sensitive information.

However, security isn't my domain, so @martinwicke can you please make a determination here?

mrry · 2018-09-11T15:49:57Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+    """
+    super(IgniteDataset, self).__init__()
+
+    with IgniteClient(host, port, username, password, certfile, keyfile,\


There is no need for a line continuation character here (or in the cases below) because it is implied by the open parentheses.

Thanks, fixed.

dmitrievanthony · 2018-09-12T18:33:37Z

Hi @mrry. Thank you for very detailed and deep review, I really appreciate it. I think I fixed all your comments today, so could you please have a look my changes?

dmitrievanthony · 2018-09-13T09:46:01Z

Meanwhile, @martinwicke, @perfinion, could you please rerun tests? I made a lot of changes, would be great to check CI statuses.

yongtang · 2018-09-13T12:57:15Z

: added kokoro:run label to help start the tests.

yongtang · 2018-09-13T13:22:40Z

@dmitrievanthony The test failure for Experimental clang-format Check is related to clang-format. You can use clang-format -is --style=google <filename.cc> to fix them.

However, different clang-format versions may generate different outputs. I think you could use clang-format bundled with Ubuntu 16.04 to get the outputs that matches the Experimental clang-format Check.

If you have Docker installed on your machine, I think you may use:

docker run -i -t --rm \
    -v $PWD:/tensorflow -w /tensorflow --net=host ubuntu:16.04 \
    sh -c 'apt-get -y update && apt-get -y install clang-format && clang-format -i --style=google tensorflow/contrib/ignite/kernels/ignite_byte_swapper.h'

to format the ignite_byte_swapper.h that is consistent with Experimental clang-format Check.

dmitrievanthony · 2018-09-17T15:31:25Z

Hi @mrry. It's been 6 days since I fixed all review comments. Could you please take a look?

mrry · 2018-09-18T15:27:51Z

tensorflow/contrib/ignite/kernels/ignite_byte_swapper.h

+ public:
+  ByteSwapper(bool big_endian) {
+    int x = 1;
+    bool is_little_endian = (*(char *)&x == 1);


Use the formulation here to work out the endianness statically:

tensorflow/tensorflow/compiler/xla/literal.cc

Line 46 in 25c9913

constexpr bool kLittleEndian = __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__;

I updated it similar way.

mrry · 2018-09-18T15:44:46Z

tensorflow/contrib/ignite/python/ops/ignite_dataset_ops.py

+        name="page_size")
+    self.username = ops.convert_to_tensor("" if username is None else username,\
+        dtype=dtypes.string, name="username")
+    self.password = ops.convert_to_tensor("" if password is None else password,\


Perhaps we should only support the environment variable, in that case? I'm concerned that novice users won't understand the distinction, take the simple route, and leak sensitive information.

However, security isn't my domain, so @martinwicke can you please make a determination here?

dmitrievanthony · 2018-09-18T16:17:21Z

Regarding security question, @mrry, @martinwicke, I think we can add warning in case user specifies sensitive data via parameters. What do you think?

martinwicke · 2018-09-18T16:38:12Z

I think we should make it hard for credentials to end up in files or on the network in the clear. Therefore, I would prefer if we insist here that all credentials are present on the executing machine already, and we make it impossible to use credentials that are stored in the graph.

I think environment variables would work fine in this case, can we restrict it to that for all sensitive information?

dmitrievanthony · 2018-09-18T18:30:08Z

I agree, it's reasonable, @mrry, @martinwicke. I updated code so that sensitive information is not encoded in graph. Be aware that it's still used in python for initial access (that is required to get data schema), but after that only environment variables are used.

dmitrievanthony · 2018-09-18T19:34:43Z

Do we have any open questions, @mrry?

mrry

Let's go ahead and merge this.

dmitrievanthony · 2018-09-24T09:18:31Z

Guys, looks like I introduced a bug on Windows when changed ignite_byte_swapper.h. It's fixed now, so please rerun tests who can do it.

Also, as we found out previously, it was a bug in master that leads to Windows GPU build failure. See #22210 (comment):

@dmitrievanthony I created a PR #22258 to fix the Windows GPU build failure issue.

Should I merge master into my branch? Or it's fine that Windows GPU build is broken by non-related to my code reason?

yongtang · 2018-09-24T13:15:16Z

@dmitrievanthony The windows GPU fix has been submitted internally so the build should pass. Let me help with rerun the test.

dmitrievanthony · 2018-09-24T14:40:21Z

Ok, @mrry, @martinwicke, @yongtang, all tests I expected to pass actually passed. XLA fails, but it fails all the time by reasons that are not related to my code.

Derek, sorry for bothering you again, but we need you approval again. After that this PR can be merged, right?

mrry

Re-approving. There are a few more internal steps required before the PR is merged, but you shouldn't need to do anything else from this point.

dmitrievanthony · 2018-09-28T09:31:48Z

Hi, @mrry, @martinwicke. I'm not sure how long it takes to pass all internal steps, but it looks like it takes time. Meanwhile, as far as I see, there are several conflicts appeared. Shall I fix them?

mrry · 2018-09-28T21:25:37Z

No need to update the branch. We're in the process of getting it to merge internally.

FYI: You should check the diff between what we end up merging, and the original PR. Our internal checks seem to be more picky about style and other issues than the presubmit. It has also been necessary to disable the SSL tests and remove the checked-in private key file.

PiperOrigin-RevId: 215258743

dmitrievanthony added 6 commits September 11, 2018 10:01

Add IgniteDataset that allows to work with Apache Ignite.

8530167

Remove duplicated header from README.md.

28b0608

Update after review: change 'ignite' namespace to 'tensorflow', renam…

241c174

…e variables to satisty code style, use pointers instead of references.

Update README.md.

1408a15

Fix pylint checks, fix VS compilation issue.

9201976

Fix code style.

0b6654b

dmitrievanthony requested a review from mrry as a code owner September 11, 2018 10:10

googlebot added the cla: yes label Sep 11, 2018

dmitrievanthony mentioned this pull request Sep 11, 2018

Apache Ignite Dataset #21853

Closed

This was referenced Sep 11, 2018

RFC: Sunset tf.contrib tensorflow/community#18

Merged

Apache Ignite File System #22194

Merged

perfinion added the kokoro:force-run Tests on submitted change label Sep 11, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 11, 2018

perfinion requested a review from martinwicke September 11, 2018 14:11

mrry suggested changes Sep 11, 2018

View reviewed changes

tensorflowbutler assigned yifeif Sep 12, 2018

Fixes after second review.

9ec9c8b

dmitrievanthony added 2 commits September 13, 2018 11:24

Add forgotten ignite_byte_swapper.h

ce9b230

Fix windows build.

d797e99

yongtang added the kokoro:force-run Tests on submitted change label Sep 13, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 13, 2018

yongtang added kokoro:run kokoro:force-run Tests on submitted change labels Sep 13, 2018

kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Sep 13, 2018

mrry reviewed Sep 18, 2018

View reviewed changes

Work out the endianness statically.

6d67ba4

Avoid saving sensitive information in graph.

14e9345

mrry previously approved these changes Sep 23, 2018

View reviewed changes

martinwicke added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 24, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 24, 2018

Fix clang styles.

8f4ded5

dmitrievanthony dismissed mrry’s stale review via 8f4ded5 September 24, 2018 08:17

Fix byte-order issue.

90c6877

yongtang added kokoro:run kokoro:force-run Tests on submitted change labels Sep 24, 2018

kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Sep 24, 2018

mrry approved these changes Sep 24, 2018

View reviewed changes

tensorflow-copybara merged commit 90c6877 into tensorflow:master Oct 1, 2018

tensorflow-copybara pushed a commit that referenced this pull request Oct 1, 2018

Merge pull request #22210 from dmitrievanthony:apache-ignite-dataset

61a8720

PiperOrigin-RevId: 215258743

dmitrievanthony mentioned this pull request Oct 2, 2018

Apache Ignite Dataset: Fixes merge artifacts #22671

Merged

dmitrievanthony mentioned this pull request Oct 16, 2018

Add Apache Arrow Support to TensorFlow Dataset #23002

Closed

dmitrievanthony mentioned this pull request Jan 23, 2019

Added release notes for 1.13 release #25084

Merged

Apache Ignite Dataset #22210

Apache Ignite Dataset #22210

Conversation

dmitrievanthony commented Sep 11, 2018

dmitrievanthony commented Sep 11, 2018

mrry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrievanthony commented Sep 12, 2018

dmitrievanthony commented Sep 13, 2018

yongtang commented Sep 13, 2018

yongtang commented Sep 13, 2018

dmitrievanthony commented Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrievanthony commented Sep 18, 2018

martinwicke commented Sep 18, 2018

dmitrievanthony commented Sep 18, 2018

dmitrievanthony commented Sep 18, 2018

mrry left a comment

Choose a reason for hiding this comment

dmitrievanthony commented Sep 24, 2018 • edited

yongtang commented Sep 24, 2018

dmitrievanthony commented Sep 24, 2018

mrry left a comment

Choose a reason for hiding this comment

dmitrievanthony commented Sep 28, 2018

mrry commented Sep 28, 2018

dmitrievanthony commented Sep 17, 2018 •

edited

dmitrievanthony commented Sep 24, 2018 •

edited