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]: Enable get_actor_name for actor runtime context #39347

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

jonathan-anyscale
Copy link
Contributor

@jonathan-anyscale jonathan-anyscale commented Sep 6, 2023

Why are these changes needed?

Expose current actor's name through RuntimeContext()

Related issue number

Closes #35849

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

Jonathan Nitisastro added 2 commits September 6, 2023 15:39
Signed-off-by: Jonathan Nitisastro <jonathannitisastro@jonathancn-QC69NQYVVG.local.meter>
Signed-off-by: Jonathan Nitisastro <jonathannitisastro@jonathancn-QC69NQYVVG.local.meter>
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

Can you add a test?

python/ray/runtime_context.py Outdated Show resolved Hide resolved
python/ray/runtime_context.py Outdated Show resolved Hide resolved
python/ray/runtime_context.py Outdated Show resolved Hide resolved
python/ray/runtime_context.py Outdated Show resolved Hide resolved
python/ray/runtime_context.py Outdated Show resolved Hide resolved
python/ray/runtime_context.py Outdated Show resolved Hide resolved
@@ -346,6 +346,19 @@ def foo(self):
ray.get(actor.foo.remote())


def test_actor_name(ray_start_regular):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit can you just add this test in the previous test test_ids? Having a new suite will do ray.init which is pretty slow (and I think this test is too small, so it is probably better just piggybacking to the previous test)

Copy link
Contributor Author

@jonathan-anyscale jonathan-anyscale Sep 7, 2023

Choose a reason for hiding this comment

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

Gotcha! I merged it with the test_ids

jonathan-anyscale and others added 2 commits September 7, 2023 09:19
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
Copy link
Contributor

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

Nice work!

python/ray/tests/test_runtime_context.py Show resolved Hide resolved
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
…_name

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
…into expose_actor_name

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

Returns:
The current actor name of this worker.
Return empty string if there's no actor name or if it's a driver
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring is inconsistent.

If it is a driver == if it is not an actor

but it says

if it is a driver -> empty string
if it is not an actor -> None.

Can you update the docstring to

If a current worker is an actor, and if actor name doesn't exist, it returns an empty string.

If a current worker is not an actor, it returns None. 

assert ray.get(named_actor.name.remote()) == ACTOR_NAME

# unnamed actor name
unnamed_actor = NamedActor.options().remote()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests to verify all the specs from the API?

Make sure if it is a task, it returns "None".
Make sure if it is a driver, it returns "None".
Make sure if it has no actor name, it returns "".
Make sure if it is an actor and has a name, it returns the name

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>
@rkooo567 rkooo567 merged commit a8fe351 into ray-project:master Sep 11, 2023
105 of 112 checks passed
jimthompson5802 pushed a commit to jimthompson5802/ray that referenced this pull request Sep 12, 2023
…39347)

Expose current actor's name through RuntimeContext()

Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
simonsays1980 pushed a commit to simonsays1980/ray that referenced this pull request Sep 12, 2023
…39347)

Expose current actor's name through RuntimeContext()

Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
…39347)

Expose current actor's name through RuntimeContext()

Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] [FR] Access actor name (and other options) from within the actor
3 participants