Skip to content

Add label_selector option to remote functions #51707

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 44 commits into from
Apr 21, 2025
Merged

Add label_selector option to remote functions #51707

merged 44 commits into from
Apr 21, 2025

Conversation

ryanaoleary
Copy link
Contributor

@ryanaoleary ryanaoleary commented Mar 26, 2025 •
edited
Loading

Why are these changes needed?

Adds the label_selector option to Ray tasks/actors through _common_options, to specify labels used for scheduling on Ray nodes.

Related issue number

#51564

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

@ryanaoleary
Add label_selector to remote function
aa53a0f 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Add get_node_labels to runtime context
6b08fbe 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary ryanaoleary mentioned this pull request Mar 26, 2025
Open
23 tasks
@ryanaoleary
Remove runtime context change from this PR
2dcd849 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary ryanaoleary marked this pull request as ready for review March 26, 2025 10:32
@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Mar 26, 2025
@MengjinYan MengjinYan self-assigned this Mar 26, 2025
@MengjinYan MengjinYan self-requested a review March 26, 2025 20:45
MengjinYan
MengjinYan reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@MengjinYan MengjinYan 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 effort!

I have one general question: It seems that this PR only contains the logic to add the options. For the validation logic, are you planning to add it in a future PR?

ryanaoleary and others added 4 commits March 26, 2025 23:47
@ryanaoleary @MengjinYan
Update python/ray/remote_function.py
157c619 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary @MengjinYan
Update python/ray/actor.py
39e66ba 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary @MengjinYan
Update python/ray/actor.py
0250ac5 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary
Add label selector tests
0b09cd5 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Copy link
Contributor Author

Thanks for the effort!

I have one general question: It seems that this PR only contains the logic to add the options. For the validation logic, are you planning to add it in a future PR?

It already validates that label_selector is a valid remote function option here. For other Actor options, I don't see additional validation about what's passed in as an argument besides that check. Should we be validating here that the labels passed in follow the expected syntax (the same as we do for the node labels) and that the conditions are supported? Or is it sufficient just to allow the user to pass in the label_selector and only schedule it to the node if they are valid.

@ryanaoleary ryanaoleary requested a review from MengjinYan March 27, 2025 08:59
@MengjinYan
Copy link
Contributor

Thanks for the effort!
I have one general question: It seems that this PR only contains the logic to add the options. For the validation logic, are you planning to add it in a future PR?

It already validates that label_selector is a valid remote function option here. For other Actor options, I don't see additional validation about what's passed in as an argument besides that check. Should we be validating here that the labels passed in follow the expected syntax (the same as we do for the node labels) and that the conditions are supported? Or is it sufficient just to allow the user to pass in the label_selector and only schedule it to the node if they are valid.

I think we definitely shouldn't silently ignore the selectors if they are not valid. We need to give users explicitly warnings to users. Personally, I would prefer we check the syntax here and raise an error early so that the users can be aware of the invalid syntax and fix accordingly.

MengjinYan
MengjinYan reviewed Mar 27, 2025
View reviewed changes
ryanaoleary and others added 5 commits March 27, 2025 10:39
@ryanaoleary @MengjinYan
Update python/ray/actor.py
a4ddd85 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary @MengjinYan
Update python/ray/tests/test_actor.py
a41fe30 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary
Add tests and validate label selectors
4465d1b 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Make space optional in regex
b41dba6 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Add back in length check for consistency
2fa06a2 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary ryanaoleary requested a review from MengjinYan March 28, 2025 01:13
@ryanaoleary
Copy link
Contributor Author

[2fa06a2](/ray-project/ray/pull/51707/commits/2fa06a26515c1824e915c2485674c62674247bd8)

That makes sense to me, I updated the tests and added validation logic for label_selector in 4465d1b. I moved a lot of the validation logic from #51706 to this PR instead since we can re-use some of the same functions for validating the node labels.

MengjinYan
MengjinYan reviewed Mar 28, 2025
View reviewed changes
ryanaoleary and others added 3 commits March 28, 2025 16:30
@ryanaoleary @MengjinYan
Update python/ray/_private/utils.py
3958699 
Co-authored-by: Mengjin Yan <mengjinyan3@gmail.com>
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary
Add examples of valid and invalid label values
4878652 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Fix example
f0bd1db 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Fix tests
319c8be 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
directly raise value error
4e0c3b0 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Parametrize functions where appropriate
71c5681 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Add additional test cases
21eb99c 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Add length constraint
a109157 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Add label_selector to worker
8d89ccd 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Pass label_selector to create_actor
447edeb 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Don't pass label_selector to core worker yet
0389963 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@edoakes
Copy link
Collaborator

edoakes commented Apr 11, 2025

@ryanaoleary there are some merge conflicts due to the revert, pls fix one the other PR is re-merged

@ryanaoleary ryanaoleary mentioned this pull request Apr 15, 2025
Merged
8 tasks
ryanaoleary and others added 3 commits April 15, 2025 13:35
@ryanaoleary
Merge branch 'master' into add-label-selector-to-remote
0fe8c53 
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary
Fix bad merge
7d230a0 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
remove changes to to test_node_labels
e64bf39 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Copy link
Contributor Author

@ryanaoleary there are some merge conflicts due to the revert, pls fix one the other PR is re-merged

@edoakes I've fixed the merge conflicts now that the other PR is merged, I also updated the validation logic to retain the previous check based on the discussion from the other PR.

@edoakes
Copy link
Collaborator

edoakes commented Apr 15, 2025

@ryanaoleary there are some merge conflicts due to the revert, pls fix one the other PR is re-merged

@edoakes I've fixed the merge conflicts now that the other PR is merged, I also updated the validation logic to retain the previous check based on the discussion from the other PR.

There's another merge conflict 😓

@ryanaoleary
Merge branch 'master' into add-label-selector-to-remote
b62ef9e 
Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com>
@ryanaoleary
Copy link
Contributor Author

@ryanaoleary there are some merge conflicts due to the revert, pls fix one the other PR is re-merged

@edoakes I've fixed the merge conflicts now that the other PR is merged, I also updated the validation logic to retain the previous check based on the discussion from the other PR.

There's another merge conflict 😓

Ah lol should be fixed now

edoakes
edoakes approved these changes Apr 16, 2025
View reviewed changes
Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

Looks good, minor nit

ryanaoleary and others added 3 commits April 21, 2025 08:37
@ryanaoleary
Mark docstring as experimental
4b70316 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Remove now unneeded ray.io prefix tests and fix file test on windows
711237e 
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@ryanaoleary
Merge branch 'master' into add-label-selector-to-remote
382595e
This was referenced Apr 21, 2025
Closed
Merged
edoakes
edoakes reviewed Apr 21, 2025
View reviewed changes
python/ray/tests/test_node_labels.py
Comment on lines +158 to +160
finally:
subprocess.check_call(["ray", "stop", "--force"])
os.remove(test_file_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

will merge the PR as-is, but please follow up to move all such cleanup logic in this file into fixtures as best practice

@edoakes edoakes merged commit 3441318 into ray-project:master Apr 21, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edoakes edoakes edoakes approved these changes

@MengjinYan MengjinYan MengjinYan approved these changes

Assignees

@MengjinYan MengjinYan

Labels
community-backlog community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ryanaoleary @MengjinYan @edoakes @hainesmichaelc @jcotant1