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

krfricke · 2023-03-05T04:11:41Z

Why are these changes needed?

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests).

By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected.

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 <coding@kaifricke.com>

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

justinvyu

Looks good! I added a few questions/clarifications.

The eager_caching flag and keep_one logic seems a bit custom tailored to this specific Tune controller loop - is there a way to make it more general? One idea I had was to make this keep_at_least=1 instead. We don't need to over-generalize though, since I'm not too sure where we can use this _ObjectCache in other places in Tune. Any ideas there?

justinvyu · 2023-03-06T17:38:03Z

+
+            while len(objs) > max:
+                self._num_cached_objects -= 1
+                yield objs.pop(0)


Do we want to pop all first, then yield the results? Or is popping as we go intended behavior?

If we break/return in the middle of handling the result, some of the objects we tried to flush will still be in there.

I think that's what we want. The caller may have to run cleanup on the objects, and if we pop them first and fail, the objects are lost and we may end up with leaked objects.

krfricke · 2023-03-06T18:53:34Z

Yeah I agree that this is pretty specific to Tune's needs. The main two reasons to separate this out are 1) better unit testability (currently the reuse logic is happening fully within Trial Executor logic) and 2) reuse in the refactoring towards the Tune Controller (to avoid too much code duplication).

I'd propose we leave it as is (i.e. keep eager caching for exactly one unit) and if we actually reuse this in a different component with different requirements, we can change it then. Ok?

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

justinvyu

Makes sense, looks good to me!

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

…ct-cache-reuse

xwjiang2010

sorry just one more comment :)

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

krfricke · 2023-03-07T19:13:41Z

The failure in test_commands seems unrelated to this issue and comes up for me locally on master as well. It's due to extensive rllib imports. I'll merge this PR

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests). By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected. Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests). By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected. Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

Similar regression that was originally fixed in #31974, but re-surfaced after #33045. With actor re-use, we speculatively keep one cached actor around in case it is needed when new trials are added (e.g. if we add trials one-by-one). However, we should only do this when the search has not ended, as otherwise we keep an extra actor until the end of the experiment, wasting resources. This leads the tune_scale_up_down release test fail. In our refactor to #33045 we generalized object caching, but the eviction logic here explicitly keeps one object in cache. Our previous implementation in RayTrialExecutor relied on not calling the eviction function at all when trials were still coming up, essentially not adjusting the number of cached actors down. This is rarely a problem in practice, but does not make a clean contract when separated out into a component. In this PR, we change the logic as follows: When the search ended, no trials are pending execution, and we don't want to explicitly cache an actor, we force eviction of all cached objects. We are refactoring our execution backend, thus I believe it's sufficient to keep the release test to catch this regression. In the new backend we can add light weight unit tests to capture this behavior. Signed-off-by: Kai Fricke <kai@anyscale.com>

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests). By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected. Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Kai Fricke <kai@anyscale.com>

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests). By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected. Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: elliottower <elliot@elliottower.com>

…ject#33593) Similar regression that was originally fixed in ray-project#31974, but re-surfaced after ray-project#33045. With actor re-use, we speculatively keep one cached actor around in case it is needed when new trials are added (e.g. if we add trials one-by-one). However, we should only do this when the search has not ended, as otherwise we keep an extra actor until the end of the experiment, wasting resources. This leads the tune_scale_up_down release test fail. In our refactor to ray-project#33045 we generalized object caching, but the eviction logic here explicitly keeps one object in cache. Our previous implementation in RayTrialExecutor relied on not calling the eviction function at all when trials were still coming up, essentially not adjusting the number of cached actors down. This is rarely a problem in practice, but does not make a clean contract when separated out into a component. In this PR, we change the logic as follows: When the search ended, no trials are pending execution, and we don't want to explicitly cache an actor, we force eviction of all cached objects. We are refactoring our execution backend, thus I believe it's sufficient to keep the release test to catch this regression. In the new backend we can add light weight unit tests to capture this behavior. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: elliottower <elliot@elliottower.com>

