-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[Core]: Enable get_actor_name for actor runtime context #39347
Conversation
Signed-off-by: Jonathan Nitisastro <[email protected]>
Signed-off-by: Jonathan Nitisastro <[email protected]>
8dce9ed
to
7649c27
Compare
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.
Can you add a test?
@@ -346,6 +346,19 @@ def foo(self): | |||
ray.get(actor.foo.remote()) | |||
|
|||
|
|||
def test_actor_name(ray_start_regular): |
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.
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)
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.
Gotcha! I merged it with the test_ids
Signed-off-by: Jonathan Nitisastro <[email protected]>
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.
Nice work!
Signed-off-by: Jonathan Nitisastro <[email protected]>
…_name Signed-off-by: Jonathan Nitisastro <[email protected]>
…into expose_actor_name Signed-off-by: Jonathan Nitisastro <[email protected]>
Signed-off-by: Jonathan Nitisastro <[email protected]>
python/ray/runtime_context.py
Outdated
|
||
Returns: | ||
The current actor name of this worker. | ||
Return empty string if there's no actor name or if it's a driver |
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.
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() |
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.
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 <[email protected]>
Signed-off-by: Jonathan Nitisastro <[email protected]>
…39347) Expose current actor's name through RuntimeContext() Signed-off-by: Jim Thompson <[email protected]>
…39347) Expose current actor's name through RuntimeContext() Signed-off-by: Simon Zehnder <[email protected]>
…39347) Expose current actor's name through RuntimeContext() Signed-off-by: Victor <[email protected]>
Why are these changes needed?
Expose current actor's name through
RuntimeContext()
Related issue number
Closes #35849
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.