[WIP] Support to auto-generate Java files from flatbuffer #3716

jovany-wang · 2019-01-08T08:48:28Z

What do these changes do?

generate java files from flatbuffer automatically.
add auto_gen_tool.py script to change files automatically.

After this PR, We should not change them manually.

guoyuhong

Thanks. I left some comments.

guoyuhong · 2019-01-08T09:08:56Z

src/ray/gcs/CMakeLists.txt

+  VERBATIM)
+
+# Generate Java bindings for the flatbuffers objects.
+set(RAY_HOME ${CMAKE_CURRENT_LIST_DIR}/../../..)


If you add RAY_HOME, you can also apply it to set(PYTHON_OUTPUT_DIR ${CMAKE_CURRENT_LIST_DIR}/../../../python/ray/core/generated/).

guoyuhong · 2019-01-08T09:09:52Z

auto_gen_tool.py

@@ -0,0 +1,89 @@
+#!/usr/bin/env python


This comment is not necessary?

guoyuhong · 2019-01-08T09:16:06Z

auto_gen_tool.py

+        return -1
+
+
+template = '''


template looks too general here. Shall we use template_for_byte_buffer_getter?

raulchen · 2019-01-08T09:19:17Z

auto_gen_tool.py

+    file_names = os.listdir(generated_root_path)
+    for file_name in file_names:
+        if not file_name.endswith('.java'):
+            break


this should be continue?

raulchen · 2019-01-08T09:31:40Z

auto_gen_tool.py

+def add_new_line(file, line_num, text):
+    with open(file, "r") as file_handler:
+        lines = file_handler.readlines()
+        if (line_num < 0) or (line_num > len(lines) + 1):


raulchen · 2019-01-08T09:34:41Z

auto_gen_tool.py

+
+        for index in range(len(lines) - 1, -1, -1):
+            if lines[index] == '}\n':
+                index_to_be_inserted = index


why can't index_to_be_inserted just be len(lines) - 1?

Because we can not make sure that there is no blank line in the end of file. Maybe it have 2 blank lines.

raulchen · 2019-01-08T09:35:47Z

auto_gen_tool.py

+                index_to_be_inserted = index
+
+    if index_to_be_inserted >= 0:
+        offset = get_offset(file, field)


we should raise an exception if get_offset fails to find the line number

raulchen · 2019-01-08T09:37:13Z

auto_gen_tool.py

+    generate_and_insert_method('%s/TaskInfo.java' % root_path,
+                               'returns', 'returnsAsByteBuffer')
+    generate_and_insert_method('%s/Arg.java' % root_path,
+                               'objectIds', 'objectIdAsByteBuffer')


Let's define these fields as a list. E.g.,

files_and_fields = [ ('TaskInfo', 'returns'), ('Arg', 'objectIds'), ]

raulchen · 2019-01-08T09:38:56Z

src/ray/gcs/CMakeLists.txt

+add_custom_command(
+  TARGET gen_gcs_fbs
+  COMMAND ${FLATBUFFERS_COMPILER} -j -o ${JAVA_OUTPUT_DIR} ${GCS_FBS_SRC}
+  COMMAND python ${RAY_HOME}/auto_gen_tool.py ${RAY_HOME}


Let's put the script under java/ dir, and name it modify_generated_java_flatbuffers_files.py

AmplabJenkins · 2019-01-08T11:12:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10671/
Test PASSed.

jovany-wang · 2019-01-08T12:45:41Z

Thanks for your comments, @guoyuhong @raulchen
I have addressed them. :)

suquark · 2019-01-08T13:58:28Z

I have added this PR to the project board since it removes magic flatbuffer files. We could probably mention it in the java docs for later developers. In the future we could ask the flatbuffer team to solve this problem.

suquark · 2019-01-08T14:03:48Z

And I am agree with @raulchen that we should rename auto_gen_tool.py to modify_generated_java_flatbuffers_files and put it under the java dir.

jovany-wang · 2019-01-08T14:08:11Z

And I am agree with @raulchen that we should rename auto_gen_tool.py to modify_generated_java_flatbuffers_files and put it under the java dir.

Thanks for your mentioning again. I have addressed it. :)

AmplabJenkins · 2019-01-08T14:41:13Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10674/
Test PASSed.

AmplabJenkins · 2019-01-08T16:15:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10675/
Test PASSed.

AmplabJenkins · 2019-01-08T17:09:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10678/
Test PASSed.

robertnishihara · 2019-01-08T21:08:35Z

@jovany-wang @raulchen given the discussion in google/flatbuffers#5092, isn't the simplest solution to define a separate table in order to implement a 2D array of bytes? E.g.,

table Bar {
   data: [ubyte];
}
table MyTable {
   bar: [Bar];
}

as @raulchen suggested. That would be fairly unintrusive, right? Then we don't need to modify the autogenerated files at all. That seems like a more sustainable approach.

Also, which flatbuffer schema is the one that actually requires the 2D array of bytes?

jovany-wang · 2019-01-09T03:25:35Z

Also, which flatbuffer schema is the one that actually requires the 2D array of bytes?

Re:
L79 and L45 in gcs.fbs require 2D array.
If we don't want to modify the auto-generated files, we have 2 approach:

as @raulchen suggested, we could define another table:

table ReturnObject {
data: [ubyte];
}

table TaskInfo {
returns: [ReturnObject];
}

But it's too odd.
2) we could store the hex string of objects in flatbuffer.

Btw, there is also another issue if we don't modify the auto-generated files:
The package declaration in Java is different from python and C++, and there is no approach to specify package(namespace) for Java like this:

flatc -j -o dir gcs.fbs --namepsace org.a.b.c

robertnishihara

Ok, this is much better than than the current state of affairs, so I think we should do this, but longer term we really shouldn't modify the autogenerated files even if it makes the schema a little bit uglier.

E.g., another option is to have a single string which is just all of the object IDs concatenated together.

A heavier weight option is to switch to protobuf.

java/modify_generated_java_flatbuffers_files.py

@@ -0,0 +1,99 @@
+import os


robertnishihara · 2019-01-09T04:16:50Z

java/modify_generated_java_flatbuffers_files.py

+def add_package_declarations(generated_root_path):
+    file_names = os.listdir(generated_root_path)
+    for file_name in file_names:
+        if not file_name.endswith('.java'):


use " everywhere instead of '

robertnishihara · 2019-01-09T04:17:06Z

java/modify_generated_java_flatbuffers_files.py

+        return -1
+
+
+template_for_byte_buffer_getter = '''


