Polish Bazel build scripts #6424

mehrdadn · 2019-12-11T01:37:26Z

Why are these changes needed?

Normalizes the conditionals used for platform-dependent build configurations (introduced in Streaming data transfer and python integration #6185).
Fixes string quotation

Related issue number

#6185

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.

AmplabJenkins · 2019-12-11T01:46:55Z

Can one of the admins verify this patch?

AmplabJenkins · 2019-12-11T07:00:08Z

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

raulchen

Thanks, I wasn't aware of these built-in conditions.

mehrdadn · 2019-12-11T09:00:23Z

No worries! There might be some more changes coming though -- I realized after I made this PR that I need to add some copts and such for Windows compatibility. I'll update the PR when I get it working.

raulchen · 2019-12-11T09:01:42Z

BUILD.bazel

            "//conditions:default": [
                "-Wl,--version-script,$(location //:src/ray/ray_version_script.lds)",
            ],
        }),
    },
-    copts = COPTS + if_linux_x86_64(["-fno-gnu-unique"]),
+    copts = COPTS + select({
+        "@bazel_tools//src/conditions:linux_x86_64": ["-fno-gnu-unique"],


I'm seeing the following error on macOS. I think you need to add a default condition here.

/Users/travis/build/ray-project/ray/BUILD.bazel:785:1: Configurable attribute "copts" doesn't match this configuration (would a default condition help?). Conditions checked: @bazel_tools//src/conditions:linux_x86_64

Yup I do have that fix locally! I haven't pushed it to avoid clogging the builds for now. Will hopefully push it when I fix some other things as well.

mehrdadn · 2019-12-11T11:15:46Z

@raulchen This is really unfortunate... I've I just started getting a ton of linker errors on Windows as a result of the previous PR, and tracking them down is proving to be somewhat of a nightmare. To give some examples, here's a sample of what I'm seeing:

lld-link: error: undefined symbol: public: __cdecl ray::RayLog::RayLog(char const *, int, enum ray::RayLogLevel)
>>> referenced by C:\users\home\_bazel_home\lkvlq2zo\execroot\com_github_ray_project_ray\src\ray\common\buffer.h:57
>>>               bazel-out/x64_windows-fastbuild/bin/_objs/python/ray/streaming/_streaming.so/_streaming.obj:(public: __cdecl ray::LocalMemoryBuffer::LocalMemoryBuffer(unsigned char *, unsigned __int64, bool))

lld-link: error: undefined symbol: public: virtual __cdecl ray::RayLog::~RayLog(void)
>>> referenced by C:\users\home\_bazel_home\lkvlq2zo\execroot\com_github_ray_project_ray\src\ray\common\buffer.h:57
>>>               bazel-out/x64_windows-fastbuild/bin/_objs/python/ray/streaming/_streaming.so/_streaming.obj:(public: __cdecl ray::LocalMemoryBuffer::LocalMemoryBuffer(unsigned char *, unsigned __int64, bool))

lld-link: error: undefined symbol: unsigned __int64 __cdecl ray::MurmurHash64A(void const *, int, unsigned int)
>>> referenced by C:\users\home\_bazel_home\lkvlq2zo\execroot\com_github_ray_project_ray\src\ray\common\id.h:443
>>>               streaming_lib.lib(data_reader.obj):(public: unsigned __int64 __cdecl ray::BaseID<class ray::ObjectID>::Hash(void) const)
>>> referenced by streaming_lib.lib(data_writer.obj)
>>> referenced by streaming_queue.lib(queue_handler.obj)
>>> referenced by streaming_lib.lib(channel.obj)

lld-link: error: undefined symbol: __imp_InterlockedExchangeAdd
>>> referenced by C:\users\home\_bazel_home\lkvlq2zo\execroot\com_github_ray_project_ray\external\boost\boost\thread\win32\basic_timed_mutex.hpp:244
>>>               thread.lib(thread.obj):(public: void __cdecl boost::detail::basic_timed_mutex::unlock(void))

It seems to me that a lot of these are due to the new use of shared libraries, which we had tried to avoid until this point—getting them to work seamlessly seems to be really painful. In fact, I don't yet even know how to handle them at all! But I'm unsure that the current approach is correct either—if for no other reason than the fact that Bazel should have a mechanism to auto-detect the library names so that you don't have to specify .so manually (given that on Windows you would need .dll instead).

Would it be possible for you to lend me a hand at fixing some issues? (Do you happen to have a Windows machine to test on, or any guesses as to how to fix them?)

Or even better—is there any chance you could switch to static libraries? That might make things much, much easier.

raulchen · 2019-12-11T11:44:40Z

@mehrdadn sorry about these problems.
@chaokunyang can help dig into this issue. He is more familiar with that.
I think if this issue turns out to be hard to fix, we can at least disable building streaming for Windows and make sure Ray core is okay.

mehrdadn · 2019-12-11T11:57:25Z

Oh that's a great idea if that's possible. Right now it's a dependency of ray_pkg so I assumed it's required, but if they can be separated that could also be helpful for now. (I'm not really aware of what targets are required for what purposes, so it's hard for me to judge these myself unfortunately.)

chaokunyang · 2019-12-11T12:09:39Z

@mehrdadn I'll disable building streaming for Windows for now. And looking into whether we can use exported dynamic libraries symbols in windows.

mehrdadn · 2019-12-11T12:11:17Z

@chaokunyang Okay sure, thanks! Feel free to just push a commit here if that's easier.

chaokunyang · 2019-12-11T12:46:57Z

@mehrdadn You can try this, I don't have a windows machine for this test, hope it works.

diff --git a/BUILD.bazel b/BUILD.bazel
index ef6db4d88..ca30a0fa9 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -951,11 +951,22 @@ py_library(
     visibility = ["__subpackages__"],
 )

+genrule(
+    name = "cp_streaming_lib",
+    srcs = ["python/ray/streaming/_streaming.so"],
+    outs = ["cp_streaming_lib.out"],
+    cmd = """
+        set -x &&
+        WORK_DIR=$$(pwd) &&
+        cp -f $(location python/ray/streaming/_streaming.so) $$WORK_DIR/python/ray/streaming &&
+        echo "$$WORK_DIR" > $@    
+    """,
+)
+
 genrule(
     name = "ray_pkg",
     srcs = [
         "python/ray/_raylet.so",
-        "python/ray/streaming/_streaming.so",
         "//:python_sources",
         "//:all_py_proto",
         "//:redis-server",
@@ -965,13 +976,15 @@ genrule(
         "//:raylet_monitor",
         "@plasma//:plasma_store_server",
         "//streaming:copy_streaming_py_proto",
-    ],
+    ] + select({ 
+        "@bazel_tools//src/conditions:windows": [],  # ignore build streaming for windows
+        "//conditions:default": [":cp_streaming_lib"],
+    }),
     outs = ["ray_pkg.out"],
     cmd = """
         set -x &&
         WORK_DIR=$$(pwd) &&
         cp -f $(location python/ray/_raylet.so) "$$WORK_DIR/python/ray" &&
-        cp -f $(location python/ray/streaming/_streaming.so) "$$WORK_DIR/python/ray/streaming" &&
         mkdir -p "$$WORK_DIR/python/ray/core/src/ray/thirdparty/redis/src/" &&
         cp -f $(location //:redis-server) "$$WORK_DIR/python/ray/core/src/ray/thirdparty/redis/src/" &&
         cp -f $(location //:redis-cli) "$$WORK_DIR/python/ray/core/src/ray/thirdparty/redis/src/" &&

mehrdadn · 2019-12-11T13:00:39Z

@chaokunyang I think it's working! I just pushed a new commit with this change, I think it should be good to merge on my end if it builds fine on other platforms. Thanks a ton! :)

chaokunyang · 2019-12-11T13:53:14Z

@mehrdadn Please add local = 1, for cp_streaming_lib to disable sandbox, otherwise it will fail in non-windows platform. Thanks

mehrdadn · 2019-12-11T14:27:10Z

@chaokunyang Done, thanks!

AmplabJenkins · 2019-12-12T00:28:39Z

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

AmplabJenkins · 2019-12-12T02:06: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/19455/
Test PASSed.

AmplabJenkins · 2019-12-12T02:49:01Z

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

AmplabJenkins · 2019-12-12T08:42:33Z

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

mehrdadn · 2019-12-13T10:28:24Z

Actually, @chaokunyang I have a question for you:

As far as I can tell (though I'm not entirely sure), Clang doesn't support -fno-gnu-unique, and hence it prevents building with Clang.
One obvious workaround is to try to special-case this for GCC, but I presume omitting it would break something... is that correct?
If so, do you know if there is a workaround for Clang? If there isn't, then should we block Clang builds?
Or if not, then is it something that can be removed on GCC as well?

chaokunyang · 2019-12-13T11:27:07Z

Hi @mehrdadn, -fno-gnu-unique can be removed now.

mehrdadn · 2019-12-13T11:32:20Z

Oh awesome, okay thanks @chaokunyang! I just removed it.

AmplabJenkins · 2019-12-14T03:40:36Z

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

Polish Bazel build scripts

3e0e616

raulchen approved these changes Dec 11, 2019

View reviewed changes

raulchen reviewed Dec 11, 2019

View reviewed changes

web-flow added 2 commits December 11, 2019 04:01

Remove glog references from streaming_logging.cc

a6e162e

Move out COPTS and reference them

01c8a04

mehrdadn force-pushed the bazel-fix branch from 81e213d to 01c8a04 Compare December 11, 2019 12:02

Disable streaming on Windows

49e0daf

mehrdadn force-pushed the bazel-fix branch from 83d30ca to 49e0daf Compare December 11, 2019 14:26

5e167f5

Remove -fno-gnu-unique

d6fcf15

mehrdadn requested a review from pcmoritz December 16, 2019 11:54

pcmoritz merged commit 7a24144 into ray-project:master Dec 17, 2019

mehrdadn deleted the bazel-fix branch December 17, 2019 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish Bazel build scripts #6424

Polish Bazel build scripts #6424

mehrdadn commented Dec 11, 2019 •

edited

AmplabJenkins commented Dec 11, 2019

AmplabJenkins commented Dec 11, 2019

raulchen left a comment

mehrdadn commented Dec 11, 2019

raulchen Dec 11, 2019

mehrdadn Dec 11, 2019

mehrdadn commented Dec 11, 2019

raulchen commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

mehrdadn commented Dec 13, 2019

chaokunyang commented Dec 13, 2019 •

edited

mehrdadn commented Dec 13, 2019

AmplabJenkins commented Dec 14, 2019

Polish Bazel build scripts #6424

Polish Bazel build scripts #6424

Conversation

mehrdadn commented Dec 11, 2019 • edited

Why are these changes needed?

Related issue number

Checks

AmplabJenkins commented Dec 11, 2019

AmplabJenkins commented Dec 11, 2019

raulchen left a comment

Choose a reason for hiding this comment

mehrdadn commented Dec 11, 2019

raulchen Dec 11, 2019

Choose a reason for hiding this comment

mehrdadn Dec 11, 2019

Choose a reason for hiding this comment

mehrdadn commented Dec 11, 2019

raulchen commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

chaokunyang commented Dec 11, 2019

mehrdadn commented Dec 11, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

AmplabJenkins commented Dec 12, 2019

mehrdadn commented Dec 13, 2019

chaokunyang commented Dec 13, 2019 • edited

mehrdadn commented Dec 13, 2019

AmplabJenkins commented Dec 14, 2019

mehrdadn commented Dec 11, 2019 •

edited

chaokunyang commented Dec 13, 2019 •

edited