MAINT improve code based on Andreas' suggestions

Author: mfeurer

openml/flows/__init__.py

@@ -1,6 +1,6 @@
 from .flow import OpenMLFlow
-from .functions import get_flow
 from .sklearn_converter import sklearn_to_flow, flow_to_sklearn
+from .functions import get_flow
 
 __all__ = ['OpenMLFlow', 'create_flow_from_model', 'get_flow',
            'sklearn_to_flow', 'flow_to_sklearn']

openml/flows/flow.py

@@ -145,17 +145,17 @@ def _to_xml(self):
         return flow_xml
 
     def _to_dict(self):
-        """ Helper function used by _to_xml and _to_dict.
+        """ Helper function used by _to_xml and itself.
 
         Creates a dictionary representation of self which can be serialized
         to xml by the function _to_xml. Since a flow can contain subflows
         (components) this helper function calls itself recursively to also
         serialize these flows to dictionaries.
 
-        Uses OrderedDict everywhere to make sure that the order of data stays
-        at it is added here. The return value (OrderedDict) will be used to
-        create the upload xml file. The xml file must have the tags in exactly
-        the order given in the xsd schema of a flow (see class docstring).
+        Uses OrderedDict to ensure consistent ordering when converting to xml.
+        The return value (OrderedDict) will be used to create the upload xml
+        file. The xml file must have the tags in exactly the order given in the
+        xsd schema of a flow (see class docstring).
 
         Returns
         -------
@@ -324,30 +324,6 @@ def _from_dict(cls, xml_dict):
         arguments['model'] = None
         return cls(**arguments)
 
