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] Place plasma bazel targets into separate directories #52016

Merged
merged 6 commits into from
Apr 11, 2025

Conversation

Ziy1-Tan
Copy link
Contributor

@Ziy1-Tan Ziy1-Tan commented Apr 5, 2025

Why are these changes needed?

Related issue number

Closes #52010

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@@ -219,3 +219,9 @@ filter_files_with_suffix = rule(
"suffix": attr.string(),
},
)

FLATC_ARGS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a comment

load("//bazel:ray.bzl", "COPTS", "FLATC_ARGS", "ray_cc_library", "ray_cc_test")

# TODO(mehrdadn): (How to) support dynamic linking?
PROPAGATED_WINDOWS_DEFINES = ["ARROW_STATIC"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: starlark mostly follows python grammar, so prefer to use _ prefix to indicate private variables.

out_prefix = "",
)

ray_cc_test(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI: I don't have a strong preference here, but generally BUILD file lives under the same directory as the source file (which is also the purpose for this issue :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I checked the /src/ray/common and it put the tests at seperate BUILD file.

@Ziy1-Tan Ziy1-Tan force-pushed the subbazel_plasma branch 2 times, most recently from 2d454c7 to ab3ce40 Compare April 6, 2025 09:11
@dentiny
Copy link
Contributor

dentiny commented Apr 6, 2025

@Ziy1-Tan Just FYI, as far as I'm aware of, currently main branch is broken and tests pretty flaky; so you don't need to take too much time debugging and merging main :)

@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Apr 7, 2025
@Ziy1-Tan Ziy1-Tan requested a review from dentiny April 7, 2025 09:01
@@ -18,8 +18,6 @@
#include <utility>
#include <vector>

#include "absl/random/random.h"
#include "absl/strings/str_format.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up!

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Apr 7, 2025
@dentiny
Copy link
Contributor

dentiny commented Apr 7, 2025

Just enabled ci for you, ping me when it's passed, thank you!

@hainesmichaelc hainesmichaelc added the community-contribution Contributed by the community label Apr 7, 2025
@Ziy1-Tan Ziy1-Tan force-pushed the subbazel_plasma branch 2 times, most recently from 3d668ff to 7207c64 Compare April 9, 2025 12:12
@Ziy1-Tan Ziy1-Tan requested a review from dentiny April 10, 2025 13:48
@Ziy1-Tan
Copy link
Contributor Author

Hey, @dentiny . The CI is good now:)

Copy link
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

thanks!

@dentiny
Copy link
Contributor

dentiny commented Apr 11, 2025

@edoakes / @jjyao could you please help merge? Thanks!

@edoakes edoakes merged commit 5ee54b8 into ray-project:master Apr 11, 2025
5 checks passed
han-steve pushed a commit to han-steve/ray that referenced this pull request Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Place plasma bazel targets into separate directories
5 participants