Apache Ignite File System #22194

dmitrievanthony · 2018-09-10T16:08:18Z

This is a proposal to support Apache Ignite File System in TensorFlow.

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 custom filesystem plugin from TensorFlow side and IGFS Native API from Apache Ignite side. More information about this module you can find in README.md.

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.

Be aware that this pull request is based on previous #22210 that is not merged yet (significant part of the code you see here will be merged in #22210).

googlebot · 2018-09-10T16:08:28Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

yifeif · 2018-09-10T19:38:16Z

Thanks @dmitrievanthony. Could you check the cla for all contributors who have commits in this PR?

dmitrievanthony · 2018-09-10T20:08:16Z

Hi, @yifeif. Thank you for attention. Did I understand correctly from the message above that all authors have signed CLA and the problem is only that the committer and the author of one commit are not the same? If so, I asked @artemmalykh to write here that he's ok with this PR.

artemmalykh · 2018-09-10T20:45:24Z

I'm totally OK with PR!

yifeif · 2018-09-11T05:46:12Z

Thanks @dmitrievanthony and @artemmalykh. Looks good now.

googlebot · 2018-09-11T05:46:26Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2018-09-17T11:09:57Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

dmitrievanthony · 2018-09-17T11:13:41Z

@yifeif, I just rebased this branch, so commits haven't been changed. I think CLA check should be fine. I still waiting #22210 to be merged so I'll make one more rebase when it will be merged and ping you again.

googlebot · 2018-09-27T21:30:01Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2018-10-02T16:51:11Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

dmitrievanthony · 2018-10-02T17:01:23Z

Hi, @mrry, @martinwicke, @yifeif.

After merge of #22210 I rebased and updated this pull request, so now it's ready for review. It's quite big, but less that Ignite Dataset.

Derek, I'll be grateful if you have a look and start reviewing. Please let me know if changes required.

Would be great if someone starts tests and clicks cla:yes (cla checker is too suspicious in case of rebases).

googlebot · 2018-10-02T17:30:27Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

dmitrievanthony · 2018-10-08T14:33:08Z

Hi, @mrry. Any updates?

mrry · 2018-10-12T16:57:15Z

tensorflow/contrib/igfs/__init__.py

+plugged into Hadoop or Spark deployments. This contrib package contains an
+intergration between IGFS and TensorFlow.
+
+@@IGFS


This module doesn't appear to import a symbol called IGFS. Do you need this line?

mrry · 2018-10-12T16:58:21Z

tensorflow/contrib/igfs/kernels/igfs.cc