-    def __eq__(self, other):
-        """Check equality.
-
-        Two flows are equal if their all keys which are not set by the server
-        are equal, as well as all their parameters and components.
-        """
-        if not isinstance(other, self.__class__):
-            return NotImplemented
-
-        # Name is actually not generated by the server, but it will be
-        # tested further down with a getter (allows mocking in the tests)
-        generated_by_the_server = ['name', 'flow_id', 'uploader', 'version',
-                                   'upload_date', 'source_url',
-                                   'binary_url', 'source_format',
-                                   'binary_format', 'source_md5',
-                                   'binary_md5', 'model']
-
-        for key in set(self.__dict__.keys()).union(other.__dict__.keys()):
-            if key in generated_by_the_server:
-                continue
-            if getattr(self, key, None) != getattr(other, key, None):
-                return False
-        return True
-
     def publish(self):
         """Publish flow to OpenML server.
 

openml/flows/functions.py

@@ -1,9 +1,7 @@
 import xmltodict
 
 from openml._api_calls import _perform_api_call
-# Absolute imports, to avoid circular dependencies
-from openml.flows.sklearn_converter import flow_to_sklearn
-from . import OpenMLFlow
+from . import OpenMLFlow, flow_to_sklearn
 
 
 def get_flow(flow_id):

openml/flows/sklearn_converter.py

@@ -1,4 +1,6 @@
-from collections import OrderedDict, defaultdict
+"""Convert scikit-learn estimators into an OpenMLFlows and vice versa."""
+
+from collections import OrderedDict
 import importlib
 import inspect
 import json
@@ -23,9 +25,6 @@
     JSONDecodeError = ValueError
 
 
-"""Convert scikit-learn estimators into an OpenMLFlows and vice versa."""
-
-
 def sklearn_to_flow(o):
 
     if _is_estimator(o):
@@ -52,7 +51,7 @@ def sklearn_to_flow(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 we want here as there shouldn't be any
+    # 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):
         rval = serialize_function(o)
@@ -126,7 +125,6 @@ def flow_to_sklearn(o, **kwargs):
         rval = _deserialize_model(o, **kwargs)
     else:
         raise TypeError(o)
-    assert o is None or rval is not None
 
     return rval
 
@@ -153,17 +151,7 @@ def _serialize_model(model):
 
     # Check that a component does not occur multiple times in a flow as this
     # is not supported by OpenML
-    to_visit_stack = []
-    to_visit_stack.extend(sub_components.values())
-    known_sub_components = set()
-    while len(to_visit_stack) > 0:
-        visitee = to_visit_stack.pop()
-        if visitee.name in known_sub_components:
-            raise ValueError('Found a second occurence of component %s when '
-                             'trying to serialize %s.' % (visitee.name, model))
-        else:
-            known_sub_components.add(visitee.name)
-            to_visit_stack.extend(visitee.components.values())
+    _check_multiple_occurence_of_component_in_flow(model, sub_components)
 
     # Create a flow name, which contains all components in brackets, for
     # example RandomizedSearchCV(Pipeline(StandardScaler,AdaBoostClassifier(DecisionTreeClassifier)),StandardScaler,AdaBoostClassifier(DecisionTreeClassifier))
@@ -184,22 +172,7 @@ def _serialize_model(model):
         name = class_name
 
     # Get the external versions of all sub-components
-    model_package_name = model.__module__.split('.')[0]
-    module = importlib.import_module(model_package_name)
-    model_package_version_number = module.__version__
-    external_version = _format_external_version(model_package_name, model_package_version_number)
-
-    external_versions = set()
-    external_versions.add(external_version)
-    to_visit_stack = []
-    to_visit_stack.extend(sub_components.values())
-    while len(to_visit_stack) > 0:
-        visitee = to_visit_stack.pop()
-        for external_version in visitee.external_version.split(','):
-            external_versions.add(external_version)
-        to_visit_stack.extend(visitee.components.values())
-    external_versions = list(sorted(external_versions))
-    external_version = ','.join(external_versions)
+    external_version = _get_external_version_string(model, sub_components)
 
     flow = OpenMLFlow(name=name,
                       class_name=class_name,
@@ -217,6 +190,41 @@ def _serialize_model(model):
     return flow
 
 
+def _get_external_version_string(model, sub_components):
+    # Create external version string for a flow, given the model and the
+    # already parsed dictionary of sub_components. Retrieves the external
+    # version of all subcomponents, which themselves already contain all
+    # requirements for their subcomponents. The external version string is a
+    # sorted concatenation of all modules which are present in this run.
+    model_package_name = model.__module__.split('.')[0]
+    module = importlib.import_module(model_package_name)
+    model_package_version_number = module.__version__
+    external_version = _format_external_version(model_package_name,
+                                                model_package_version_number)
+    external_versions = set()
+    external_versions.add(external_version)
+    for visitee in sub_components.values():
+        for external_version in visitee.external_version.split(','):
+            external_versions.add(external_version)
+    external_versions = list(sorted(external_versions))
+    external_version = ','.join(external_versions)
+    return external_version
+
+
+def _check_multiple_occurence_of_component_in_flow(model, sub_components):
+    to_visit_stack = []
+    to_visit_stack.extend(sub_components.values())
+    known_sub_components = set()
+    while len(to_visit_stack) > 0:
+        visitee = to_visit_stack.pop()
+        if visitee.name in known_sub_components:
+            raise ValueError('Found a second occurence of component %s when '
+                             'trying to serialize %s.' % (visitee.name, model))
+        else:
+            known_sub_components.add(visitee.name)
+            to_visit_stack.extend(visitee.components.values())
+
+
 def _extract_information_from_model(model):
     # This function contains four "global" states and is quite long and
     # complicated. If it gets to complicated to ensure it's correctness,
@@ -257,7 +265,7 @@ def _extract_information_from_model(model):
                     # Add the component to the list of components, add a
                     # component reference as a placeholder to the list of
                     # parameters, which will be replaced by the real component
-                    # when deserealizing the parameter
+                    # when deserializing the parameter
                     sub_component_identifier = k + '__' + identifier
                     sub_components_explicit.add(sub_component_identifier)
                     sub_components[sub_component_identifier] = sub_component

openml/testing.py

@@ -42,7 +42,7 @@ def setUp(self):
         self.cached = True
         # amueller's read/write key that he will throw away later
         openml.config.apikey = "610344db6388d9ba34f6db45a3cf71de"
-        self.production_server = "https://www.openml.org/api/v1/xml"
+        self.production_server = openml.config.server
         self.test_server = "https://test.openml.org/api/v1/xml"
         openml.config.server = self.test_server
 

openml/util.py

@@ -5,12 +5,12 @@
 else:
     from urllib.error import URLError
 
-import six
 
 def is_string(obj):
     try:
         return isinstance(obj, basestring)
     except NameError:
         return isinstance(obj, str)
 
+
 __all__ = ['URLError', 'is_string']

tests/flows/dummy_learn/__init__.py

@@ -1 +0,0 @@
-__version__ = 1.0

tests/flows/dummy_learn/dummy_forest.py

@@ -9,4 +9,4 @@ def get_params(self, deep=False):
         return {}
 
     def set_params(self, params):
-        return None
+        return self

tests/flows/test_flow.py

@@ -24,6 +24,38 @@
 from openml.flows.sklearn_converter import _format_external_version
 
 
+def are_flows_equal(flow1, flow2):
+    """Check equality of two flows.
+
+    Two flows are equal if their all keys which are not set by the server
+    are equal, as well as all their parameters and components.
+    """
+    if not isinstance(flow2, flow1.__class__):
+        return False
+
+    # Name is actually not generated by the server, but it will be
+    # tested further down with a getter (allows mocking in the tests)
+    generated_by_the_server = ['name', 'flow_id', 'uploader', 'version',
+                               'upload_date', 'source_url',
+                               'binary_url', 'source_format',
+                               'binary_format', 'source_md5',
+                               'binary_md5', 'model']
+
+    for key in set(flow1.__dict__.keys()).union(flow2.__dict__.keys()):
+        if key in generated_by_the_server:
+            continue
+        attr1 = getattr(flow1, key, None)
+        attr2 = getattr(flow2, key, None)
+        if key == 'components':
+            for name in set(attr1.keys()).union(attr2.keys()):
+                if not are_flows_equal(attr1[name], attr2[name]):
+                    return False
+        else:
+            if attr1 != attr2:
+                return False
+    return True
+
+
 def get_sentinel():
     # Create a unique prefix for the flow. Necessary because the flow is
     # identified by its name and external version online. Having a unique
@@ -110,7 +142,7 @@ def test_to_xml_from_xml(self):
         xml = flow._to_xml()
         xml_dict = xmltodict.parse(xml)
         new_flow = openml.flows.OpenMLFlow._from_dict(xml_dict)
-        self.assertEqual(new_flow, flow)
+        self.assertTrue(are_flows_equal(new_flow, flow))
         self.assertIsNot(new_flow, flow)
 
     def test_publish_flow(self):
@@ -213,7 +245,7 @@ def test_sklearn_to_upload_to_flow(self):
 
         self.assertEqual(server_xml, local_xml)
 
-        self.assertEqual(new_flow, flow)
+        self.assertTrue(are_flows_equal(new_flow, flow))
         self.assertIsNot(new_flow, flow)
 
         fixture_name = 'sklearn.model_selection._search.RandomizedSearchCV(' \