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

[Serve] Add hash function of RunningReplicaInfo #32772

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions python/ray/serve/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -549,4 +549,12 @@ py_test(
srcs = serve_tests_srcs,
tags = ["exclusive", "team:serve"],
deps = [":serve_lib"],
)

py_test(
name = "test_common",
size = "small",
srcs = serve_tests_srcs,
tags = ["exclusive", "team:serve"],
deps = [":serve_lib"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This test file wasn't running in CI before? 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly...

)
23 changes: 23 additions & 0 deletions python/ray/serve/_private/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,3 +292,26 @@ class RunningReplicaInfo:
actor_handle: ActorHandle
max_concurrent_queries: int
is_cross_language: bool = False

def __post_init__(self):
# Set hash value when object is constructed.
# We use _acotor_id to hash the ActorHandle object
sihanwang41 marked this conversation as resolved.
Show resolved Hide resolved
# instead of actor_handle itself to make sure
# it is consistently same actor handle between different
# object ids.

hash_val = hash(
" ".join(
[
self.deployment_name,
self.replica_tag,
str(self.actor_handle._actor_id),
str(self.max_concurrent_queries),
str(self.is_cross_language),
]
)
)
object.__setattr__(self, "_hash", hash_val)
Copy link
Contributor

Choose a reason for hiding this comment

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

For my education what's the benefit of this over self._hash = hash_val? If not obvious maybe add a code comment

Copy link
Contributor Author

@sihanwang41 sihanwang41 Feb 23, 2023

Choose a reason for hiding this comment

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

oh it is a hacky way to set attribute, since we use frozen for this dataclass class. (Not allow to set attribute as you mentioned)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! Would be good to put this in a code comment


def __hash__(self):
return self._hash
18 changes: 18 additions & 0 deletions python/ray/serve/tests/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
DeploymentStatusInfo,
ApplicationStatus,
ApplicationStatusInfo,
RunningReplicaInfo,
)
from ray.serve.generated.serve_pb2 import (
StatusOverview as StatusOverviewProto,
Expand Down Expand Up @@ -185,6 +186,23 @@ def test_proto(self, application_status):
assert status_info == reconstructed_info


def test_running_replica_info():
"""Test hash value of RunningReplicaInfo"""

class FakeActorHandler:
def __init__(self, actor_id):
self._actor_id = actor_id

fake_h1 = FakeActorHandler("1")
fake_h2 = FakeActorHandler("1")
assert fake_h1 != fake_h2
replica1 = RunningReplicaInfo("my_deployment", "1", fake_h1, 1, False)
replica2 = RunningReplicaInfo("my_deployment", "1", fake_h2, 1, False)
replica3 = RunningReplicaInfo("my_deployment", "1", fake_h2, 1, True)
assert replica1._hash == replica2._hash
assert replica3._hash != replica1._hash


if __name__ == "__main__":
import sys

Expand Down
11 changes: 8 additions & 3 deletions python/ray/serve/tests/test_deployment_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import pytest

import ray
from ray.actor import ActorHandle
from ray.serve._private.common import (
DeploymentConfig,
DeploymentInfo,
Expand All @@ -33,6 +32,11 @@
from ray.util.scheduling_strategies import NodeAffinitySchedulingStrategy


class MockActorHandle:
def __init__(self):
self._actor_id = "fake_id"


class MockReplicaActorWrapper:
def __init__(
self,
Expand Down Expand Up @@ -67,6 +71,7 @@ def __init__(
self.healthy = True
self._is_cross_language = False
self._scheduling_strategy = scheduling_strategy
self._actor_handle = MockActorHandle()

@property
def is_cross_language(self) -> bool:
Expand All @@ -81,8 +86,8 @@ def deployment_name(self) -> str:
return self._deployment_name

@property
def actor_handle(self) -> ActorHandle:
return None
def actor_handle(self) -> MockActorHandle:
return self._actor_handle

@property
def max_concurrent_queries(self) -> int:
Expand Down