[tune] nevergrad add points_to_evaluate #12207

viotemp1 · 2020-11-20T13:44:38Z

Why are these changes needed?

Add points_to_evaluate to NevergradSearch

Related issue number

Checks

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

krfricke · 2020-11-20T14:04:25Z

Great, that worked, thanks! I'll look into it shortly

krfricke

Thanks for this contribution!

Besides the minor comments it would be good if you could add a test (e.g. in test_sample.py). This test should instantiate a nevergrad searcher with a list of e.g. 3 points to evaluate and confirm that the first three sampled configurations are indeed these points.

Let me know if you need any help with that.

krfricke · 2020-11-20T14:05:25Z

python/ray/tune/suggest/nevergrad.py

+
+        current_best_params = [{
+            'width': 10,
+            'height': 0,
+            'activation': 0, # The index of "relu"
+        }]


Let's use our usual quotes here (" instead of ')

krfricke · 2020-11-20T14:12:00Z

python/ray/tune/suggest/nevergrad.py

+            point_to_evaluate = self._points_to_evaluate.pop(0)
+            for key in self._space.value:
+                if isinstance(self._space[key], ng.p.Choice):
+                    point_to_evaluate[key] = self._space[key].choices.value[point_to_evaluate[key]]


Question: Couldn't we pass e.g. activation: "relu" in the points_to_evaluate dict directly and omit this lookup?

That's for sure. I did like this in the beginning, but I change it to be compatible with points_to_evaluate from HyperOptSearch.

Let's do it with strings then (e.g. activation: "relu") and we'll change the hyperoptsearch implementation in a different PR.

krfricke · 2020-11-20T14:16:32Z

python/ray/tune/suggest/nevergrad.py

+        if points_to_evaluate is None:
+            self._points_to_evaluate = None
+        else:
+            assert isinstance(points_to_evaluate, (list, tuple))
+            self._points_to_evaluate = points_to_evaluate
+


Let's raise an actionable error message here.I'd suggest something like:

if not isinstance(points_to_evaluate, Sequence): raise ValueError( f"Invalid object type passed for `points_to_evaluate`: {type(points_to_evaluate)}. " f"Please pass a list of points (dictionaries) instead.") self._points_to_evaluate = list(points_to_evaluate)

(This would have to be linted)

viotemp1 · 2020-11-20T14:39:43Z

Should I add one more test in test_sample.py - something like testConvertNevergrad_best_params?

krfricke · 2020-11-20T14:42:40Z

Yep, let's call it testNevergradBestParams to follow the naming conventions. That's great, thanks!

…gorical values are not encoded

viotemp1 · 2020-11-20T18:13:57Z

added new test in test_sample.py

krfricke

Almost done - just one final one line change

krfricke · 2020-11-23T10:33:48Z

python/ray/tune/suggest/nevergrad.py

+                f"Invalid object type passed for `points_to_evaluate`: {type(points_to_evaluate)}. "
+                f"Please pass a list of points (dictionaries) instead.")
+        else:
+            self._points_to_evaluate = points_to_evaluate


We will either have to convert to a list here (i.e. self._points_to_evaluate = list(points_to_evaluate)) or check for MutableSequence above, as this implements the pop() interface we use in the code below.

krfricke · 2020-11-23T12:25:41Z

That's awesome! Thanks! We'll let the tests run and assuming they pass we should be ready to merge.

krfricke · 2020-11-23T14:48:56Z

There's a few linting errors:

python/ray/tune/suggest/nevergrad.py:133:80: E501 line too long (100 > 79 characters)

python/ray/tune/tests/test_sample.py:592:9: F841 local variable 'best_params0' is assigned to but never used

python/ray/tune/tests/test_sample.py:592:23: F841 local variable 'best_params1' is assigned to but never used

Could you fix them? (And run ./scripts/format.sh afterwards to confirm).

The other failing tests seem unrelated to this PR.

(wait for tests)

krfricke · 2020-11-23T19:07:17Z

Perfect! Thanks @viotemp1, that's a very useful contribution!

This should be ready to merge cc @richardliaw

richardliaw · 2020-11-23T20:51:10Z

thanks!

