Hide the interface so that all samplers can use `HyperbandPruner` by HideakiImamura · Pull Request #1168 · optuna/optuna

HideakiImamura · 2020-04-26T10:29:21Z

Motivation

The current Hyperband implementation in Optuna imposes users who want to implement a new sampler to implement the specific logic. This annoys us because it is exactly the same logic for all samplers. This PR aims to hide the interface so that all samplers can use HyperbandPruner without any additional efforts.

Description of the changes

Remove the specific logic for Hyperband in samplers/tpe/sampler.py.
Add sampler selection logic to study.py when the pruner is HyperbandPruner.
Add special sampler _HyperbandSampler to pruners/hyperband.py.

Note

This PR opposite PR #1196. If PR #1196 is merged, this PR should not be merged, and vice versa.

TODO

Implementation
Benchmark experiments to verify the no performance difference for RandomSampler and TPESampler.

Benchmark results

(CPU: Intel Core i7-7820HQ, python: 3.6.5)

Using kurobako
Compare the previous Hyperband and the new Hyperband
2 types of samplers: RandomSampler and TPESampler
5 types of benchmark dataset: HPO-bench-Naval, HPO-bench-Parkinson, HPO-bench-Protein, HPO-bench-Slice, and NASBench (A).

`RandomSampler`

`TPESampler`

…ler-logic2

codecov-io · 2020-04-26T10:55:16Z

Codecov Report

Merging #1168 into master will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
- Coverage   91.02%   91.00%   -0.02%     
==========================================
  Files         142      142              
  Lines       12276    12300      +24     
==========================================
+ Hits        11174    11194      +20     
- Misses       1102     1106       +4

Impacted Files	Coverage Δ
optuna/samplers/tpe/sampler.py	`88.11% <ø> (-0.20%)`	⬇️
optuna/study.py	`92.63% <66.66%> (-0.28%)`	⬇️
optuna/pruners/hyperband.py	`87.23% <88.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3573c8d...4d09112. Read the comment docs.

optuna/pruners/hyperband.py

hvy · 2020-04-28T03:10:26Z

optuna/study.py

        self.sampler = sampler or samplers.TPESampler()
        self.pruner = pruner or pruners.MedianPruner()
+        if isinstance(self.pruner, pruners.HyperbandPruner):
+            self.sampler = pruners.hyperband._HyperbandSampler(self.sampler, self.pruner)


How about using custom getters and setters for self.sampler (and internally store self._sampler = _HyperbandSampler)? Study.sampler is a part of the public interface after all.

Should we test this in e.g. test_hyperband.py?

I implemented the custom getter and setter of _HyperbandSampler in hyperband.py. I think it addresses the suggestion as pointed out.

Hm, I'm not so sure about that. study.sampler here return an instance of _HyperbandSampler. I was imagining having study._sampler being the _HyperbandSampler while study.sampler being a custom, in this case, accessor that return study._sampler.sampler which is the original sampler object given by the user. Similarly for the setter.

Note that we must also update Trial._suggest and Trial._init_relative_params.

hvy

Thanks for the PR, I left some early comments if you could take a look.

hvy · 2020-04-28T03:12:26Z

optuna/study.py

        self.sampler = sampler or samplers.TPESampler()
        self.pruner = pruner or pruners.MedianPruner()
+        if isinstance(self.pruner, pruners.HyperbandPruner):
+            self.sampler = pruners.hyperband._HyperbandSampler(self.sampler, self.pruner)


Should we test this in e.g. test_hyperband.py?

hvy · 2020-04-28T03:17:31Z

optuna/pruners/hyperband.py

+
+    @sampler.setter
+    def sampler(self, new_sampler: BaseSampler) -> None:
+        self._sampler = new_sampler


I believe we don't need custom getters/setters here as there's no logic involved.

hvy · 2020-04-28T06:52:52Z

optuna/study.py

        self.sampler = sampler or samplers.TPESampler()
        self.pruner = pruner or pruners.MedianPruner()
+        if isinstance(self.pruner, pruners.HyperbandPruner):
+            self.sampler = pruners.hyperband._HyperbandSampler(self.sampler, self.pruner)


Hm, I'm not so sure about that. study.sampler here return an instance of _HyperbandSampler. I was imagining having study._sampler being the _HyperbandSampler while study.sampler being a custom, in this case, accessor that return study._sampler.sampler which is the original sampler object given by the user. Similarly for the setter.

hvy · 2020-04-28T06:54:54Z

optuna/pruners/hyperband.py

        return _BracketStudy(study, bracket_index)
+
+
+class _HyperbandSampler(BaseSampler):


How about _BracketSampler? I think it better describes its logic/better aligned with _BracketStudy.

I think the name of _BracketSampler is not good because the _HyperbandSampler depends on the underlying HyperbandPruner but _BracketStudy depends on each bracket of HyperbandPruner. Therefore, I think BracketStudy and _HyperbandSampler are appropriate as they are now.

HideakiImamura · 2020-04-28T10:48:42Z

@hvy Thank you for your insightful comments. I would like to talk about how study.sampler looks like from users.

If we implement custom getter/setter for study.sampler, then users need not know about _HyperbandSampler not at all. It is a good news for users. However, in this case, the developers of optuna must know about the difference between study.sampler and study._sampler. We have to replace all access to study.sampler in the current code base with study._sampler. All developers have to remember the difference over the future. I think it is not good news for developers (ours).

On the other hand, if we do not implement a custom getter, then users need know about _HyperbandSampler but any other code base never change. The changes does not effect outside the functionality of Hyperband and its users.

Which do you prefer in light of your experience?

HideakiImamura · 2020-05-13T07:25:33Z

I decided to use the alternative implementation (#1196 ) to achieve this functionality, so I close this PR.

HideakiImamura added 6 commits April 23, 2020 12:26

Remove logics in samplers

bae533e

Add _HyperbandSampler and some logic to study.py.

e1bfbfb

Add getter and setter

7722510

Merge master into refactor/hyperband/sampler-logic2

d06c93c

Merge branch 'refactor/hyperband/budget' into refactor/hyperband/samp…

01e005b

…ler-logic2

Merge branch 'refactor/hyperband/budget' into refactor/hyperband/samp…

ac6a7a1

…ler-logic2

hvy added the feature Change that does not break compatibility, but affects the public interfaces. label Apr 27, 2020

HideakiImamura mentioned this pull request Apr 27, 2020

Fix the default value of max_resource to HyperbandPruner #1171

Merged

HideakiImamura changed the title ~~[WIP] Hide the interface so that all samplers can use HyperbandPruner~~ Hide the interface so that all samplers can use HyperbandPruner Apr 27, 2020

hvy self-assigned this Apr 27, 2020

toshihikoyanase reviewed Apr 28, 2020

View reviewed changes

optuna/pruners/hyperband.py Show resolved Hide resolved

HideakiImamura added 2 commits April 28, 2020 11:59

Add reseed method

d0afb70

Merge branch 'master' into refactor/hyperband/sampler-logic2

4d09112

hvy reviewed Apr 28, 2020

View reviewed changes

hvy requested changes Apr 28, 2020

View reviewed changes

hvy added the optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions. label Apr 28, 2020

Fix getter/setter of study.sampler

ec8317b

HideakiImamura added a commit to HideakiImamura/optuna that referenced this pull request Apr 30, 2020

Remove dependency @proptuna#1168

89f6663

HideakiImamura mentioned this pull request May 1, 2020

Alternative implementation to hide the interface so that all samplers can use HyperbandPruner. #1196

Merged

HideakiImamura closed this May 13, 2020

HideakiImamura deleted the refactor/hyperband/sampler-logic2 branch May 18, 2021 04:38

		return _BracketStudy(study, bracket_index)


		class _HyperbandSampler(BaseSampler):

Uh oh!

Conversation

HideakiImamura commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description of the changes

Note

TODO

Benchmark results

RandomSampler

TPESampler

Uh oh!

codecov-io commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvy Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HideakiImamura commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HideakiImamura commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HideakiImamura commented Apr 26, 2020 •

edited

Loading

`RandomSampler`

`TPESampler`

codecov-io commented Apr 26, 2020 •

edited

Loading

hvy Apr 28, 2020 •

edited

Loading

HideakiImamura commented Apr 28, 2020 •

edited

Loading