Skip to content

[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

Merged
merged 4 commits into from
Jun 21, 2025

Conversation

SolitaryThinker
Copy link
Contributor

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
image
image
image


After this PR
image
image
image

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

Signed-off-by: will.lin <will.lin@anyscale.com>
@Copilot Copilot AI review requested due to automatic review settings June 21, 2025 05:01
Copy link
Contributor

@Copilot Copilot AI left a 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]:

@pcmoritz
Copy link
Contributor

pcmoritz commented Jun 21, 2025

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.

@pcmoritz pcmoritz added the go add ONLY when ready to merge, run all tests label Jun 21, 2025
@SolitaryThinker
Copy link
Contributor Author

good point, forgot about this. Will take a look

@pcmoritz pcmoritz requested a review from a team as a code owner June 21, 2025 19:56
pcmoritz added 2 commits June 21, 2025 13:17
Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
@pcmoritz
Copy link
Contributor

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.

@pcmoritz pcmoritz enabled auto-merge (squash) June 21, 2025 22:03
@dayshah dayshah disabled auto-merge June 21, 2025 22:14
@dayshah
Copy link
Contributor

dayshah commented Jun 21, 2025

sorry, accidentally disabled auto-merge while looking at this 😵‍💫

but ty for doing this, have always been annoyed by this!!

@pcmoritz pcmoritz enabled auto-merge (squash) June 21, 2025 22:20
@pcmoritz pcmoritz merged commit efbc996 into ray-project:master Jun 21, 2025
7 checks passed
@SolitaryThinker SolitaryThinker deleted the will/actorproxy_type branch June 24, 2025 07:03
minerharry pushed a commit to minerharry/ray that referenced this pull request Jun 27, 2025
…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**

![image](https://github.com/user-attachments/assets/681a6572-b27d-46fb-aa7e-f2d4fddc55a0)

![image](https://github.com/user-attachments/assets/3e0f2a3f-33dd-41e9-bd05-657efd1e6f59)

![image](https://github.com/user-attachments/assets/da45eb6e-479e-4114-b4cc-cefd4cefafb2)

---

**After this PR**

![image](https://github.com/user-attachments/assets/c60943a8-7e9e-4baa-88aa-57122e617889)

![image](https://github.com/user-attachments/assets/55afe9bf-282f-45cf-b615-4facde9e1623)

![image](https://github.com/user-attachments/assets/73b65613-800a-4c24-968c-7062b3956410)


## 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>
elliot-barn pushed a commit that referenced this pull request Jul 2, 2025
…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**

![image](https://github.com/user-attachments/assets/681a6572-b27d-46fb-aa7e-f2d4fddc55a0)

![image](https://github.com/user-attachments/assets/3e0f2a3f-33dd-41e9-bd05-657efd1e6f59)

![image](https://github.com/user-attachments/assets/da45eb6e-479e-4114-b4cc-cefd4cefafb2)

---

**After this PR**

![image](https://github.com/user-attachments/assets/c60943a8-7e9e-4baa-88aa-57122e617889)

![image](https://github.com/user-attachments/assets/55afe9bf-282f-45cf-b615-4facde9e1623)

![image](https://github.com/user-attachments/assets/73b65613-800a-4c24-968c-7062b3956410)

## 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>
goutamvenkat-anyscale pushed a commit to goutamvenkat-anyscale/ray that referenced this pull request Jul 4, 2025
…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**

![image](https://github.com/user-attachments/assets/681a6572-b27d-46fb-aa7e-f2d4fddc55a0)

![image](https://github.com/user-attachments/assets/3e0f2a3f-33dd-41e9-bd05-657efd1e6f59)

![image](https://github.com/user-attachments/assets/da45eb6e-479e-4114-b4cc-cefd4cefafb2)

---

**After this PR**

![image](https://github.com/user-attachments/assets/c60943a8-7e9e-4baa-88aa-57122e617889)

![image](https://github.com/user-attachments/assets/55afe9bf-282f-45cf-b615-4facde9e1623)

![image](https://github.com/user-attachments/assets/73b65613-800a-4c24-968c-7062b3956410)

## 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>
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