Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jan 21, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
439d307
cleanup utils folder bazel dependency
dentiny Jan 18, 2025
e794acd
fix build and revert static
dentiny Jan 18, 2025
8ba3ffa
add pr reference
dentiny Jan 19, 2025
43808ba
Merge branch 'master' into hjiang/cleanup-utils-bazel-dependency
dentiny Jan 20, 2025
8755a5f
backoff
dentiny Jan 20, 2025
10ab8b9
fix git conflict
dentiny Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,7 @@ ray_cc_library(
],
deps = [
"//src/ray/util",
"//src/ray/util:size_literals",
"@com_github_jupp0r_prometheus_cpp//pull",
"@com_google_absl//absl/base:core_headers",
"@com_google_absl//absl/container:flat_hash_map",
Expand Down Expand Up @@ -786,6 +787,7 @@ ray_cc_library(
":worker_rpc",
"//src/ray/protobuf:worker_cc_proto",
"//src/ray/util",
"//src/ray/util:shared_lru",
"@boost//:circular_buffer",
"@boost//:fiber",
"@com_google_absl//absl/cleanup",
Expand Down
1 change: 1 addition & 0 deletions src/ray/common/asio/asio_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "ray/common/asio/instrumented_io_context.h"
#include "ray/util/array.h"
#include "ray/util/thread_utils.h"
#include "ray/util/util.h"

template <typename Duration>
Expand Down
1 change: 1 addition & 0 deletions src/ray/common/id.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "ray/common/constants.h"
#include "ray/util/logging.h"
#include "ray/util/random.h"
#include "ray/util/util.h"
#include "ray/util/visibility.h"

Expand Down
9 changes: 6 additions & 3 deletions src/ray/gcs/gcs_server/test/export_api/gcs_node_manager_export_event_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,21 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <gtest/gtest.h>

#include <chrono>
#include <memory>
#include <thread>

// clang-format off
#include "gtest/gtest.h"
#include "ray/gcs/gcs_server/test/gcs_server_test_util.h"
#include "ray/gcs/test/gcs_test_util.h"
#include "ray/util/event.h"
#include "ray/util/string_utils.h"

// clang-format off
#include "ray/rpc/node_manager/node_manager_client.h"
#include "ray/rpc/node_manager/node_manager_client_pool.h"
#include "mock/ray/pubsub/publisher.h"
#include "ray/util/event.h"
// clang-format on

using json = nlohmann::json;
Expand Down
1 change: 1 addition & 0 deletions src/ray/gcs/redis_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "ray/common/asio/instrumented_io_context.h"
#include "ray/common/status.h"
#include "ray/gcs/redis_async_context.h"
#include "ray/util/exponential_backoff.h"
#include "ray/util/logging.h"
#include "ray/util/util.h"
#include "src/ray/protobuf/gcs.pb.h"
Expand Down
3 changes: 2 additions & 1 deletion src/ray/object_manager/plasma/store_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@

#include "ray/common/ray_config.h"
#include "ray/object_manager/plasma/plasma_allocator.h"
#include "ray/util/thread_utils.h"

namespace plasma {
namespace internal {
void SetMallocGranularity(int value);
}
} // namespace internal

PlasmaStoreRunner::PlasmaStoreRunner(std::string socket_name,
int64_t system_memory,
Expand Down
1 change: 1 addition & 0 deletions src/ray/raylet/agent_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "ray/util/event_label.h"
#include "ray/util/logging.h"
#include "ray/util/process.h"
#include "ray/util/thread_utils.h"
#include "ray/util/util.h"

namespace ray {
Expand Down
1 change: 1 addition & 0 deletions src/ray/rpc/client_call.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "ray/common/grpc_util.h"
#include "ray/common/id.h"
#include "ray/common/status.h"
#include "ray/util/thread_utils.h"
#include "ray/util/util.h"

namespace ray {
Expand Down
1 change: 1 addition & 0 deletions src/ray/rpc/grpc_server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "ray/common/ray_config.h"
#include "ray/rpc/common.h"
#include "ray/stats/metric.h"
#include "ray/util/thread_utils.h"
#include "ray/util/util.h"

namespace ray {
Expand Down
223 changes: 197 additions & 26 deletions src/ray/util/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,213 @@
load("//bazel:ray.bzl", "ray_cc_library")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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"],
)
Comment on lines 3 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ray_cc_library(
name = "macros",
hdrs = ["macros.h"],
)

ray_cc_library(
name = "event_label",
hdrs = ["event_label.h"],
)

ray_cc_library(
name = "array",
hdrs = ["array.h"],
)

ray_cc_library(
name = "thread_utils",
hdrs = ["thread_utils.h"],
deps = [
":thread_checker",
"//:aligned_alloc",
"//:sha256",
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"],
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

ray_cc_library(
name = "exponential_backoff",
hdrs = ["exponential_backoff.h"],
srcs = ["exponential_backoff.cc"],
deps = [
":logging",
],
)

# TODO(hjiang): filesystem and logging has interdependency, we should split them into three targets: filesystem, logging, ray_check_macros.
ray_cc_library(
name = "logging",
hdrs = [
"filesystem.h",
"logging.h",
],
srcs = [
"filesystem.cc",
"logging.cc",
],
deps = [
":event_label",
":macros",
":thread_utils",
"@com_github_spdlog//:spdlog",
"@com_google_absl//absl/debugging:failure_signal_handler",
"@com_google_absl//absl/strings:str_format",
"@com_google_googletest//:gtest_main",
"@nlohmann_json",
],
)

ray_cc_library(
name = "container_util",
hdrs = ["container_util.h"],
deps = [
":logging",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/container:flat_hash_set",
],
)

ray_cc_library(
name = "process",
hdrs = [
"process.h",
"subreaper.h",
],
srcs = [
"process.cc",
"subreaper.cc",
],
deps = [
":logging",
":macros",
"@boost//:asio",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/synchronization",
],
)

ray_cc_library(
name = "function_traits",
hdrs = ["function_traits.h"],
deps = [
"@boost//:functional",
],
)

ray_cc_library(
name = "counter_map",
hdrs = ["counter_map.h"],
deps = [
":logging",
],
)

ray_cc_library(
name = "event",
hdrs = ["event.h"],
srcs = ["event.cc"],
deps = [
":logging",
":random",
":string_utils",
":timestamp_utils",
"//src/ray/protobuf:event_cc_proto",
"//src/ray/protobuf:export_event_cc_proto",
"@boost//:asio",
"@com_github_spdlog//:spdlog",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/debugging:failure_signal_handler",
"@com_google_absl//absl/debugging:stacktrace",
"@com_google_absl//absl/debugging:symbolize",
"@com_google_absl//absl/random",
"@com_google_absl//absl/synchronization",
"@com_google_absl//absl/time",
"@com_google_googletest//:gtest_main",
"@com_google_googletest//:gtest_prod",
"@com_google_protobuf//:protobuf",
"@nlohmann_json",
],
)

ray_cc_library(
name = "timestamp_utils",
hdrs = ["timestamp_utils.h"],
)

ray_cc_library(
name = "random",
hdrs = ["random.h"],
deps = [
"@com_google_absl//absl/random",
],
)

ray_cc_library(
name = "string_utils",
hdrs = ["string_utils.h"],
)

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

ray_cc_library(
name = "type_traits",
hdrs = ["type_traits.h"],
)

ray_cc_library(
name = "throttler",
hdrs = ["throttler.h"],
deps = [
"@com_google_absl//absl/time",
],
)

ray_cc_library(
name = "sequencer",
hdrs = ["sequencer.h"],
deps = [
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/synchronization",
],
)

ray_cc_library(
name = "sample",
hdrs = ["sample.h"],
)

# A giant 'util' target is split since PR https://github.com/ray-project/ray/pull/49938, here we keep the 'util' target for API compatibility.
#
# TODO(hjiang): We include a bunch of misc util function/class inside of the class, should split into multiple files and build targets.
ray_cc_library(
name = "util",
hdrs = ["util.h"],
srcs = ["util.cc"],
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",
Comment on lines +186 to +207
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
)

ray_cc_library(
name = "size_literals",
hdrs = ["size_literals.h"],
Expand Down Expand Up @@ -82,6 +251,8 @@ ray_cc_library(
srcs = ["pipe_logger.cc"],
deps = [
":compat",
":stream_redirection_options",
":thread_utils",
":util",
"@com_github_spdlog//:spdlog",
"@com_google_absl//absl/strings",
Expand Down
3 changes: 3 additions & 0 deletions src/ray/util/event.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

#include "absl/base/call_once.h"
#include "absl/time/time.h"
#include "ray/util/random.h"
#include "ray/util/string_utils.h"
#include "ray/util/timestamp_utils.h"

using json = nlohmann::json;

Expand Down
1 change: 0 additions & 1 deletion src/ray/util/event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "absl/container/flat_hash_map.h"
#include "nlohmann/json.hpp"
#include "ray/util/logging.h"
#include "ray/util/util.h"
#include "spdlog/sinks/basic_file_sink.h"
#include "spdlog/sinks/rotating_file_sink.h"
#include "spdlog/spdlog.h"
Expand Down
2 changes: 1 addition & 1 deletion src/ray/util/logging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
#include "nlohmann/json.hpp"
#include "ray/util/event_label.h"
#include "ray/util/filesystem.h"
#include "ray/util/util.h"
#include "ray/util/thread_utils.h"
#include "spdlog/sinks/basic_file_sink.h"
#include "spdlog/sinks/rotating_file_sink.h"
#include "spdlog/sinks/stdout_color_sinks.h"
Expand Down
2 changes: 1 addition & 1 deletion src/ray/util/memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

#pragma once

#include <stdint.h>
#include <cstdint>

namespace ray {

Expand Down
Loading
Loading