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

Jump to bottom

[Core] Cover cpplint for /src/ray/core_worker (excluding transport) #51557

Merged
merged 8 commits into from
Mar 22, 2025
Merged

[Core] Cover cpplint for /src/ray/core_worker (excluding transport) #51557

merged 8 commits into from
Mar 22, 2025
+324 −67

Conversation

nishi-t
Copy link
Contributor

@nishi-t nishi-t commented Mar 20, 2025 •
edited
Loading

Why are these changes needed?

As part of the initiative to introduce cpplint into the pre-commit hook, we are gradually cleaning up C++ folders to ensure compliance with code style requirements. This issue focuses on cleaning up /src/ray/core_worker (excluding transport).

Related issue number

Closes #51510

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

Sorry, something went wrong.

@nishi-t nishi-t force-pushed the cpplint_core_worker branch from a2c84c2 to 299d947 Compare March 20, 2025 13:51
dentiny
dentiny reviewed Mar 20, 2025
View reviewed changes
src/ray/core_worker/core_worker.cc
@@ -1003,7 +1009,7 @@ CoreWorker::CoreWorker(CoreWorkerOptions options, const WorkerID &worker_id)
} else {
ConnectToRayletInternal();
}
}
} // NOLINT(readability/fn_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, first time see this error

dentiny
dentiny reviewed Mar 20, 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.

Thank you! My only comment is to add it to precommit hook file.

@nishi-t nishi-t force-pushed the cpplint_core_worker branch from 299d947 to 23966da Compare March 21, 2025 00:02
@nishi-t
Copy link
Contributor Author

nishi-t commented Mar 21, 2025 •
edited
Loading

I realize I missed fixing some lint errors and will correct them.

@nishi-t nishi-t force-pushed the cpplint_core_worker branch from 8a7d888 to e0b5ae2 Compare March 21, 2025 07:38
@dentiny
Copy link
Contributor

dentiny commented Mar 21, 2025

You need to run linter ./ci/lint/lint.sh pre_commit

nishi-t added 5 commits March 22, 2025 00:08
@nishi-t
Cover cpplint for /src/ray/core_worker
073a5ea 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t
Update pre-commit-config for adding src/ray/core_worker
97931ef 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t
Update pre-commit config and fix lint errors.
ce746bd 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t
Removed a commented-out line.
759d040 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t
Enable cpplint for subdirectories under core_worker and update pre-co…
Loading
Loading status checks…
c6e164c 
…mmit-config for excluding autogenerated header files.

Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t nishi-t force-pushed the cpplint_core_worker branch from 2399060 to c6e164c Compare March 21, 2025 15:09
@nishi-t
Add NOLINT to #include jni_utils.h to suppress cpplint warning
Loading
Loading status checks…
cf8228f 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t nishi-t force-pushed the cpplint_core_worker branch from 5ac2577 to cf8228f Compare March 21, 2025 16:36
@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Mar 21, 2025
@nishi-t
Copy link
Contributor Author

nishi-t commented Mar 21, 2025

@dentiny
Thank you for your advice.
Sorry, I made a mistake when checking lint locally.
cpplint already covers core_worker, including subdirectories.
Could you review it again?

dentiny
dentiny reviewed Mar 21, 2025
View reviewed changes
src/ray/core_worker/lib/java/io_ray_runtime_object_NativeObjectStore.cc Outdated
#include "ray/common/id.h"
#include "ray/core_worker/common.h"
#include "ray/core_worker/core_worker.h"
#include "jni_utils.h" // NOLINT(build/include_subdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious did cpplint make this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since generally header inclusions happen in alphabetical order

Copy link
Contributor Author

@nishi-t nishi-t Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My mistake. I reordered the includes during the trial-and-error process.
I have now restored the original include position and ran scripts/format.sh.

dentiny
dentiny reviewed Mar 21, 2025
View reviewed changes
src/ray/core_worker/test/direct_actor_transport_test.cc
@@ -27,10 +31,6 @@
#include "mock/ray/core_worker/reference_count.h"
// clang-format on

// clang-format off
#include "mock/ray/core_worker/task_manager.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Mar 21, 2025
nishi-t added 2 commits March 22, 2025 09:19
@nishi-t
Restore the original include position of jni_utils.h
8434987 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@nishi-t
Run format script
Loading
Loading status checks…
f935b4a 
Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
@dentiny
Copy link
Contributor

dentiny commented Mar 22, 2025

@nishi-t Thanks for your help! Feel free to ping me when CI passes

@nishi-t
Copy link
Contributor Author

nishi-t commented Mar 22, 2025 •
edited
Loading

@dentiny Thank you for your review! The CI has passed.

dentiny
dentiny approved these changes Mar 22, 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.

LGMT, thanks!

@dentiny
Copy link
Contributor

dentiny commented Mar 22, 2025

Hi @aslonnie / @jjyao / @edoakes could you please help merge the PR? Thanks!

@jjyao jjyao merged commit 07c24e9 into ray-project:master Mar 22, 2025
5 checks passed
dhakshin32 pushed a commit to dhakshin32/ray that referenced this pull request Mar 27, 2025
@nishi-t
[Core] Cover cpplint for /src/ray/core_worker (excluding transport) (r…
ded8e59 
…ay-project#51557)

Signed-off-by: Tatsuya Nishiyama <nishiyama.tatsuya0@gmail.com>
Signed-off-by: Dhakshin Suriakannu <d_suriakannu@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dentiny dentiny

Assignees
No one assigned
Labels
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] Cover cpplint for ray/core_worker (excluding transport)
4 participants
@nishi-t @dentiny @jjyao @jcotant1