Merge pull request #222 from openml/issue209

Issue209

Author: janvanrijn

openml/flows/sklearn_converter.py

@@ -19,7 +19,8 @@
 # Necessary to have signature available in python 2.7
 from sklearn.utils.fixes import signature
 
-from .flow import OpenMLFlow
+from openml.flows import OpenMLFlow
+from openml.exceptions import PyOpenMLError
 
 
 if sys.version_info >= (3, 5):
@@ -32,37 +33,45 @@
     '^(?P<name>[\w\-]+)((?P<operation>==|>=|>)(?P<version>(\d+\.)?(\d+\.)?(\d+)))?$')
 
 
-def sklearn_to_flow(o):
+def sklearn_to_flow(o, parent_model=None):
+    # TODO: assert that only on first recursion lvl `parent_model` can be None
 
     if _is_estimator(o):
+        # is the main model or a submodel
         rval = _serialize_model(o)
     elif isinstance(o, (list, tuple)):
-        rval = [sklearn_to_flow(element) for element in o]
+        # TODO: explain what type of parameter is here
+        rval = [sklearn_to_flow(element, parent_model) for element in o]
         if isinstance(o, tuple):
             rval = tuple(rval)
     elif isinstance(o, (bool, int, float, six.string_types)) or o is None:
+        # base parameter values
         rval = o
     elif isinstance(o, dict):
+        # TODO: explain what type of parameter is here
         rval = OrderedDict()
         for key, value in o.items():
             if not isinstance(key, six.string_types):
                 raise TypeError('Can only use string as keys, you passed '
                                 'type %s for value %s.' %
                                 (type(key), str(key)))
-            key = sklearn_to_flow(key)
-            value = sklearn_to_flow(value)
+            key = sklearn_to_flow(key, parent_model)
+            value = sklearn_to_flow(value, parent_model)
             rval[key] = value
         rval = rval
     elif isinstance(o, type):
+        # TODO: explain what type of parameter is here
         rval = serialize_type(o)
     elif isinstance(o, scipy.stats.distributions.rv_frozen):
         rval = serialize_rv_frozen(o)
     # This only works for user-defined functions (and not even partial).
     # I think this is exactly what we want here as there shouldn't be any
     # built-in or functool.partials in a pipeline
     elif inspect.isfunction(o):
+        # TODO: explain what type of parameter is here
         rval = serialize_function(o)
     elif _is_cross_validator(o):
+        # TODO: explain what type of parameter is here
         rval = _serialize_cross_validator(o)
     else:
         raise TypeError(o, type(o))
@@ -256,18 +265,26 @@ def _extract_information_from_model(model):
 
     model_parameters = model.get_params(deep=False)
     for k, v in sorted(model_parameters.items(), key=lambda t: t[0]):
-        rval = sklearn_to_flow(v)
+        rval = sklearn_to_flow(v, model)
 
         if (isinstance(rval, (list, tuple)) and len(rval) > 0 and
                 isinstance(rval[0], (list, tuple)) and
                 [type(rval[0]) == type(rval[i]) for i in range(len(rval))]):
 
-            # Steps in a pipeline or feature union
+            # Steps in a pipeline or feature union, or base classifiers in voting classifier
             parameter_value = list()
+            reserved_keywords = set(model.get_params(deep=False).keys())
+
             for sub_component_tuple in rval:
                 identifier, sub_component = sub_component_tuple
                 sub_component_type = type(sub_component_tuple)
 
+                if identifier in reserved_keywords:
+                    parent_model_name = model.__module__ + "." + \
+                                        model.__class__.__name__
+                    raise PyOpenMLError('Found element shadowing official ' + \
+                                        'parameter for %s: %s' % (parent_model_name, identifier))
+
                 if sub_component is None:
                     # In a FeatureUnion it is legal to have a None step
 
@@ -310,7 +327,7 @@ def _extract_information_from_model(model):
             component_reference[
                 'oml-python:serialized_object'] = 'component_reference'
             component_reference['value'] = OrderedDict(key=k, step_name=None)
-            component_reference = sklearn_to_flow(component_reference)
+            component_reference = sklearn_to_flow(component_reference, model)
             parameters[k] = json.dumps(component_reference)
 
         else:

openml/runs/run.py

@@ -231,6 +231,9 @@ def get_flow_dict(_flow):
                 elif flow.name.startswith("sklearn.pipeline.FeatureUnion"):
                     # tolerate
                     pass
+                elif flow.name.startswith("sklearn.ensemble.voting_classifier.VotingClassifier"):
+                    # tolerate
+                    pass
                 else:
                     raise ValueError("parameter %s not in flow description of flow %s" %(param,flow.name))
 

tests/test_flows/test_sklearn.py

@@ -24,9 +24,9 @@
 import sklearn.preprocessing
 import sklearn.tree
 
-import openml
 from openml.flows import OpenMLFlow, sklearn_to_flow, flow_to_sklearn
 from openml.flows.sklearn_converter import _format_external_version, _check_dependencies
+from openml.exceptions import PyOpenMLError
 
 this_directory = os.path.dirname(os.path.abspath(__file__))
 sys.path.append(this_directory)
@@ -524,3 +524,34 @@ def test_check_dependencies(self):
         dependencies = ['sklearn==0.1', 'sklearn>=99.99.99', 'sklearn>99.99.99']
         for dependency in dependencies:
             self.assertRaises(ValueError, _check_dependencies, dependency)
+
+    def test_illegal_parameter_names(self):
+        # illegal name: estimators
+        clf1 = sklearn.ensemble.VotingClassifier(
+            estimators=[('estimators', sklearn.ensemble.RandomForestClassifier()),
+                        ('whatevs', sklearn.ensemble.ExtraTreesClassifier())])
+        clf2 = sklearn.ensemble.VotingClassifier(
+            estimators=[('whatevs', sklearn.ensemble.RandomForestClassifier()),
+                        ('estimators', sklearn.ensemble.ExtraTreesClassifier())])
+        cases = [clf1, clf2]
+
+        for case in cases:
+            self.assertRaises(PyOpenMLError, sklearn_to_flow, case)
+
+    def test_illegal_parameter_names_pipeline(self):
+        # illegal name: steps
+        steps = [
+            ('Imputer', sklearn.preprocessing.Imputer(strategy='median')),
+            ('OneHotEncoder', sklearn.preprocessing.OneHotEncoder(sparse=False, handle_unknown='ignore')),
+            ('steps', sklearn.ensemble.BaggingClassifier(base_estimator=sklearn.tree.DecisionTreeClassifier))
+        ]
+        self.assertRaises(ValueError, sklearn.pipeline.Pipeline, steps=steps)
+
+
+    def test_illegal_parameter_names_featureunion(self):
+        # illegal name: transformer_list
+        transformer_list = [
+            ('transformer_list', sklearn.preprocessing.Imputer(strategy='median')),
+            ('OneHotEncoder', sklearn.preprocessing.OneHotEncoder(sparse=False, handle_unknown='ignore'))
+        ]
+        self.assertRaises(ValueError, sklearn.pipeline.FeatureUnion, transformer_list=transformer_list)

tests/test_runs/test_run_functions.py

@@ -205,7 +205,7 @@ def test_get_runs_list_by_uploader(self):
         uploader_ids = [29]
 
         runs = openml.runs.list_runs(uploader=uploader_ids)
-        self.assertGreaterEqual(len(runs), 3)
+        self.assertGreaterEqual(len(runs), 2)
         for rid in runs:
             self.assertIn(runs[rid]['uploader'], uploader_ids)
             self._check_run(runs[rid])