Actor reuse is currently implemented within the RayTrialExecutor and tightly coupled to its concepts. To simplify the code, this PR introduces a new internal _ObjectCache class that generically caches objects (e.g. Actors+PGs) given a grouping key (e.g. resource requests). By moving the caching logic into a separate component, we can write better unit tests to ensure actor reuse works correctly and as expected. Signed-off-by: Kai Fricke <coding@kaifricke.com> Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

…ject#33593) Similar regression that was originally fixed in ray-project#31974, but re-surfaced after ray-project#33045. With actor re-use, we speculatively keep one cached actor around in case it is needed when new trials are added (e.g. if we add trials one-by-one). However, we should only do this when the search has not ended, as otherwise we keep an extra actor until the end of the experiment, wasting resources. This leads the tune_scale_up_down release test fail. In our refactor to ray-project#33045 we generalized object caching, but the eviction logic here explicitly keeps one object in cache. Our previous implementation in RayTrialExecutor relied on not calling the eviction function at all when trials were still coming up, essentially not adjusting the number of cached actors down. This is rarely a problem in practice, but does not make a clean contract when separated out into a component. In this PR, we change the logic as follows: When the search ended, no trials are pending execution, and we don't want to explicitly cache an actor, we force eviction of all cached objects. We are refactoring our execution backend, thus I believe it's sufficient to keep the release test to catch this regression. In the new backend we can add light weight unit tests to capture this behavior. Signed-off-by: Kai Fricke <kai@anyscale.com> Signed-off-by: Jack He <jackhe2345@gmail.com>

krfricke added 2 commits March 4, 2023 20:11

[tune] Use generic _ObjectCache for actor reuse

a58f9b3

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

Fix eager logic, add more tests

a1743d7

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

krfricke assigned justinvyu and xwjiang2010 Mar 5, 2023

krfricke requested review from justinvyu and xwjiang2010 March 5, 2023 05:52

krfricke and others added 3 commits March 5, 2023 14:33

another fix

84d6b51

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

Fix test

65f7b18

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

Merge branch 'master' into tune/generic-object-cache-reuse

f34ee30

justinvyu reviewed Mar 6, 2023

View reviewed changes

Kai Fricke added 2 commits March 6, 2023 11:19

Apply suggestions from review

99c04b2

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

Fix another test

c9552ed

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

justinvyu approved these changes Mar 6, 2023

View reviewed changes

xwjiang2010 reviewed Mar 6, 2023

View reviewed changes

Comment thread python/ray/tune/utils/object_cache.py Outdated

Kai Fricke added 2 commits March 6, 2023 16:11

Docstring

6fcc950

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

Merge remote-tracking branch 'upstream/master' into tune/generic-obje…

1b54c1d

…ct-cache-reuse

xwjiang2010 approved these changes Mar 7, 2023

View reviewed changes

Comment thread python/ray/tune/utils/object_cache.py Outdated

Rename

6bc2a8f

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

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

krfricke deleted the tune/generic-object-cache-reuse branch March 7, 2023 19:14

krfricke mentioned this pull request Mar 22, 2023

[tune] Evict object cache for actor re-use when search ended #33593

Merged

8 tasks

Navigation Menu

Uh oh!

[tune] Use generic _ObjectCache for actor reuse - #33045

[tune] Use generic _ObjectCache for actor reuse#33045
krfricke merged 10 commits into
ray-project:masterfrom
krfricke:tune/generic-object-cache-reuse

krfricke commented Mar 5, 2023

Uh oh!

justinvyu left a comment

Uh oh!

Uh oh!

Uh oh!

justinvyu Mar 6, 2023

Uh oh!

krfricke Mar 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krfricke commented Mar 6, 2023

Uh oh!

justinvyu left a comment

Uh oh!

Uh oh!

xwjiang2010 left a comment

Uh oh!

Uh oh!

krfricke commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

krfricke commented Mar 5, 2023

Why are these changes needed?

Related issue number

Checks

Uh oh!

justinvyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinvyu Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

krfricke Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krfricke commented Mar 6, 2023

Uh oh!

justinvyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xwjiang2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krfricke commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants