Repository navigation
[core][__on_object_freed__][1/n] Detect the __on_object_freed__ hook on actor classes #67232
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| import sys | ||
|
|
||
| import pytest | ||
|
|
||
| import ray | ||
|
|
||
|
|
||
| @ray.remote | ||
| class NoHook: | ||
| def callback_state(self): | ||
| return ( | ||
| ray._private.worker.global_worker.core_worker.get_producer_freed_callback() | ||
| ) | ||
|
|
||
|
|
||
| @ray.remote | ||
| class SyncHook: | ||
| def __on_object_freed__(self, object_id): | ||
| pass | ||
|
|
||
| def callback_state(self): | ||
| return ( | ||
| ray._private.worker.global_worker.core_worker.get_producer_freed_callback() | ||
| ) | ||
|
|
||
|
|
||
| @ray.remote | ||
| class AsyncHook: | ||
| async def __on_object_freed__(self, object_id): | ||
| pass | ||
|
|
||
| def callback_state(self): | ||
| return ( | ||
| ray._private.worker.global_worker.core_worker.get_producer_freed_callback() | ||
| ) | ||
|
|
||
|
|
||
| @ray.remote(concurrency_groups={"freed": 1}) | ||
| class GroupHook: | ||
| @ray.method(concurrency_group="freed") | ||
| def __on_object_freed__(self, object_id): | ||
| pass | ||
|
|
||
| def callback_state(self): | ||
| return ( | ||
| ray._private.worker.global_worker.core_worker.get_producer_freed_callback() | ||
| ) | ||
|
|
||
|
Comment on lines
+38
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a test case for a @ray.remote(concurrency_groups={"freed": 1})\nclass GroupHook:\n @ray.method(concurrency_group="freed")\n def __on_object_freed__(self, object_id):\n pass\n\n def callback_state(self):\n return (\n ray._private.worker.global_worker.core_worker.get_producer_freed_callback()\n )\n\n\n@ray.remote(concurrency_groups={"freed": 1})\nclass GroupClassHook:\n @classmethod\n @ray.method(concurrency_group="freed")\n def __on_object_freed__(cls, object_id):\n pass\n\n def callback_state(self):\n return (\n ray._private.worker.global_worker.core_worker.get_producer_freed_callback()\n ) |
||
|
|
||
| class HookBase: | ||
| def __on_object_freed__(self, object_id): | ||
| pass | ||
|
|
||
|
|
||
| @ray.remote | ||
| class InheritedHook(HookBase): | ||
| def callback_state(self): | ||
| return ( | ||
| ray._private.worker.global_worker.core_worker.get_producer_freed_callback() | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "actor_cls, expected", | ||
| [ | ||
| (NoHook, (False, "")), | ||
| (SyncHook, (True, "")), | ||
| (AsyncHook, (True, "")), | ||
| (GroupHook, (True, "freed")), | ||
| (InheritedHook, (True, "")), | ||
| ], | ||
| ) | ||
|
Comment on lines
+63
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include @pytest.mark.parametrize(\n "actor_cls, expected",\n [\n (NoHook, (False, "")),\n (SyncHook, (True, "")),\n (AsyncHook, (True, "")),\n (GroupHook, (True, "freed")),\n (GroupClassHook, (True, "freed")),\n (InheritedHook, (True, "")),\n ],\n) |
||
| def test_hook_detected_at_actor_creation(ray_start_regular_shared, actor_cls, expected): | ||
| actor = actor_cls.remote() | ||
| assert ray.get(actor.callback_state.remote()) == expected | ||
|
|
||
|
|
||
| def test_driver_has_no_hook(ray_start_regular_shared): | ||
| core_worker = ray._private.worker.global_worker.core_worker | ||
| assert core_worker.get_producer_freed_callback() == (False, "") | ||
|
|
||
|
|
||
| def test_hook_not_callable_through_handle(ray_start_regular_shared): | ||
| actor = SyncHook.remote() | ||
| with pytest.raises(AttributeError, match="is called by Ray"): | ||
| actor.__on_object_freed__.remote("00") | ||
| assert ray.get(actor.callback_state.remote()) == (True, "") | ||
|
|
||
|
|
||
| class AsyncDef: | ||
| async def __on_object_freed__(self, object_id): | ||
| pass | ||
|
|
||
|
|
||
| class ExtraOptionalArg: | ||
| def __on_object_freed__(self, object_id, extra=None): | ||
| pass | ||
|
|
||
|
|
||
| class VarArgs: | ||
| def __on_object_freed__(self, *args): | ||
| pass | ||
|
|
||
|
|
||
| class StaticHook: | ||
| @staticmethod | ||
| def __on_object_freed__(object_id): | ||
| pass | ||
|
|
||
|
|
||
| class ClassHook: | ||
| @classmethod | ||
| def __on_object_freed__(cls, object_id): | ||
| pass | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "cls", [HookBase, AsyncDef, ExtraOptionalArg, VarArgs, StaticHook, ClassHook] | ||
| ) | ||
| def test_valid_hook_signatures_accepted(cls): | ||
| ray.remote(cls) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Signature tests belong in unitLow Severity 💡 This test appears to only test pure Python logic. Consider moving it to Additional Locations (1)Triggered by project rule: Bugbot Rules Reviewed by Cursor Bugbot for commit a2d0bca. Configure here. |
||
|
|
||
| class NoArg: | ||
| def __on_object_freed__(self): | ||
| pass | ||
|
|
||
|
|
||
| class TwoArgs: | ||
| def __on_object_freed__(self, object_id, other): | ||
| pass | ||
|
|
||
|
|
||
| class KeywordOnlyArg: | ||
| def __on_object_freed__(self, *, object_id): | ||
| pass | ||
|
|
||
|
|
||
| class StaticNoArg: | ||
| @staticmethod | ||
| def __on_object_freed__(): | ||
| pass | ||
|
|
||
|
|
||
| class Generator: | ||
| def __on_object_freed__(self, object_id): | ||
| yield object_id | ||
|
|
||
|
|
||
| class AsyncGenerator: | ||
| async def __on_object_freed__(self, object_id): | ||
| yield object_id | ||
|
|
||
|
|
||
| class NotAMethod: | ||
| __on_object_freed__ = None | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "cls, match", | ||
| [ | ||
| (NoArg, "callable with one argument"), | ||
| (TwoArgs, "callable with one argument"), | ||
| (KeywordOnlyArg, "callable with one argument"), | ||
| (StaticNoArg, "callable with one argument"), | ||
| (Generator, "can't be a generator"), | ||
| (AsyncGenerator, "can't be a generator"), | ||
| (NotAMethod, "must be a method"), | ||
| ], | ||
| ) | ||
| def test_invalid_hook_rejected(cls, match): | ||
| with pytest.raises(TypeError, match=match): | ||
| ray.remote(cls) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(pytest.main(["-sv", __file__])) | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
__on_object_freed__is defined as a@classmethod,getattr(actor_class, "__on_object_freed__")returns a bound method. In Python, bound methods do not delegate attribute lookup to their underlying functions, so any attributes set by decorators like@ray.method(concurrency_group=...)on the underlying function will not be found directly on the bound method.\n\nTo fix this, we should unwrap the bound method to its underlying function usinggetattr(on_object_freed_hook, "__func__", on_object_freed_hook)before looking up__ray_concurrency_group__.