''' -> """

robertnishihara · 2019-01-09T04:17:27Z

java/modify_generated_java_flatbuffers_files.py

+
+
+def modify_generated_java_flatbuffers_files(ray_home, tuples):
+    root_path = '%s/java/runtime/src/main/java/org/ray/runtime/generated' % ray_home


use os.path.join instead

robertnishihara · 2019-01-09T04:17:34Z

java/modify_generated_java_flatbuffers_files.py

+    add_package_declarations(root_path)
+
+    for tuple in tuples:
+        file_name = '%s/%s.java' % (root_path, tuple[0])


os.path.join

robertnishihara · 2019-01-09T04:18:25Z

java/modify_generated_java_flatbuffers_files.py

+        ('Arg', 'objectIds', 'objectIdsAsByteBuffer'),
+    ]
+
+    modify_generated_java_flatbuffers_files(sys.argv[1], tuples)


Document usage at the top of the file, what is sys.argv[1]? Explain the purpose of this script?

robertnishihara · 2019-01-09T04:19:55Z

src/ray/gcs/CMakeLists.txt

+add_custom_command(
+  TARGET gen_gcs_fbs
+  COMMAND ${FLATBUFFERS_COMPILER} -j -o ${JAVA_OUTPUT_DIR} ${GCS_FBS_SRC}
+  COMMAND python ${RAY_HOME}/java/modify_generated_java_flatbuffers_files.py ${RAY_HOME}


The command python may not exist on the user's machine, e.g., they may only have python3 or some command like that.

robertnishihara · 2019-01-09T04:21:18Z

src/ray/gcs/CMakeLists.txt

+  COMMENT "Running flatc compiler on ${GCS_FBS_SRC} for python"
+  VERBATIM)
+
+# Generate Java bindings for the flatbuffers objects.


This deserves a more in depth explanation of what is going on here. Please explain that it is modifying the autogenerated flatbuffer files and say why and link to google/flatbuffers#5092.

AmplabJenkins · 2019-01-09T05:10:55Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10693/
Test FAILed.

AmplabJenkins · 2019-01-09T08:48:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10701/
Test PASSed.

AmplabJenkins · 2019-01-09T08:57:19Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10698/
Test PASSed.

raulchen · 2019-01-09T08:54:49Z

java/modify_generated_java_flatbuffers_files.py

+    tuples = [
+        ("TaskInfo", "returns", "returnsAsByteBuffer"),
+        ("Arg", "objectIds", "objectIdsAsByteBuffer"),
+    ]


the third element isn't needed.

maybe rename tuples to class_and_field_pairs

raulchen · 2019-01-09T08:56:15Z

java/modify_generated_java_flatbuffers_files.py

+        if not file_name.endswith(".java"):
+            continue
+        full_name = generated_root_path + "/" + file_name
+        success = add_new_line(full_name, 2, "package org.ray.runtime.generated;")


define the package declaration string as a constant.

raulchen · 2019-01-09T08:56:42Z

java/modify_generated_java_flatbuffers_files.py

+        file_name = os.path.join(root_path, "%s.java" % tuple[0])
+        generate_and_insert_method(file_name,
+                                   tuple[1],
+                                   tuple[2])


This doesn't seem to be well formatted

raulchen · 2019-01-09T08:59:11Z

src/ray/gcs/CMakeLists.txt

+#    See https://github.com/google/flatbuffers/issues/5092 for more details.
+#
+# 2) The package declaration in Java is different from python and C++, and there is
+#    no approach to specify package(namepsace) for Java specially.


maybe it's better to put this comment in the modify_generated_java_flatbuffers_files.py file, because it explains the purpose of that script.

raulchen · 2019-01-09T09:02:39Z

src/ray/gcs/CMakeLists.txt

+#
+# 1) We have 2D array fields in Table TaskInfo and Arg in `gcs.fbs`, and we
+#    have no better approach to get the element from 2D array field in flatbuffer.
+#    See https://github.com/google/flatbuffers/issues/5092 for more details.


In `gcs.fbs`, some fields are defined as `[string]`. And currently flatbuffers doesn't have an API to get a single element in the list as raw bytes. See https://github.com/google/flatbuffers/issues/5092 for more details.

raulchen · 2019-01-09T09:04:50Z

src/ray/gcs/CMakeLists.txt

+#    See https://github.com/google/flatbuffers/issues/5092 for more details.
+#
+# 2) The package declaration in Java is different from python and C++, and there is
+#    no approach to specify package(namepsace) for Java specially.


no approach -> no option in the flatc command

raulchen · 2019-01-09T09:05:44Z

java/modify_generated_java_flatbuffers_files.py

+        return -1
+
+
+template_for_byte_buffer_getter = """


upper-case the var name

raulchen · 2019-01-09T09:06:26Z

java/modify_generated_java_flatbuffers_files.py

+"""
+
+
+def generate_and_insert_method(file, field, method_name):


genrate_byte_buffer_getter

AmplabJenkins · 2019-01-09T12:12:34Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10708/
Test PASSed.

robertnishihara · 2019-01-09T20:03:49Z

@jovany-wang @raulchen what do you think about replacing new_actor_handles: [string]; with new_actor_handles: string;, where the string is simply the concatenation of all of the relevant strings? Then we wouldn't need to modify the autogenerated files.

This doesn't address point 2, but it addresses point 1 and we can probably find a way to address point 2.

raulchen · 2019-01-10T04:40:42Z

@jovany-wang @raulchen what do you think about replacing new_actor_handles: [string]; with new_actor_handles: string;, where the string is simply the concatenation of all of the relevant strings? Then we wouldn't need to modify the autogenerated files.

This doesn't address point 2, but it addresses point 1 and we can probably find a way to address point 2.

I think that' fine as well. (probably it's better to use [ubytes], if that doesn't require too many code changes.)

For point 2, I think we still need a patch for now. But this patch is more stable, because it only inserts a line at the top, and doesn't depend on the content of the generated files.

jovany-wang · 2019-01-10T06:16:15Z

@robertnishihara @raulchen
Thanks for your suggestions, I will work for this issue.

pcmoritz · 2019-01-10T07:06:02Z

I'm in favor of either defining

table ObjectID {
id: [ubyte];
}

(in the C++ codebase I think it could be done with relatively little changes by modifying from_flatbuf etc) or concatenating (concatenating would be ok since the problem only arises in few locations).

Having to postprocess flatbuffer files is awkward and the problem will probably come up again once we support more languages.

AmplabJenkins · 2019-01-10T19:24:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10749/
Test PASSed.

jovany-wang · 2019-01-11T06:07:41Z

Since this PR is outdated too much, I will request another one later and this could be closed now.

Jovan added 4 commits January 8, 2019 11:09

auto gen flatbuffers for Java

9f7783e

Add auto_gen_tool.py

6bc369c

Refine

ccf6a72

Add a comment

2af2096

jovany-wang requested review from guoyuhong, raulchen and robertnishihara January 8, 2019 08:48

guoyuhong reviewed Jan 8, 2019

View reviewed changes

raulchen requested changes Jan 8, 2019

View reviewed changes

address comments.

eb3e8e6

suquark assigned jovany-wang Jan 8, 2019

Address comments.

03f269b

Addressed

e819555

Refine

6554686

robertnishihara reviewed Jan 9, 2019

View reviewed changes

Jovan added 3 commits January 9, 2019 14:22

Address comments

a6dc98d

Fix typo

7e5bcfc

Add exception

8b96f26

raulchen reviewed Jan 9, 2019

View reviewed changes

Address comments.

91e05f8

jovany-wang changed the title ~~Support to auto-generate Java files from flatbuffer~~ [WIP] Support to auto-generate Java files from flatbuffer Jan 10, 2019

Jovan added 2 commits January 10, 2019 15:47

Merge branch 'rs-master' into auto-gen-java-fbs

9068207

WIP

ccce531

jovany-wang closed this Jan 11, 2019

jovany-wang mentioned this pull request Jan 11, 2019

Support to auto-generate Java files from flatbuffer #3749

Merged

jovany-wang deleted the auto-gen-java-fbs branch January 17, 2019 12:51



		def modify_generated_java_flatbuffers_files(ray_home, tuples):
		root_path = '%s/java/runtime/src/main/java/org/ray/runtime/generated' % ray_home

		"""


		def generate_and_insert_method(file, field, method_name):

[WIP] Support to auto-generate Java files from flatbuffer #3716

[WIP] Support to auto-generate Java files from flatbuffer #3716

Uh oh!

Conversation

jovany-wang commented Jan 8, 2019

What do these changes do?

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

jovany-wang Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AmplabJenkins commented Jan 8, 2019

Uh oh!

jovany-wang commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suquark commented Jan 8, 2019

Uh oh!

suquark commented Jan 8, 2019

Uh oh!

jovany-wang commented Jan 8, 2019

Uh oh!

AmplabJenkins commented Jan 8, 2019

Uh oh!

AmplabJenkins commented Jan 8, 2019

Uh oh!

AmplabJenkins commented Jan 8, 2019

Uh oh!

robertnishihara commented Jan 8, 2019

Uh oh!

jovany-wang commented Jan 9, 2019

Uh oh!

robertnishihara left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jan 9, 2019

Uh oh!

AmplabJenkins commented Jan 9, 2019

Uh oh!

AmplabJenkins commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jovany-wang Jan 8, 2019 •

edited

Loading

jovany-wang commented Jan 8, 2019 •

edited

Loading