[core] [no-op] Cleanup utils folder bazel dependency #49938

dentiny · 2025-01-18T07:40:30Z

Motivation: @jjyao was helping debugging a boost dependency issue, and found for some of the util targets are built twice since they were included in two targets.

The best practice for using bazel is to define bazel targets for each translation unit (for C++), and only wrap a few translation units when (1) they're usually used together; (2) they have interdependency.

This PR split the giant util target into smaller targets, so next time we could , and should include a clear single target.
it includes some dependency cleanup and apply IWYU, overall a no-op change.

Signed-off-by: dentiny <[email protected]>

dayshah

nice lgtm, did you get get boost iostreams to work?

src/ray/util/filesystem.h

dentiny · 2025-01-18T20:14:58Z

nice lgtm, did you get get boost iostreams to work?

Yeah Jiajun helped me work out, the issue is we have two targets compiling pipe logger; one of them has the necessary dependency but the other one doesn't :(

Signed-off-by: dentiny <[email protected]>

jjyao · 2025-01-19T05:26:41Z

src/ray/core_worker/task_manager.cc

@@ -1015,7 +1015,7 @@ bool TaskManager::RetryTaskIfPossible(const TaskID &task_id,
    // TODO(clarng): clean up and remove task_retry_delay_ms that is relied
    // on by some tests.
    int32_t delay_ms = task_failed_due_to_oom
-                           ? ExponentialBackoff::GetBackoffMs(
+                           ? ExponentialBackOff::GetBackoffMs(


what's changed in this line?

upper case and lower case.

jjyao · 2025-01-19T05:30:24Z

src/ray/util/BUILD

@@ -1,44 +1,213 @@
 load("//bazel:ray.bzl", "ray_cc_library")



@aslonnie is this file now follows bazel best practice? Need your expertise here :)

Not yet, the followup PR is:

Split the giant utils bazel target and depend what we use;

Split logging / filesystem dependency (should be an easy followup).

overall seems fine. probably will be non-trivial to maintain this manually though.

it is the right thing to do to have fine-grained bazel build rules to have the best build performance. in the long run, we will need tools to auto maintain this build file based on static analysis of source files.

in the long run, we will need tools to auto maintain this build file based on static analysis of source files.

I agree, in Nuro we have BUILD file auto-generation so I never need to hand-write bazel file;
I checked some open source tools like gazelle, but none of them seem to support C++; if you could help me with that I would be much grateful!

jjyao · 2025-01-19T05:30:59Z

src/ray/util/exponential_backoff.h

+  uint64_t Next() {
+    auto ret = curr_value_;
+    curr_value_ = curr_value_ * multiplier_;
+    curr_value_ = std::min(curr_value_, max_value_);
+    return ret;
+  }
+
+  uint64_t Current() { return curr_value_; }
+
+  void Reset() { curr_value_ = initial_value_; }


Can we split unifying ExponentialBackoff to a separate PR?

aslonnie · 2025-01-19T10:11:05Z

src/ray/util/BUILD

 ray_cc_library(
-    name = "util",
-    srcs = glob(
-        [
-            "*.cc",
-        ],
-        exclude = [
-            "*_test.cc",
-        ],
-    ),
-    hdrs = glob([
-        "*.h",
-    ]),
-    linkopts = select({
-        "@platforms//os:windows": [],
-        "//conditions:default": ["-lpthread"],
-    }),
+    name = "visibility",
+    hdrs = ["visibility.h"],
+)


why are these single header libraries? are they actually just header-only libraries?

this will not even generate object files. I think there are also bazel rules that are cc_headers for headers.

I understand, but it's common to have header-only targets.
For example, abseil one: https://github.com/abseil/abseil-cpp/blob/3735766b3bd68522c8f291675df3fddb4bc3c70d/absl/base/BUILD.bazel

aslonnie · 2025-01-19T10:13:02Z

src/ray/util/BUILD

    deps = [
        ":thread_checker",
-        "//:aligned_alloc",
-        "//:sha256",
+    ],


where is this :thread_checker defined? I do not seem to find it in this file.

ray/src/ray/util/BUILD

Lines 216 to 221 in 8ba3ffa

ray_cc_library(

name = "thread_checker",

hdrs = ["thread_checker.h"],

srcs = ["thread_checker.cc"],

)

Well for missing dependency issue, we should trust the compiler and CI

aslonnie · 2025-01-19T10:14:29Z

src/ray/util/BUILD

+    deps = [
+        ":array",
+        ":container_util",
+        ":counter_map",
+        ":event",
+        ":event_label",
+        ":exponential_backoff",
+        ":function_traits",
+        ":logging",
+        ":macros",
+        ":memory",
+        ":process",
+        ":random",
+        ":sample",
+        ":sequencer",
+        ":string_utils",
+        ":timestamp_utils",
+        ":throttler",
+        ":thread_utils",
+        ":type_traits",
+        ":visibility",
+        "//:sha256",


yeah.. this feels a bit wrong.. that a util library depends on so many other libs here.

It's written this way for target compatibility, otherwise I have to manually update a thousand files.

As I mentioned here, it's worth another PR to further cleanup the util target dependency.

aslonnie · 2025-01-19T10:16:51Z

src/ray/util/BUILD

@@ -1,44 +1,213 @@
 load("//bazel:ray.bzl", "ray_cc_library")



overall seems fine. probably will be non-trivial to maintain this manually though.

it is the right thing to do to have fine-grained bazel build rules to have the best build performance. in the long run, we will need tools to auto maintain this build file based on static analysis of source files.

@jjyao

We have two exponential backoff class, one with `Off` in uppercase, another one with `off` in lowercase, which is confusion; and I think it doesn't make sense to have two classes serving strongly relavent feature. This PR is originally part of the refactor PR, suggested by @jjyao to make a separate one. Refactor PR for reference: #49938 Jiajun's comment: #49938 (comment) --------- Signed-off-by: dentiny <[email protected]>

@jjyao

We have two exponential backoff class, one with `Off` in uppercase, another one with `off` in lowercase, which is confusion; and I think it doesn't make sense to have two classes serving strongly relavent feature. This PR is originally part of the refactor PR, suggested by @jjyao to make a separate one. Refactor PR for reference: ray-project#49938 Jiajun's comment: ray-project#49938 (comment) --------- Signed-off-by: dentiny <[email protected]> Signed-off-by: Jimmy Xie <[email protected]>

@jjyao

We have two exponential backoff class, one with `Off` in uppercase, another one with `off` in lowercase, which is confusion; and I think it doesn't make sense to have two classes serving strongly relavent feature. This PR is originally part of the refactor PR, suggested by @jjyao to make a separate one. Refactor PR for reference: ray-project#49938 Jiajun's comment: ray-project#49938 (comment) --------- Signed-off-by: dentiny <[email protected]> Signed-off-by: Jimmy Xie <[email protected]>

Signed-off-by: dentiny <[email protected]>

jjyao

Thanks!

src/ray/util/tests/util_test.cc

Signed-off-by: dentiny <[email protected]>

@jjyao

We have two exponential backoff class, one with `Off` in uppercase, another one with `off` in lowercase, which is confusion; and I think it doesn't make sense to have two classes serving strongly relavent feature. This PR is originally part of the refactor PR, suggested by @jjyao to make a separate one. Refactor PR for reference: ray-project#49938 Jiajun's comment: ray-project#49938 (comment) --------- Signed-off-by: dentiny <[email protected]> Signed-off-by: Anson Qian <[email protected]>

Signed-off-by: dentiny <[email protected]> Signed-off-by: Anson Qian <[email protected]>

cleanup utils folder bazel dependency

439d307

Signed-off-by: dentiny <[email protected]>

dentiny added the go add ONLY when ready to merge, run all tests label Jan 18, 2025

dentiny requested review from jjyao, rynewang and dayshah January 18, 2025 07:40

dentiny assigned jjyao, rynewang and dayshah Jan 18, 2025

dentiny requested a review from a team as a code owner January 18, 2025 07:40

dayshah approved these changes Jan 18, 2025

View reviewed changes

src/ray/util/filesystem.h Outdated Show resolved Hide resolved

dentiny added 2 commits January 18, 2025 20:43

fix build and revert static

e794acd

Signed-off-by: dentiny <[email protected]>

add pr reference

8ba3ffa

Signed-off-by: dentiny <[email protected]>

jjyao reviewed Jan 19, 2025

View reviewed changes

dentiny mentioned this pull request Jan 19, 2025

[core] [easy] [no-op] Unify exponential backoff #49957

Merged

dentiny requested a review from jjyao January 19, 2025 05:58

aslonnie reviewed Jan 19, 2025

View reviewed changes

dentiny requested a review from aslonnie January 19, 2025 10:26

aslonnie approved these changes Jan 19, 2025

View reviewed changes

dentiny added 2 commits January 20, 2025 03:27

Merge branch 'master' into hjiang/cleanup-utils-bazel-dependency

43808ba

backoff

8755a5f

Signed-off-by: dentiny <[email protected]>

jjyao approved these changes Jan 20, 2025

View reviewed changes

src/ray/util/tests/util_test.cc Outdated Show resolved Hide resolved

fix git conflict

10ab8b9

Signed-off-by: dentiny <[email protected]>

jjyao merged commit ce40c98 into ray-project:master Jan 21, 2025
5 checks passed

dentiny mentioned this pull request Jan 23, 2025

[core] Fix ray util bazel build files #49928

Closed

anson627 pushed a commit to anson627/ray that referenced this pull request Jan 31, 2025

[core] [no-op] Cleanup utils folder bazel dependency (ray-project#49938)

cc7d2b0

Signed-off-by: dentiny <[email protected]> Signed-off-by: Anson Qian <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] [no-op] Cleanup utils folder bazel dependency #49938

[core] [no-op] Cleanup utils folder bazel dependency #49938

dentiny commented Jan 18, 2025 •

edited

Loading

dayshah left a comment

dentiny commented Jan 18, 2025 •

edited

Loading

jjyao Jan 19, 2025

dentiny Jan 19, 2025

jjyao Jan 19, 2025

dentiny Jan 19, 2025

aslonnie Jan 19, 2025

dentiny Jan 19, 2025

jjyao Jan 19, 2025

dentiny Jan 19, 2025

aslonnie Jan 19, 2025

dentiny Jan 19, 2025

aslonnie Jan 19, 2025

dentiny Jan 19, 2025

dentiny Jan 19, 2025

aslonnie Jan 19, 2025

dentiny Jan 19, 2025

aslonnie Jan 19, 2025

jjyao left a comment

	ray_cc_library(
	name = "thread_checker",
	hdrs = ["thread_checker.h"],
	srcs = ["thread_checker.cc"],

	)

[core] [no-op] Cleanup utils folder bazel dependency #49938

[core] [no-op] Cleanup utils folder bazel dependency #49938

Conversation

dentiny commented Jan 18, 2025 • edited Loading

dayshah left a comment

Choose a reason for hiding this comment

dentiny commented Jan 18, 2025 • edited Loading

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

jjyao left a comment

Choose a reason for hiding this comment

dentiny commented Jan 18, 2025 •

edited

Loading

dentiny commented Jan 18, 2025 •

edited

Loading