-
Notifications
You must be signed in to change notification settings - Fork 6.6k
[core] Fix ActorClass.remote return typing and expose Actor class methods to static analysis #53986
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
[core] Fix ActorClass.remote return typing and expose Actor class methods to static analysis #53986
Conversation
Signed-off-by: will.lin <will.lin@anyscale.com>
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.
Pull Request Overview
This PR fixes the mis-typing of ActorClass.remote() by introducing generics, which improves type inference for IDEs and static analysis.
- Added generic type parameters to ActorClass, ActorHandle, and the remote()/_remote() methods.
- Introduced a new type alias (ActorProxy) for returning actor handles.
- Updated type overloads in worker.py to support the generic ActorClass.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
python/ray/actor.py | Added generics to ActorClass and ActorHandle; updated remote methods. |
python/ray/_private/worker.py | Updated overload for remote actor instantiation to use type hints. |
Comments suppressed due to low confidence (2)
python/ray/actor.py:921
- Consider updating the docstring for the 'remote' method to explicitly mention its new generic return type (ActorProxy[T]) for improved clarity.
def remote(self, *args, **kwargs) -> ActorProxy[T]:
python/ray/actor.py:1072
- Update the '_remote' method docstring to include details about its generic return type (ActorProxy[T]) to ensure consistency with the updated API design.
def _remote(self, args=None, kwargs=None, **actor_options) -> ActorProxy[T]:
Thanks, great job! Are there any other occurences of ActorHandle we need to change to ActorProxy? If the type checker figures out that the ActorProxy is actually an ActorHandle, it will not show the completion of the actor's methods, so we need to make sure that ActorHandles are typed as ActorProxy everywhere. And some type checkers are quite smart about this and will do type propagation from all the uses of a variable. |
good point, forgot about this. Will take a look |
Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
Ok, I checked a little more about the ActorHandle stuff and I think that's not a problem at the moment since actor handles are almost never explicitly passed into something that is typed as actor handle, so for now it is probably better to keep the changes minimal. I'll merge the PR when the tests pass. |
sorry, accidentally disabled auto-merge while looking at this 😵💫 but ty for doing this, have always been annoyed by this!! |
…hods to static analysis (ray-project#53986) <!-- Thank you for your contribution! Please review https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before opening a pull request. --> <!-- Please add a reviewer to the assignee section when you create a PR. If you don't have the access to it, we will shortly find a reviewer and assign them to your PR. --> ## Why are these changes needed? Changes in this PR are inspired by https://github.com/zen-xu/sunray Calling `remote()` on an Actor class incorrectly returns an `ObjectRef[...]` type. Also, the Actor class's methods are not available to IDEs, as the type is incorrect. This PR makes `ActorHandle` a generic type `ActorHandle[T]` and also creates an type alias: `ActorProxy = Union["ActorHandle[T]", type[T]]` to expose the method signatures from the Actor class. Here are some other possible candidates for the type name instead of `ActorProxy` from claude: ActorInterface TypedActorHandle RemoteActorType ActorProxy ActorT cc @richardliaw @pcmoritz We'll add better type hints for actor methods in a follow up PR. The following are screenshots demonstrating the type hints in Cursor before and after this PR: **Before this PR**    --- **After this PR**    ## Related issue number <!-- For example: "Closes ray-project#1234" --> ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] 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 :( --------- Signed-off-by: will.lin <will.lin@anyscale.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com>
…hods to static analysis (#53986) <!-- Thank you for your contribution! Please review https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before opening a pull request. --> <!-- Please add a reviewer to the assignee section when you create a PR. If you don't have the access to it, we will shortly find a reviewer and assign them to your PR. --> ## Why are these changes needed? Changes in this PR are inspired by https://github.com/zen-xu/sunray Calling `remote()` on an Actor class incorrectly returns an `ObjectRef[...]` type. Also, the Actor class's methods are not available to IDEs, as the type is incorrect. This PR makes `ActorHandle` a generic type `ActorHandle[T]` and also creates an type alias: `ActorProxy = Union["ActorHandle[T]", type[T]]` to expose the method signatures from the Actor class. Here are some other possible candidates for the type name instead of `ActorProxy` from claude: ActorInterface TypedActorHandle RemoteActorType ActorProxy ActorT cc @richardliaw @pcmoritz We'll add better type hints for actor methods in a follow up PR. The following are screenshots demonstrating the type hints in Cursor before and after this PR: **Before this PR**    --- **After this PR**    ## Related issue number <!-- For example: "Closes #1234" --> ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] 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 :( --------- Signed-off-by: will.lin <will.lin@anyscale.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
…hods to static analysis (ray-project#53986) <!-- Thank you for your contribution! Please review https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before opening a pull request. --> <!-- Please add a reviewer to the assignee section when you create a PR. If you don't have the access to it, we will shortly find a reviewer and assign them to your PR. --> ## Why are these changes needed? Changes in this PR are inspired by https://github.com/zen-xu/sunray Calling `remote()` on an Actor class incorrectly returns an `ObjectRef[...]` type. Also, the Actor class's methods are not available to IDEs, as the type is incorrect. This PR makes `ActorHandle` a generic type `ActorHandle[T]` and also creates an type alias: `ActorProxy = Union["ActorHandle[T]", type[T]]` to expose the method signatures from the Actor class. Here are some other possible candidates for the type name instead of `ActorProxy` from claude: ActorInterface TypedActorHandle RemoteActorType ActorProxy ActorT cc @richardliaw @pcmoritz We'll add better type hints for actor methods in a follow up PR. The following are screenshots demonstrating the type hints in Cursor before and after this PR: **Before this PR**    --- **After this PR**    ## Related issue number <!-- For example: "Closes ray-project#1234" --> ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] 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 :( --------- Signed-off-by: will.lin <will.lin@anyscale.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com> Signed-off-by: Goutam V <goutam@anyscale.com>
Why are these changes needed?
Changes in this PR are inspired by https://github.com/zen-xu/sunray
Calling
remote()
on an Actor class incorrectly returns anObjectRef[...]
type. Also, the Actor class's methods are not available to IDEs, as the type is incorrect.This PR makes
ActorHandle
a generic typeActorHandle[T]
and also creates an type alias:ActorProxy = Union["ActorHandle[T]", type[T]]
to expose the method signatures from the Actor class.Here are some other possible candidates for the type name instead of
ActorProxy
from claude:ActorInterface
TypedActorHandle
RemoteActorType
ActorProxy
ActorT
cc @richardliaw @pcmoritz
We'll add better type hints for actor methods in a follow up PR.
The following are screenshots demonstrating the type hints in Cursor before and after this PR:
Before this PR



After this PR



Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.