Skip to content

[core] Place object_manager bazel targets into separate directories #52285

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

Merged
merged 4 commits into from
Apr 23, 2025

Conversation

Ziy1-Tan
Copy link
Contributor

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

Why are these changes needed?

Related issue number

Closes #52268

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 :(

@Ziy1-Tan
Copy link
Contributor Author

PTAL @dentiny

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Apr 13, 2025
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.

Thank you so much for cleaning up!

@dentiny
Copy link
Contributor

dentiny commented Apr 13, 2025

Just FYI, we have flaky test on dashboard these days, so might take longer to pass all CI; ping me when it passes, or you think failed tests irrelevant, thanks!

@Ziy1-Tan
Copy link
Contributor Author

Hey, @dentiny . The ci is ok :)

@dentiny
Copy link
Contributor

dentiny commented Apr 16, 2025

I just enabled windows and mac test for you --- we've met some nasty platform specific prod issues these day, better to be more careful. Ping me when green.

@hainesmichaelc hainesmichaelc added the community-contribution Contributed by the community label Apr 16, 2025
@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Apr 16, 2025
@Ziy1-Tan Ziy1-Tan requested a review from dentiny April 17, 2025 23:45
@Ziy1-Tan
Copy link
Contributor Author

@dentiny , Ci is green now :)

@Ziy1-Tan
Copy link
Contributor Author

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

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 for the help! Code change LGTM
CC @dayshah as well, we need to check windows/mac build before merge.

@jjyao
Copy link
Collaborator

jjyao commented Apr 21, 2025

Triggered windows test.

@jjyao
Copy link
Collaborator

jjyao commented Apr 21, 2025

[2025-04-21T18:53:06Z] //src/ray/object_manager/test:ownership_based_object_directory_test      FAILED in 3 out of 3 in 0.1s

Maybe the target name is too long for Windows? cc @dayshah

@dayshah
Copy link
Contributor

dayshah commented Apr 21, 2025

[2025-04-21T18:53:06Z] //src/ray/object_manager/test:ownership_based_object_directory_test      FAILED in 3 out of 3 in 0.1s

Maybe the target name is too long for Windows? cc @dayshah

ERROR: src/main/native/windows/util.cc(262): GetShortPathNameW(\\?\C:\raytmp\6w7zvoxv\execroot\com_github_ray_project_ray\bazel-out\x64_windows-opt\bin\src\ray\object_manager\test\ownership_based_object_directory_test.exe.runfiles\com_github_ray_project_ray\src\ray\object_manager\test\ownership_based_object_directory_test.exe): cannot shorten the path enough

yup @Ziy1-Tan Can you change the name of the test and related source files to ownership_object_directory.

@Ziy1-Tan Ziy1-Tan force-pushed the subbazel_objm branch 2 times, most recently from 6a58c48 to 1020202 Compare April 22, 2025 15:08
.bazelrc Outdated
Comment on lines 3 to 4
build --local_ram_resources=HOST_RAM*.7 --local_cpu_resources=4
build --disk_cache=~/bazel-cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I'll remove these. They are only used locally.

@Ziy1-Tan Ziy1-Tan requested a review from jjyao April 23, 2025 04:46
@jjyao jjyao merged commit ede4d8b into ray-project:master Apr 23, 2025
4 of 5 checks passed
ktyxx pushed a commit to ktyxx/ray that referenced this pull request Apr 29, 2025
vickytsang pushed a commit to ROCm/ray that referenced this pull request May 5, 2025
zhaoch23 pushed a commit to Bye-legumes/ray that referenced this pull request May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-backlog 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 object_manager targets into separate directories
6 participants