🏆 Live Soccer TV: ray project/ray/pull/

krfricke · 2023-03-07T02:46:33Z

Why are these changes needed?

In #32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in #30091.

This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials.

To prove that it works, we modify the fix from #32560 to not require a workaround anymore.

To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run.

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

Signed-off-by: Kai Fricke <kai@anyscale.com>

Signed-off-by: Kai Fricke <coding@kaifricke.com>

ericl · 2023-03-07T07:00:52Z

+    def _register_atexit(self):
+        if not self._atexit_handler_registered:
+            atexit.register(_unregister_all)
+            self._atexit_handler_registered = True


I guess we're assuming this is only ever used on the driver. Is that true?

That's a good point. rllib registers envs/trainers on import. So if a remote process triggers an import, this may be triggered. I've added a check to only register this handler on the driver, which should be safe.

Guarding this on the driver only sounds good to me.

ericl · 2023-03-07T07:03:15Z

I wonder if we should rethink this whole registry thing. Would it be possible to replace it with a named actor pinned to the head node, which would automatically get gc'ed with the job end?

Signed-off-by: Kai Fricke <kai@anyscale.com>

krfricke · 2023-03-07T18:21:03Z

We discuss some alternatives in this doc, including the detached actor.

My main concern at the moment is the fault tolerance, i.e. recreating the actor if it fails. My feeling (maybe wrongly so) is that the global kv store has better availability guarantees - is that accurate?

Since we have the registry already, I think this PR is very non-invasive to the current logic. If it's fine with you, I'd like to go ahead with the changes in the PR and track the detached actor refactor as a P2 improvement.

krfricke · 2023-03-07T18:22:20Z

+            return
+
+        if ray._private.worker.global_worker.mode != ray.SCRIPT_MODE:
+            # Only cleanup on the driver


I think it's acceptable to not care about Ray client here. The garbage we may collect should be small in any case, and we didn't do any cleanup previously at all.

That seems ok, though wouldn't this also work in client mode automatically, as the Tune driver is usually run on the driver in client mode too?

The default behavior is to wrap the whole tune run/tuner object in a remote task/actor, which would then be WORKER_MODE

ericl · 2023-03-07T21:07:19Z

+            return
+
+        if ray._private.worker.global_worker.mode != ray.SCRIPT_MODE:
+            # Only cleanup on the driver


That seems ok, though wouldn't this also work in client mode automatically, as the Tune driver is usually run on the driver in client mode too?

ericl · 2023-03-07T21:08:29Z

+        self._atexit_handler_registered = False
+
+    @property
+    def prefix(self):


Suggested change

def prefix(self):

def job_id_prefix(self):

For clarity.

I'll leave it as prefix as it can be specified in the registry init to something other than the job id (mostly for compatibility, we can remove this later though).

…n multi tenancy (ray-project#33095) In ray-project#32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in ray-project#30091. This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials. To prove that it works, we modify the fix from ray-project#32560 to not require a workaround anymore. To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

…n multi tenancy (ray-project#33095) In ray-project#32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in ray-project#30091. This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials. To prove that it works, we modify the fix from ray-project#32560 to not require a workaround anymore. To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…n multi tenancy (ray-project#33095) In ray-project#32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in ray-project#30091. This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials. To prove that it works, we modify the fix from ray-project#32560 to not require a workaround anymore. To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Kai Fricke <coding@kaifricke.com>

…n multi tenancy (ray-project#33095) In ray-project#32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in ray-project#30091. This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials. To prove that it works, we modify the fix from ray-project#32560 to not require a workaround anymore. To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: elliottower <elliot@elliottower.com>

…n multi tenancy (ray-project#33095) In ray-project#32560, we documented a workaround for the multi tenancy issues in Ray Tune, e.g. described in ray-project#30091. This PR fixes the root issue by prefixing the global registry with the core worker job ID, which is unique per driver process. This will avoid conflicts between parallel running tune trials. To prove that it works, we modify the fix from ray-project#32560 to not require a workaround anymore. To avoid cluttering the global key-value store with stale objects, we also de-register objects from the global KV store after finishing a Ray Tune run. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

Kai Fricke added 5 commits February 16, 2023 15:40

unregister

1c1c8ac

Signed-off-by: Kai Fricke <kai@anyscale.com>

Merge branch 'master' into tune/registry-conflict-job-id

e587981

Remove workaround

b330646

Signed-off-by: Kai Fricke <kai@anyscale.com>

Update docs

1f8436d

Signed-off-by: Kai Fricke <kai@anyscale.com>

Unregister all

830d848

Signed-off-by: Kai Fricke <kai@anyscale.com>

krfricke assigned justinvyu, matthewdeng, xwjiang2010, richardliaw and ericl Mar 7, 2023

atexit handler

ecb7ecb

Signed-off-by: Kai Fricke <coding@kaifricke.com>

krfricke marked this pull request as ready for review March 7, 2023 05:19

krfricke requested review from a team, Yard1, amogkam, matthewdeng, maxpumperla, richardliaw and xwjiang2010 as code owners March 7, 2023 05:19

ericl reviewed Mar 7, 2023

View reviewed changes

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 7, 2023

Only cleanup on driver

3780e8f

Signed-off-by: Kai Fricke <kai@anyscale.com>

krfricke removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 7, 2023

krfricke commented Mar 7, 2023

View reviewed changes

ericl approved these changes Mar 7, 2023

View reviewed changes

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 7, 2023

ericl reviewed Mar 7, 2023

View reviewed changes

krfricke merged commit 31a991f into ray-project:master Mar 7, 2023

krfricke deleted the tune/registry-conflict-job-id branch March 7, 2023 21:52

Navigation Menu

Uh oh!

[tune] Prefix global object registry with job ID to avoid conflicts in multi tenancy - #33095

[tune] Prefix global object registry with job ID to avoid conflicts in multi tenancy#33095
krfricke merged 7 commits into
ray-project:masterfrom
krfricke:tune/registry-conflict-job-id

krfricke commented Mar 7, 2023 •

edited

Loading

Uh oh!

ericl Mar 7, 2023

Uh oh!

krfricke Mar 7, 2023

Uh oh!

ericl Mar 7, 2023

Uh oh!

ericl commented Mar 7, 2023

Uh oh!

krfricke commented Mar 7, 2023 •

edited

Loading

Uh oh!

krfricke Mar 7, 2023

Uh oh!

ericl Mar 7, 2023

Uh oh!

krfricke Mar 7, 2023

Uh oh!

ericl Mar 7, 2023

Uh oh!

ericl Mar 7, 2023

Uh oh!

krfricke Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

krfricke commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericl commented Mar 7, 2023

Uh oh!

krfricke commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

krfricke commented Mar 7, 2023 •

edited

Loading

krfricke commented Mar 7, 2023 •

edited

Loading