[Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers #30895

scv119 · 2022-12-05T19:32:59Z

Why are these changes needed?

This function is deprecated. The only use of run_funcion_on_all_workers in ray core has been replaced by #31383
Delete it to make our worker prestart change simpler.
This PR depends on #31383 #31528

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

fishbone · 2022-12-05T21:25:55Z

python/ray/_private/import_thread.py

@@ -156,9 +156,6 @@ def _process_key(self, key):
            # for profiling).
            # with profiling.profile("register_remote_function"):
            (self.worker.function_actor_manager.fetch_and_register_remote_function(key))
-        elif key.startswith(b"FunctionsToRun:"):


Let's also delete import_thread.py. It's only used in run_function_on_all_workers

python/ray/_private/worker.py

rkooo567 · 2022-12-06T07:49:13Z

python/ray/_private/import_thread.py

@@ -156,9 +156,6 @@ def _process_key(self, key):
            # for profiling).
            # with profiling.profile("register_remote_function"):
            (self.worker.function_actor_manager.fetch_and_register_remote_function(key))
-        elif key.startswith(b"FunctionsToRun:"):
-            with profiling.profile("fetch_and_run_function"):


Does it mean we are losing this data? This is kind of important for the timeline feature.

pcmoritz · 2022-12-06T07:58:37Z

Very excited for this change, thank you so much for making it! This is code that is still around from a long time ago :)

scv119 · 2023-01-09T05:47:46Z

Will continue this after Ray 2.3.

clarng · 2023-02-02T17:51:54Z

python/ray/_private/worker.py

@@ -3054,7 +2959,7 @@ def remote(
            application-level errors should be retried up to max_retries times.
            This can be a boolean or a list of exceptions that should be retried.
        scheduling_strategy: Strategy about how to
-            schedule a remote function or actor. Possible values are
+            schedule a remote function or actou. Possible values are


python/ray/_private/worker.py

edoakes · 2023-02-02T17:56:43Z

🤩

scv119 · 2023-03-08T06:58:27Z

rebase

rkooo567

Hmm is it really a good idea to deprecate this without the proper alternative? I feel like we need sth like a callback API that's called when the worker starts?

scv119 · 2023-03-09T19:25:35Z

I feel like we need sth like a callback API that's called when the worker starts?

I thought that API called runtime env :)

rkooo567 · 2023-03-09T23:01:48Z

Hmm I actually have 2 suggestions in this case?

https://docs.ray.io/en/master/ray-core/handling-dependencies.html -> I saw this doc, and it is not clear to me where to look in order to perform the same action. Can you link to the sub-category that has the exact API to replace this?

Instead of completely removing the function, why don't we keep the function but just raise an exception (for the backward compatibility)?

rkooo567 · 2023-06-08T06:02:24Z

cc @RehanSD for the awareness.

rkooo567

Actually @pcmoritz instead of completely removing the API, why don't we raise an exception? Or do you think it is better just removing it? If so, we should make sure this is documented in 2.6 (do we have any place to track deprecation somewhere?)

rkooo567 · 2023-06-08T06:03:34Z

Also, I will make sure the replacement API will be available and documented by ray 2.6

pcmoritz · 2023-06-08T06:16:35Z

Given this was a private API and has been deprecated for a while, I think it is fine to just remove it. But it is a good idea to mention in the release notes the worker process init hook you are working on :)

pcmoritz · 2023-06-08T06:18:45Z

Looks like one more lint is failing. I'll also try to get the remaining code owner approvals :)

pcmoritz · 2023-06-08T15:50:11Z

There is another failure here: https://buildkite.com/ray-project/oss-ci-build-pr/builds/24959#01889a90-c54a-46c9-af89-0b62df111d16 :)

ericl

Stamping

scv119 · 2023-06-08T20:48:53Z

huh what a coincidence 369f68e#diff-41539696967c12463a48d082cec221167ea1b96ec702641db33479116c02297b

pcmoritz · 2023-06-08T21:01:13Z

Haha nice :)

…6402) #30895 upgraded the pinned version of modin, removing support for python <= 3.7. All the release tests run with py37 by default, so the cluster env was failing to build due to no compatible modin version. This PR runs these release tests with py38 instead, which aligns with the us moving to testing with py38 for all of CI. Signed-off-by: Justin Yu <justinvyu@anyscale.com>

@rkooo567

…ay-project#35179) I spent quite a bit of time debugging the test failure in ray-project#34393 (see also ray-project#35108) It turns out the PR slightly made the _do_importing race condition (first time call in the import thread) more likely to happen. There is already a plan / PR to get rid of it (ray-project#30895) but it is currently waiting for having a replacement mechanism that @rkooo567 is working on. I synced with @scv119 and for the time being, we are planning to skip the offending test on Windows and once we got rid of the import thread, we can re-activate it. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