viotemp1 · 2020-11-24T07:40:41Z

Should I delete this branch? Sorry for the question ...
Also this suggestion should be implemented: "Let's do it with strings then (e.g. activation: "relu") and we'll change the hyperoptsearch implementation in a different PR"? I have seen that internally hyperoptsearch is using categorical for choices...
Thanks!

krfricke · 2020-11-24T08:29:43Z

Hi, you can delete the branch as the changes are included in the master now. So next time if you want to contribute something, you'd do the following steps:

(If you haven't already): git remote add upstream https://github.com/ray-project/ray.git
git checkout master (will switch to your local master branch)
git pull upstream master (will pull the most recent version from the official ray repository)
git push origin master (optional, will push the most recent version to your own fork)

You can then git checkout -b branch-name to checkout a new branch, develop, push to your own fork (git push origin branch-name) and file a new PR!

Yep, I'll file an issue to track the HyperOptSearch categorical feature! Thanks again!

viotemp1 · 2020-11-24T09:08:26Z

Thank you too!

Vio U added 2 commits November 20, 2020 15:37

Add points_to_evaluate to NevergradSearch

0512c4b

Add points_to_evaluate to NevergradSearch

a773edf

krfricke requested changes Nov 20, 2020

View reviewed changes

krfricke reviewed Nov 20, 2020

View reviewed changes

amogkam assigned krfricke Nov 20, 2020

Vio U added 3 commits November 20, 2020 19:25

add testing for testNevergradBestParams

dbbea89

change suggest to work with points_to_evaluate as list of dict - Cate…

fd41f7e

…gorical values are not encoded

changed Choice to work with value, not index in list

7c43b62

fix some formatting

b1fe85b

krfricke reviewed Nov 23, 2020

View reviewed changes

fix self._points_to_evaluate = list(points_to_evaluate)

9862b46

krfricke previously approved these changes Nov 23, 2020

View reviewed changes

fix formatting issues

c0dc2e5

krfricke approved these changes Nov 23, 2020

View reviewed changes

richardliaw merged commit 4c4f189 into ray-project:master Nov 23, 2020

krfricke mentioned this pull request Nov 24, 2020

[tune] HyperOpt points_to_evaluate should support actual category (not IDs) #12340

Closed

viotemp1 deleted the tune-nevergrad branch November 24, 2020 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tune] nevergrad add points_to_evaluate #12207

[tune] nevergrad add points_to_evaluate #12207

viotemp1 commented Nov 20, 2020 •

edited

krfricke commented Nov 20, 2020

krfricke left a comment

krfricke Nov 20, 2020

viotemp1 Nov 20, 2020

krfricke Nov 20, 2020

viotemp1 Nov 20, 2020

krfricke Nov 20, 2020

viotemp1 Nov 20, 2020

krfricke Nov 20, 2020

viotemp1 Nov 20, 2020

viotemp1 commented Nov 20, 2020

krfricke commented Nov 20, 2020

viotemp1 commented Nov 20, 2020

krfricke left a comment

krfricke Nov 23, 2020

viotemp1 Nov 23, 2020

krfricke commented Nov 23, 2020

krfricke commented Nov 23, 2020

krfricke commented Nov 23, 2020

richardliaw commented Nov 23, 2020

viotemp1 commented Nov 24, 2020

krfricke commented Nov 24, 2020

viotemp1 commented Nov 24, 2020

[tune] nevergrad add points_to_evaluate #12207

[tune] nevergrad add points_to_evaluate #12207

Conversation

viotemp1 commented Nov 20, 2020 • edited

Why are these changes needed?

Related issue number

Checks

krfricke commented Nov 20, 2020

krfricke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viotemp1 commented Nov 20, 2020

krfricke commented Nov 20, 2020

viotemp1 commented Nov 20, 2020

krfricke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krfricke commented Nov 23, 2020

krfricke commented Nov 23, 2020

krfricke commented Nov 23, 2020

richardliaw commented Nov 23, 2020

viotemp1 commented Nov 24, 2020

krfricke commented Nov 24, 2020

viotemp1 commented Nov 24, 2020

viotemp1 commented Nov 20, 2020 •

edited