[tune] Tweak/allow nested pbt mutations #3455

hartikainen · 2018-12-02T06:25:26Z

What do these changes do?

Changes the population-based training exploration to allow nested mutations

Related issue number

AmplabJenkins · 2018-12-02T09:29:06Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9697/
Test FAILed.

ericl · 2018-12-04T05:42:22Z

python/ray/tune/schedulers/pbt.py

+            new_config.update({
+                key: explore(
+                    config[key], mutations[key],
+                    resample_probability, custom_explore_fn)


Should we pass a no-op custom_explore_fn here? It might be weird to be invoking that at multiple levels.

Makes sense not to invoke the custom preturbation function at every level. But instead of passing a no-op, I believe we should pass the custom_explore_fn as is but only invoke it at the last level? What do you think?

ericl · 2018-12-04T06:15:56Z

Do you mean the top-most level? Passing as no-op to any nested explore calls would achieve this I think.

…

On Mon, Dec 3, 2018, 10:11 PM Kristian Hartikainen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In python/ray/tune/schedulers/pbt.py <#3455 (comment)>: > @@ -47,7 +47,13 @@ def explore(config, mutations, resample_probability, custom_explore_fn): """ new_config = copy.deepcopy(config) for key, distribution in mutations.items(): - if isinstance(distribution, list): + if isinstance(distribution, dict): + new_config.update({ + key: explore( + config[key], mutations[key], + resample_probability, custom_explore_fn) Makes sense not to invoke the custom preturbation function at every level. But instead of passing a no-op, I believe we should pass the custom_explore_fn as is but only invoke it at the last level? What do you think? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3455 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA6SshcWxzIfj-_n79DMtVnCRgJtAaNks5u1hH6gaJpZM4Y9Q2o> .

hartikainen · 2018-12-04T07:19:56Z

I did actually mean the bottom-most level, but I do see what you mean. I was thinking that the custom function would only be applied to the actual values to be explored. What you mean makes more sense.

…

On Mon, Dec 3, 2018, 10:16 PM Eric Liang ***@***.***> wrote: Do you mean the top-most level? Passing as no-op to any nested explore calls would achieve this I think. On Mon, Dec 3, 2018, 10:11 PM Kristian Hartikainen < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In python/ray/tune/schedulers/pbt.py > <#3455 (comment)>: > > > @@ -47,7 +47,13 @@ def explore(config, mutations, resample_probability, custom_explore_fn): > """ > new_config = copy.deepcopy(config) > for key, distribution in mutations.items(): > - if isinstance(distribution, list): > + if isinstance(distribution, dict): > + new_config.update({ > + key: explore( > + config[key], mutations[key], > + resample_probability, custom_explore_fn) > > Makes sense not to invoke the custom preturbation function at every level. > But instead of passing a no-op, I believe we should pass the > custom_explore_fn as is but only invoke it at the last level? What do you > think? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#3455 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AAA6SshcWxzIfj-_n79DMtVnCRgJtAaNks5u1hH6gaJpZM4Y9Q2o > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3455 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACM5v1NSgAZnkofExVebQVxr1qtWDoVBks5u1hMogaJpZM4Y9Q2o> .

hartikainen · 2018-12-06T02:13:55Z

Changed. I still need to test this in practice.

AmplabJenkins · 2018-12-06T03:36:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9789/
Test FAILed.

ericl

LGTM!

richardliaw · 2018-12-06T23:56:07Z

Hey @hartikainen, just ping us when you get the chance to try this out and remove the wip, so we can merge it.

ericl · 2018-12-13T05:02:56Z

Looks like a py2 failure:


=================================== FAILURES ===================================
______________ PopulationBasedTestingSuite.testPerturbationValues ______________
self = <trial_scheduler_test.PopulationBasedTestingSuite testMethod=testPerturbationValues>
    def testPerturbationValues(self):
        def assertProduces(fn, values):
            random.seed(0)
            seen = set()
            for _ in range(100):
                seen.add(fn()["v"])
            self.assertEqual(seen, values)
    
        # Categorical case
        assertProduces(
            lambda: explore({"v": 4}, {"v": [3, 4, 8, 10]}, 0.0, lambda x: x),
            {3, 8})
        assertProduces(
            lambda: explore({"v": 3}, {"v": [3, 4, 8, 10]}, 0.0, lambda x: x),
            {3, 4})
        assertProduces(
            lambda: explore({"v": 10}, {"v": [3, 4, 8, 10]}, 0.0, lambda x: x),
            {8, 10})
        assertProduces(
            lambda: explore({"v": 7}, {"v": [3, 4, 8, 10]}, 0.0, lambda x: x),
            {3, 4, 8, 10})
        assertProduces(
            lambda: explore({"v": 4}, {"v": [3, 4, 8, 10]}, 1.0, lambda x: x),
            {3, 4, 8, 10})
    
        # Continuous case
        assertProduces(
            lambda: explore(
                {"v": 100}, {"v": lambda: random.choice([10, 100])}, 0.0,
                lambda x: x),
            {80, 120})
        assertProduces(
            lambda: explore(
                {"v": 100.0}, {"v": lambda: random.choice([10, 100])}, 0.0,
                lambda x: x),
            {80.0, 120.0})
        assertProduces(
            lambda: explore(
                {"v": 100.0}, {"v": lambda: random.choice([10, 100])}, 1.0,
                lambda x: x),
            {10.0, 100.0})
    
        def deep_add(seen, new_values):
            for k, new_value in new_values.items():
                if isinstance(new_value, dict):
                    if k not in seen:
                        seen[k] = {}
                    seen[k].update(deep_add(seen[k], new_value))
                else:
                    if k not in seen:
                        seen[k] = set()
                    seen[k].add(new_value)
    
            return seen
    
        def assertNestedProduces(fn, values):
            random.seed(0)
            seen = {}
            for _ in range(100):
                new_config = fn()
                seen = deep_add(seen, new_config)
            self.assertEqual(seen, values)
    
        # Nested mutation and spec
        assertNestedProduces(
            lambda: explore(
                {
                    "a": {
                        "b": 4
                    },
                    "1": {
                        "2": {
                            "3": 100
                        }
                    },
                },
                {
                    "a": {
                        "b": [3, 4, 8, 10]
                    },
                    "1": {
                        "2": {
                            "3": lambda: random.choice([10, 100])
                        }
                    },
                },
                0.0,
                lambda x: x),
            {
                "a": {
                    "b": {3, 8}
                },
                "1": {
                    "2": {
                        "3": {80, 120}
                    }
                },
            })
    
>       custom_explore_fn = unittest.mock.MagicMock(side_effect=lambda x: x)
E       AttributeError: 'module' object has no attribute 'mock'
python/ray/tune/test/trial_scheduler_test.py:806: AttributeError

AmplabJenkins · 2018-12-20T07:09:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10193/
Test FAILed.

AmplabJenkins · 2018-12-20T08:00:53Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10194/
Test FAILed.

AmplabJenkins · 2018-12-22T02:20:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10277/
Test FAILed.

AmplabJenkins · 2018-12-22T02:22:15Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10278/
Test FAILed.

hartikainen · 2018-12-22T06:25:51Z

Sorry for the delay with this one. I tested this on my end, and the nesting works fine!

hartikainen · 2018-12-22T06:26:42Z

@richardliaw thanks for updating the test mocks!

richardliaw · 2019-01-04T21:51:00Z

Oops, this fell off the radar.

No problem; thanks for the contribution!

ericl reviewed Dec 4, 2018

View reviewed changes

ericl approved these changes Dec 6, 2018

View reviewed changes

richardliaw changed the title ~~[WIP] Tweak/allow nested pbt mutations~~ [tune] Tweak/allow nested pbt mutations Dec 20, 2018

hartikainen force-pushed the tweak/allow-nested-pbt-mutations branch 2 times, most recently from 93f491b to c8ab02a Compare December 22, 2018 00:12

hartikainen and others added 5 commits December 21, 2018 16:13

Fix warning text in pbt logger

bd7d9d6

Allow nested mutations in pbt by recursing explore function

19f0b13

Add test for nested pbt mutation

4243b56

Update pbt explore to only call custom explore on top level

4d22e95

fix test

5eef7eb

hartikainen force-pushed the tweak/allow-nested-pbt-mutations branch from c8ab02a to 5eef7eb Compare December 22, 2018 00:13

richardliaw merged commit 747b117 into ray-project:master Jan 4, 2019

hartikainen deleted the tweak/allow-nested-pbt-mutations branch January 4, 2019 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tune] Tweak/allow nested pbt mutations #3455

[tune] Tweak/allow nested pbt mutations #3455

hartikainen commented Dec 2, 2018 •

edited

Loading

AmplabJenkins commented Dec 2, 2018

ericl Dec 4, 2018

hartikainen Dec 4, 2018

ericl commented Dec 4, 2018 via email

hartikainen commented Dec 4, 2018 via email

hartikainen commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

ericl left a comment

richardliaw commented Dec 6, 2018

ericl commented Dec 13, 2018

AmplabJenkins commented Dec 20, 2018

AmplabJenkins commented Dec 20, 2018

AmplabJenkins commented Dec 22, 2018

AmplabJenkins commented Dec 22, 2018

hartikainen commented Dec 22, 2018

hartikainen commented Dec 22, 2018

richardliaw commented Jan 4, 2019

[tune] Tweak/allow nested pbt mutations #3455

[tune] Tweak/allow nested pbt mutations #3455

Conversation

hartikainen commented Dec 2, 2018 • edited Loading

What do these changes do?

Related issue number

AmplabJenkins commented Dec 2, 2018

ericl Dec 4, 2018

Choose a reason for hiding this comment

hartikainen Dec 4, 2018

Choose a reason for hiding this comment

ericl commented Dec 4, 2018 via email

hartikainen commented Dec 4, 2018 via email

hartikainen commented Dec 6, 2018

AmplabJenkins commented Dec 6, 2018

ericl left a comment

Choose a reason for hiding this comment

richardliaw commented Dec 6, 2018

ericl commented Dec 13, 2018

AmplabJenkins commented Dec 20, 2018

AmplabJenkins commented Dec 20, 2018

AmplabJenkins commented Dec 22, 2018

AmplabJenkins commented Dec 22, 2018

hartikainen commented Dec 22, 2018

hartikainen commented Dec 22, 2018

richardliaw commented Jan 4, 2019

hartikainen commented Dec 2, 2018 •

edited

Loading