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

[Core] Add options(...) function to @ray.remote stubs #47330

Merged
merged 7 commits into from
Sep 23, 2024

Conversation

milesvant
Copy link
Contributor

@milesvant milesvant commented Aug 26, 2024

Why are these changes needed?

Removes incorrect mypy failures resulting from using @ray.remote.

On my reproduction script:

import ray


@ray.remote
def fn(x: int) -> int:
    return 2 * x

def main() -> None:
    fn_with_cpus = fn.options(num_cpus=1)
    print(ray.get(fn_with_cpus.remote(4)))

if __name__ == "__main__":
    ray.init()
    main()
    ray.shutdown()

with nightly Ray, mypy script.py produces error: "RemoteFunction0[int, int]" has no attribute "options" [attr-defined] whereas my branch produces no error.

Related issue number

Closes #47329.

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

@milesvant milesvant force-pushed the mvt/remote-fn-options branch 2 times, most recently from 0fb26b7 to 32b71af Compare August 26, 2024 05:59
@milesvant
Copy link
Contributor Author

Seems like the failing rllib tests are unrelated to me

@anyscalesam anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) core Issues that should be addressed in Ray Core labels Aug 26, 2024
@anyscalesam anyscalesam added P2 Important issue, but not time-critical observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Sep 4, 2024
@anyscalesam
Copy link
Contributor

@ruisearch42 since this is on the worker side of the house can you PTAL? @can-anyscale any recommendation on how we can skip/pass the seemingly unrelated RLLib failing tests?

TY for the contribution @milesvant

Copy link
Contributor

@ruisearch42 ruisearch42 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Left a couple questions, but otherwise LGTM.

python/ray/_private/worker.py Outdated Show resolved Hide resolved
RF = TypeVar("RF", bound="HasOptions")


class HasOptions(Protocol):
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that generally it is better to have proper type annotations. Just wondering in this particular case, are you trying to achieve something specific? What is the use case that you were trying to enable? IIUC currently Ray runs mypy on a limited number of files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I work in a codebase that uses the pattern in the description/issue frequently, and has strict type checking. Without this sort of annotation I have to ignore typechecking on most usages of Ray

@milesvant
Copy link
Contributor Author

@ruisearch42 What can I do to get this merged? I think the failing tests are flaky

@ruisearch42
Copy link
Contributor

@rkooo567 Does this look good to you? If so, we can ask CI team to force merge.

@milesvant
Copy link
Contributor Author

@ruisearch42 @rkooo567 bump

@ruisearch42
Copy link
Contributor

@milesvant can you merge master and retry?

@milesvant
Copy link
Contributor Author

@ruisearch42 microcheck now green

@ruisearch42
Copy link
Contributor

@jjyao @rynewang (committers) Can you help merge this unless you have more comments?

@milesvant
Copy link
Contributor Author

@ruisearch42 @jjyao @rynewang bump

@rynewang rynewang enabled auto-merge (squash) September 23, 2024 19:49
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Sep 23, 2024
@rynewang rynewang merged commit 92a0269 into ray-project:master Sep 23, 2024
7 checks passed
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
)

## Why are these changes needed?

Removes incorrect `mypy` failures resulting from using `@ray.remote`.

On my reproduction script:
```
import ray

@ray.remote
def fn(x: int) -> int:
    return 2 * x

def main() -> None:
    fn_with_cpus = fn.options(num_cpus=1)
    print(ray.get(fn_with_cpus.remote(4)))

if __name__ == "__main__":
    ray.init()
    main()
    ray.shutdown()
```
with nightly Ray, `mypy script.py` produces `error:
"RemoteFunction0[int, int]" has no attribute "options" [attr-defined]`
whereas my branch produces no error.

Signed-off-by: Miles Van Tongeren <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling P2 Important issue, but not time-critical
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] mypy failure when .options(...) is used on functions decorated with @ray.remote
4 participants