+
+namespace tensorflow {
+
+string GetEnvOrElse(const string &env, string default_value) {


Mark private functions as static or put them in an anonymous namespace.

Done, marked as static.

mrry · 2018-10-12T17:14:34Z

tensorflow/contrib/igfs/kernels/igfs_client.h

+             const string &user_name);
+  ~IGFSClient();
+
+  inline Status Handshake(CtrlResponse<HandshakeResponse> *res) {


inline is implied for member functions defined in the class definition.

Yes. It's exactly member function defined in the class definition.

Right, my point is that the inline here is redundant, so it doesn't need to be there (even though the compiler accepts and ignores it).

The other headers don't use inline when the function is defined inline in the class definition, so please delete the inline specifiers in this file for consistency.

Okay, I got it. I've removed inline in headers here and in ignite_client.h and ignite_byte_swapper.h as well.

mrry · 2018-10-12T17:15:49Z

tensorflow/contrib/BUILD

@@ -126,6 +126,7 @@ py_library(
    }) + select({
        "//tensorflow:with_ignite_support": [
            "//tensorflow/contrib/ignite",
+            "//tensorflow/contrib/igfs",


Can we combine these two modules into one? IGFS seems like it would fit under "//tensorflow/contrib/ignite".

I would prefer to have them in separate modules by several reasons. First of all, they have very different functionality ("//tensorflow/contrib/ignite" provides an ability to read data from Apache Ignite Cache while "//tensorflow/contrib/igfs" provides allows to use file system, based on Apache Ignite). User may want to use only one feature, but not other. It looks like very reasonable in some cases. I think it's a case when "write programs that do one thing and do it well" is good, isn't it? And by the way, after contrib sunsetting it makes even more sense to have them in separate packages.

Are you agree?

No, I don't agree. The two modules are coupled by dependencies from this new code to the tensorflow/contrib/ignite module. The tf.contrib.igfs module doesn't export any public symbols, so users won't interact with it directly... and (if built) it will be loaded if they like it or not.

Perhaps most importantly, the shared object rules in the BUILD file mean that the code in "//tensorflow/contrib/ignite:ignite_client" will be present in both tensorflow/contrib/igfs/_igfs_ops.so and tensorflow/contrib/ignite/_dataset_ops.so, which can lead to a One Definition Rule violation and undefined behavior.

So, for all those reasons, life would be much easier if the code were combined in one module, with a single shared object!

Sounds reasonable. I merged two modules together.

mrry · 2018-10-12T17:18:15Z

tensorflow/contrib/igfs/kernels/igfs.cc

+  return Status::OK();
+}
+
+std::shared_ptr<IGFSClient> IGFS::CreateClient() const {


Is the "shared"-ness ever used here? It looks like this could return a unique_ptr.

Replaced by unique_ptr, done.

mrry · 2018-10-12T17:20:25Z

tensorflow/contrib/igfs/kernels/igfs_extended_tcp_client.cc

+    TF_RETURN_IF_ERROR(ReadString(&key));
+    TF_RETURN_IF_ERROR(ReadString(&val));
+
+    res->insert(std::pair<string, string>(key, val));


You could std::move(key), std::move(val) here to avoid copying the strings again.

mrry · 2018-10-12T17:22:06Z

tensorflow/contrib/igfs/kernels/igfs_extended_tcp_client.cc

+Status ExtendedTCPClient::WriteString(string str) {
+  if (!str.empty()) {
+    TF_RETURN_IF_ERROR(WriteBool(false));
+    unsigned short l = str.length();


This is an unsafe cast and should raise a compiler warning: str.length() returns a size_t. You need to check that it fits in the range of an unsigned short before casting.

mrry · 2018-10-12T17:24:33Z

tensorflow/contrib/igfs/kernels/igfs_messages.cc

+                                 const std::map<string, string> &properties)
+    : Request(command_id_),
+      user_name_(std::move(user_name)),
+      path_(std::move(path)),


path is a const string&, so moving it has no effect. If you want to avoid a copy, you need to take path as a (by-value) string or a string&&.

Okay, just removed copy().

mrry · 2018-10-12T17:26:25Z

tensorflow/contrib/igfs/kernels/igfs_messages.h

+    int32_t len;
+    TF_RETURN_IF_ERROR(client->ReadInt(&len));
+
+    entries = std::vector<T>();


Suggestion: entries.clear() is slightly more readable, since the object must already have been constructed.

dmitrievanthony · 2018-10-19T09:56:52Z

Hi @mrry. Any updates?

dmitrievanthony · 2018-10-26T13:34:50Z

Hi, @mrry, @yifeif. I don't clearly understand current state. Do you have some concerns about this PR or it's ready to be merged?

mrry · 2018-11-02T01:11:57Z

tensorflow/contrib/igfs/kernels/igfs_client.h

+             const string &user_name);
+  ~IGFSClient();
+
+  inline Status Handshake(CtrlResponse<HandshakeResponse> *res) {


Right, my point is that the inline here is redundant, so it doesn't need to be there (even though the compiler accepts and ignores it).

The other headers don't use inline when the function is defined inline in the class definition, so please delete the inline specifiers in this file for consistency.

mrry · 2018-11-02T01:15:51Z

tensorflow/contrib/igfs/kernels/igfs_extended_tcp_client.cc

+  if (!str.empty()) {
+    TF_RETURN_IF_ERROR(WriteBool(false));
+    size_t l = str.length();
+    if (l > 0xFFFF) return errors::InvalidArgument("String is too long");


If I remember correctly, WriteShort() takes a (signed) int16_t, so the maximum value is 0x7fff. Maybe use std::numeric_limits<int16_t>::max to avoid errors here?

Good point, done.

mrry · 2018-11-02T01:22:16Z

tensorflow/contrib/BUILD

@@ -126,6 +126,7 @@ py_library(
    }) + select({
        "//tensorflow:with_ignite_support": [
            "//tensorflow/contrib/ignite",
+            "//tensorflow/contrib/igfs",


No, I don't agree. The two modules are coupled by dependencies from this new code to the tensorflow/contrib/ignite module. The tf.contrib.igfs module doesn't export any public symbols, so users won't interact with it directly... and (if built) it will be loaded if they like it or not.

Perhaps most importantly, the shared object rules in the BUILD file mean that the code in "//tensorflow/contrib/ignite:ignite_client" will be present in both tensorflow/contrib/igfs/_igfs_ops.so and tensorflow/contrib/ignite/_dataset_ops.so, which can lead to a One Definition Rule violation and undefined behavior.

So, for all those reasons, life would be much easier if the code were combined in one module, with a single shared object!

dmitrievanthony · 2018-11-02T14:35:54Z

Hi @mrry. I merged ignite and igfs modules together and fixed other remarks. Please have a look.

mrry · 2018-11-02T19:51:35Z

tensorflow/contrib/ignite/BUILD

+        "kernels/igfs/igfs_writable_file.h",
+    ],
+    deps = [
+        "//tensorflow/contrib/ignite:ignite_client",


Nit: This should be ":ignite_client" now.

mrry · 2018-11-02T19:51:51Z

tensorflow/contrib/ignite/BUILD

@@ -31,39 +32,88 @@ tf_custom_op_library(
    deps = [":dataset_kernels"],
 )

+tf_custom_op_library(


Since ":igfs_kernels" and ":dataset_kernels" share some symbols (in ":ignite_client"), to avoid the ODR issues, it'll be easiest to merge "_dataset_ops.so" and "_igfs_ops.so" into a single shared object (and hence tf_custom_op_library() rule).

Okay, good point. I've merged them into a single shared library _ignite_ops.so.

dmitrievanthony · 2018-11-06T08:38:57Z

Hi @mrry. I've updated the code, please have a look.

dmitrievanthony · 2018-11-07T11:38:22Z

It looks like Sanity failed because of //tensorflow/core:lib dependency in igfs_kernels. I've removed it, so please rerun tests.

dmitrievanthony · 2018-11-07T21:05:16Z

Thanks @mrry, looks like everything is fixed. Currently only one test fails and it fails in non related to my code part.

Hope the PR will be merged soon :)

googlebot · 2018-11-07T21:15:18Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

PiperOrigin-RevId: 220969388

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

googlebot added the cla: no label Sep 10, 2018

dmitrievanthony mentioned this pull request Sep 10, 2018

RFC: Sunset tf.contrib tensorflow/community#18

Merged

yifeif self-assigned this Sep 10, 2018

yifeif added cla: yes and removed cla: no labels Sep 11, 2018

yifeif added the awaiting review Pull request awaiting review label Sep 11, 2018

googlebot added cla: no and removed cla: yes labels Sep 17, 2018

martinwicke added cla: yes and removed cla: no labels Sep 27, 2018

googlebot added cla: no and removed cla: yes labels Oct 2, 2018

martinwicke added cla: yes and removed cla: no labels Oct 2, 2018

mrry suggested changes Oct 12, 2018

View reviewed changes

tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 13, 2018

mrry suggested changes Nov 2, 2018

View reviewed changes

dmitrievanthony and others added 6 commits November 2, 2018 17:06

Add IGFS (Apache Ignite File System) support.

1da12a4

Add IGFS (Apache Ignite File System) support.

a834086

Update after review of #22210.

1602570

Update Apache Ignite IGFS after review.

fdeac73

Update Apache Ignite IGFS after review.

3e0c8ab

Update Apache Ignite IGFS after review.

29bb7f4

mrry suggested changes Nov 2, 2018

View reviewed changes

Update Apache Ignite IGFS after review.

69b7ab2

mrry previously approved these changes Nov 6, 2018

View reviewed changes

mrry added awaiting testing (then merge) kokoro:force-run Tests on submitted change labels Nov 6, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 6, 2018

Remove unused dependencies.

24579bc

dmitrievanthony dismissed mrry’s stale review via 24579bc November 7, 2018 11:35

mrry added the kokoro:force-run Tests on submitted change label Nov 7, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 7, 2018

yifeif added cla: yes ready to pull PR ready for merge process and removed cla: no labels Nov 7, 2018

tensorflow-copybara merged commit 24579bc into tensorflow:master Nov 11, 2018

tensorflow-copybara pushed a commit that referenced this pull request Nov 11, 2018

Merge pull request #22194 from dmitrievanthony:apache-ignite-igfs

9d67841

PiperOrigin-RevId: 220969388

dmitrievanthony mentioned this pull request Jan 22, 2019

Added release notes for 1.13 release #25084

Merged


		namespace tensorflow {

		string GetEnvOrElse(const string &env, string default_value) {

Apache Ignite File System #22194

Apache Ignite File System #22194

Conversation

dmitrievanthony commented Sep 10, 2018 • edited

googlebot commented Sep 10, 2018

yifeif commented Sep 10, 2018

dmitrievanthony commented Sep 10, 2018 • edited

artemmalykh commented Sep 10, 2018

yifeif commented Sep 11, 2018

googlebot commented Sep 11, 2018

googlebot commented Sep 17, 2018

dmitrievanthony commented Sep 17, 2018

googlebot commented Sep 27, 2018

googlebot commented Oct 2, 2018

dmitrievanthony commented Oct 2, 2018

googlebot commented Oct 2, 2018

dmitrievanthony commented Oct 8, 2018

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 Oct 15, 2018 • edited

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 Oct 19, 2018

dmitrievanthony commented Oct 26, 2018

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 Nov 2, 2018

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 Nov 6, 2018

dmitrievanthony commented Nov 7, 2018

dmitrievanthony commented Nov 7, 2018

googlebot commented Nov 7, 2018

dmitrievanthony commented Sep 10, 2018 •

edited

dmitrievanthony commented Sep 10, 2018 •

edited

dmitrievanthony Oct 15, 2018 •

edited