8000 [Core] Split stats_metric into smaller targets to improve build performance by 400Ping · Pull Request #50595 · ray-project/ray · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Core] Split stats_metric into smaller targets to improve build performance #50595

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

Jump to bottom
Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

[Core] Split stats_metric into smaller targets to improve build performance #50595

wants to merge 17 commits into from

Conversation

400Ping
Copy link
Contributor
@400Ping 400Ping commented Feb 14, 2025

Why are these changes needed?

As of now, ray core uses bazel as build system, which encourages small targets.
Ray core is not following the best practice, which bundles all related files into several giant targets.

Related issue number

Closes: #50588

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

@400Ping 400Ping force-pushed the build/split-stats-metric branch from 64c3c25 to 64a9360 Compare February 14, 2025 06:51
@400Ping 400Ping marked this pull request as draft February 14, 2025 07:24
@400Ping 400Ping force-pushed the build/split-stats-metric branch from f021322 to 83dc5ae Compare February 14, 2025 09:42
@400Ping 400Ping marked this pull request as ready for review February 14, 2025 10:59
dentiny
dentiny reviewed Feb 14, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 14, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 14, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 15, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 15, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 15, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 15, 2025
View reviewed changes
dentiny
dentiny reviewed Feb 15, 2025
View reviewed changes
dentiny
dentiny approved these changes Feb 15, 2025
View reviewed changes
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.

LGTM, thanks for the effort!

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Feb 15, 2025
@400Ping
Copy link
Contributor Author
400Ping commented Feb 17, 2025

@dentiny what's wrong with the premerge?

@dentiny
Copy link
Contributor
dentiny commented Feb 17, 2025

@dentiny what's wrong with the premerge?

I will take a look later tonight, feel free to ping me directly!

dentiny
dentiny reviewed Feb 19, 2025
View reviewed changes
BUILD.bazel Outdated
"-lpthread",
],
"@platforms//os:windows": [],
"//conditions:default": ["-lpthread"],
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need the link option

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 seems we need this link option, or else it will show

ERROR: /ray/BUILD.bazel:815:15: configurable attribute "linkopts" in //:stats_lib doesn't match this configuration. Would a default condition help?
[2025-02-20T13:46:24Z]

@400Ping 400Ping force-pushed the build/split-stats-metric branch from 2114afc to 1e166a8 Compare February 20, 2025 13:17
400Ping added 10 commits February 28, 2025 20:39
@400Ping
[Refactor] Remove unnecessary includes and optimize dependencies
6ff8eec 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[Refactor] Remove unnecessary includes and add dependencies
c312be7 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[Chore] Reorder and fix dependences
c56d192 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[chore] remove link option
9a1b6b1 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[chore] Add linkopts for macos and linux
adef0b9 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[chore] Remove macos link option
1395334 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[Refactor] Refactor stats_metric
030c369 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
[fix] restore stats_lib
b614a15 
Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
Revert "[fix] restore stats_lib"
3bdaa5d 
This reverts commit a6cba67.

Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping
Revert "[Refactor] Refactor stats_metric"
94f79f7 
This reverts commit b3e6d42.

Signed-off-by: 400Ping <43886578+400Ping@users.noreply.github.com>
@400Ping 400Ping force-pushed the build/split-stats-metric branch from 6459efa to 94f79f7 Compare February 28, 2025 12:39
@400Ping 400Ping marked this pull request as draft March 15, 2025 11:28
@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Mar 27, 2025
@hainesmichaelc hainesmichaelc added the community-contribution Contributed by the community label Apr 4, 2025
@stale Stale
Copy link
stale bot commented May 6, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label May 6, 2025
@ 67E6 400Ping
Copy link
Contributor Author
400Ping commented May 6, 2025

Work in process

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label May 6, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Jun 6, 2025

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@github-actions github-actions bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 6, 2025
@400Ping
Copy link
Contributor Author
400Ping commented Jun 6, 2025

working on it

@github-actions github-actions bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 7, 2025
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@github-actions github-actions bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 22, 2025
@400Ping
Copy link
Contributor Author
400Ping commented Jun 22, 2025

Will be working on it

@github-actions github-actions bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dentiny dentiny dentiny approved these changes

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo

@justinvyu justinvyu Awaiting requested review from justinvyu

@matthewdeng matthewdeng Awaiting requested review from matthewdeng

@raulchen raulchen Awaiting requested review from raulchen

@woshiyyya woshiyyya Awaiting requested review from woshiyyya

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@pcmoritz pcmoritz Awaiting requested review from pcmoritz

@kevin85421 kevin85421 Awaiting requested review from kevin85421

@sven1977 sven1977 Awaiting requested review from sven1977

@simonsays1980 simonsays1980 Awaiting requested review from simonsays1980

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang

@richardliaw richardliaw Awaiting requested review from richardliaw

@edoakes edoakes Awaiting requested review from edoakes

@hongchaodeng hongchaodeng Awaiting requested review from hongchaodeng

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core] Split giant ray core C++ targets into small ones(stats)
5 participants
@400Ping @dentiny @jjyao @hainesmichaelc @jcotant1
0