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.

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] Add more debug string types #47928

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Oct 8, 2024

Followup on #47893, add more "blessed container types" to debug string function.

Share some of my thoughts (some are irrelevant to this PR):

  • The current implementation could be improved
    • There're a few types that we miss, if we want to general-purpose printer: std::byte, enum, types which supports absl stringify, fallback printer (which prints type and address, like python) if non existing printer matches;
    • A good use case for general-purpose printer would be:

      ray/src/ray/util/logging.h

      Lines 151 to 154 in 4ab6b7c

      #define RAY_CHECK_OP(left, op, right) \
      if (const auto &_left_ = (left); true) \
      if (const auto &_right_ = (right); true) \
      RAY_CHECK((_left_ op _right_)) << " " << _left_ << " vs " << _right_
  • We better split bazel targets
    • For example, all files under src/ray/util are contained in one single giant target, which greatly increase the compilation, link and test time

@dentiny dentiny force-pushed the hjiang/debug-string-more-cont branch 3 times, most recently from af5ffa4 to f7407bc Compare October 8, 2024 10:03
@dentiny
Copy link
Contributor Author

dentiny commented Oct 8, 2024

@jjyao / @rynewang Would appreciate a review :)

@dentiny dentiny force-pushed the hjiang/debug-string-more-cont branch from f7407bc to e22b951 Compare October 8, 2024 10:50
@dentiny dentiny force-pushed the hjiang/debug-string-more-cont branch from e22b951 to f9ca13e Compare October 8, 2024 11:07
src/ray/util/tests/container_util_test.cc Show resolved Hide resolved
@@ -93,10 +103,19 @@ std::ostream &operator<<(std::ostream &os, DebugStringWrapper<std::tuple<Ts...>>
return os;
}

template <typename T, std::size_t N>
std::ostream &operator<<(std::ostream &os, DebugStringWrapper<std::array<T, N>> c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have any usage of this specialization?

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 this PR, I'm trying to add well-known types to debug string.

src/ray/util/container_util.h Outdated Show resolved Hide resolved
src/ray/util/container_util.h Show resolved Hide resolved
@rynewang
Copy link
Contributor

rynewang commented Oct 8, 2024

I feel we can use our dev times more conservatively - only add a specialization when we need one. Similarly, we can split the build target but it should not help much since this header file is compiled with callers anyway (so all downstreams are rebuilt). Since this PR is already out we can merge it but for other ideas let's create a PR to track it (as P2) and I will find some other issues for you.

@dentiny
Copy link
Contributor Author

dentiny commented Oct 8, 2024

I feel we can use our dev times more conservatively - only add a specialization when we need one.

The best way is to templatize it, like the example I showed you last time;
If you don't like it, we could use gtest printer as well (https://github.com/google/googletest/blob/main/googletest/include/gtest/gtest-printers.h);
It's not perfect, still missing a few types, but generally usable, and allows customization via PrintTo and ADL.

Since this PR is already out we can merge it

Thank you!

Similarly, we can split the build target but it should not help much since this header file is compiled with callers anyway (so all downstreams are rebuilt).

Yes all downstream targets will be rebuilt, but that's oneshot effect.
Breaking targets into small pieces help compilation and link time.

I also notice we don't use static link for cc_test targets, which slows down tests.

Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from rynewang October 8, 2024 19:33
@dentiny dentiny requested a review from rynewang October 9, 2024 10:49
@dentiny
Copy link
Contributor Author

dentiny commented Oct 9, 2024

Similarly, we can split the build target but it should not help much since this header file is compiled with callers anyway

The reason why I care so much about bazel target granularity is every time I try to build GCS job manager and utils targets, it's taking me 20-30 minutes, depending on cache hit rate.

src/ray/util/container_util.h Outdated Show resolved Hide resolved
@dentiny dentiny requested a review from jjyao October 10, 2024 01:49
@rynewang rynewang enabled auto-merge (squash) October 10, 2024 06:37
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Oct 10, 2024
@rynewang rynewang merged commit 92e43a2 into ray-project:master Oct 10, 2024
7 checks passed
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
Followup on ray-project#47893, add more
"blessed container types" to debug string function.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants