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

[refactor] Move custom_types from util/state to _private/ #50183

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Feb 3, 2025

Why are these changes needed?

custom_types.py defines valid options for the Ray system, such as task status and actor status. However, some parts of the Ray codebase redefine this set of valid options or hardcode it as strings in multiple places. It will be useful to move custom_types to the top-level of the repo so that many other places can import it.

Follow up:

  1. Use custom_types to refactor some functions.
  2. Define class TypeReferenceType in a place other than dashboard.

Related issue number

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

@@ -72,9 +71,19 @@
"DRIVER_TASK",
]
TypeTaskType = Literal[tuple(TASK_TYPE)]
TypeReferenceType = Literal[
tuple(reference_type.value for reference_type in ReferenceType)
# TODO(kevin85421): `class ReferenceType(Enum)` is defined in
Copy link
Member Author

Choose a reason for hiding this comment

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

To make this PR easier to review, I will open a follow up PR to address this instead of including in this PR.

class ReferenceType(Enum):

Signed-off-by: kaihsun <[email protected]>
@kevin85421 kevin85421 force-pushed the 20250202-devbox1-tmux-19-ray2 branch from 8b366a5 to 8a513f2 Compare February 3, 2025 08:07
Signed-off-by: kaihsun <[email protected]>
@kevin85421 kevin85421 marked this pull request as ready for review February 3, 2025 19:55
@kevin85421 kevin85421 added the go add ONLY when ready to merge, run all tests label Feb 3, 2025
# `dashboard/memory_utils.py` to avoid complex dependencies. I redefined
# it here. Eventually, we should remove the one in `dashboard/memory_utils.py`
# and define it under `ray/_private`.
REFERENCE_TYPE = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we have two REFERENCE_TYPEs in our codebase, one for enum list, another for string list;
wondering if it's possible to combine them into one?

Copy link
Contributor

Choose a reason for hiding this comment

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

NVM, see you TODO comment.

@jjyao jjyao merged commit b99ad3a into ray-project:master Feb 5, 2025
5 checks passed
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