…_on_all_workers (ray-project#30895) This function is deprecated. The only use of run_funcion_on_all_workers in ray core has been replaced by ray-project#31383 Delete it to make our worker prestart change simpler. This PR depends on ray-project#31383 ray-project#31528 Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

…y-project#36402) ray-project#30895 upgraded the pinned version of modin, removing support for python <= 3.7. All the release tests run with py37 by default, so the cluster env was failing to build due to no compatible modin version. This PR runs these release tests with py38 instead, which aligns with the us moving to testing with py38 for all of CI. Signed-off-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

add

86d3d97

scv119 changed the title ~~[Core] deprecate run_function_on_all_workers~~ [Core] delete run_function_on_all_workers Dec 5, 2022

scv119 assigned fishbone and rkooo567 Dec 5, 2022

add

e2d21d9

scv119 marked this pull request as ready for review December 5, 2022 19:38

add

efac370

fishbone reviewed Dec 5, 2022

View reviewed changes

scv119 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 5, 2022

rkooo567 reviewed Dec 6, 2022

View reviewed changes

scv119 mentioned this pull request Jan 4, 2023

[Core][deprecate run_on_all_workers 1/n] set worker's sys.path through JobConfig._py_driver_sys_path #31383

Merged

7 tasks

scv119 added the Ray 2.4 label Jan 9, 2023

scv119 changed the title ~~[Core] delete run_function_on_all_workers~~ [Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers Jan 9, 2023

scv119 force-pushed the deprecate-worker-init branch from efac370 to b003f8e Compare February 2, 2023 12:44

scv119 requested a review from a team February 2, 2023 12:45

clarng approved these changes Feb 2, 2023

View reviewed changes

scv119 force-pushed the deprecate-worker-init branch from b003f8e to 2294789 Compare February 3, 2023 18:21

scv119 added do-not-merge Do not merge this PR! and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Feb 3, 2023

scv119 force-pushed the deprecate-worker-init branch 2 times, most recently from b8b0554 to e3a7561 Compare February 6, 2023 18:09

scv119 force-pushed the deprecate-worker-init branch from e3a7561 to 3d2559c Compare March 8, 2023 06:58

scv119 removed the do-not-merge Do not merge this PR! label Mar 8, 2023

rkooo567 reviewed Mar 9, 2023

View reviewed changes

scv119 added 2 commits June 7, 2023 21:15

update

a7f6ffe

update

070d9ba

scv119 requested review from ericl, c21, amogkam, scottjlee, bveeramani and raulchen as code owners June 8, 2023 04:40

update

a9f732f

rkooo567 approved these changes Jun 8, 2023

View reviewed changes

scv119 added 2 commits June 8, 2023 01:42

update

3110e66

update

f7b8a61

ericl approved these changes Jun 8, 2023

View reviewed changes

scv119 merged commit 30e6b29 into ray-project:master Jun 8, 2023

krfricke mentioned this pull request Jun 13, 2023

[ci/docker] Move CI + Docker to Python 3.8 [build_base] #35040

Merged

8 tasks

This was referenced Jun 14, 2023

[release/air] Upgrade release tests that depend on upgraded modin to py38 #36402

Merged

Skip modin example for py37 ray-project/lightgbm_ray#47

Merged

jjyao mentioned this pull request Apr 15, 2024

[core] worker.run_function_on_all_workers runs in the driver process twice #14919

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers #30895

[Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers #30895

scv119 commented Dec 5, 2022 •

edited

Loading

fishbone Dec 5, 2022

rkooo567 Dec 6, 2022

pcmoritz commented Dec 6, 2022

scv119 commented Jan 9, 2023

clarng Feb 2, 2023

edoakes commented Feb 2, 2023

scv119 commented Mar 8, 2023

rkooo567 left a comment

scv119 commented Mar 9, 2023

rkooo567 commented Mar 9, 2023

rkooo567 commented Jun 8, 2023

rkooo567 left a comment

rkooo567 commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

ericl left a comment

scv119 commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

[Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers #30895

[Core][deprecate run_function_on_all_workers 3/n] delete run_function_on_all_workers #30895

Conversation

scv119 commented Dec 5, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

fishbone Dec 5, 2022

Choose a reason for hiding this comment

rkooo567 Dec 6, 2022

Choose a reason for hiding this comment

pcmoritz commented Dec 6, 2022

scv119 commented Jan 9, 2023

clarng Feb 2, 2023

Choose a reason for hiding this comment

edoakes commented Feb 2, 2023

scv119 commented Mar 8, 2023

rkooo567 left a comment

Choose a reason for hiding this comment

scv119 commented Mar 9, 2023

rkooo567 commented Mar 9, 2023

rkooo567 commented Jun 8, 2023

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

ericl left a comment

Choose a reason for hiding this comment

scv119 commented Jun 8, 2023

pcmoritz commented Jun 8, 2023

scv119 commented Dec 5, 2022 •

edited

Loading