From 49234eabe5c1f658dcc1937c49a8ec5d1876bbd1 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Mon, 22 Jul 2019 15:02:09 -0400 Subject: [PATCH 1/8] raise a warning, not an error, when not matching version exactly --- openml/extensions/sklearn/extension.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openml/extensions/sklearn/extension.py b/openml/extensions/sklearn/extension.py index ce8e4ebf9..34e47c982 100644 --- a/openml/extensions/sklearn/extension.py +++ b/openml/extensions/sklearn/extension.py @@ -750,8 +750,8 @@ def _check_dependencies(self, dependencies: str) -> None: raise NotImplementedError( 'operation \'%s\' is not supported' % operation) if not check: - raise ValueError('Trying to deserialize a model with dependency ' - '%s not satisfied.' % dependency_string) + warnings.warn('Trying to deserialize a model with dependency ' + '%s not satisfied.' % dependency_string) def _serialize_type(self, o: Any) -> 'OrderedDict[str, str]': mapping = {float: 'float', From c12f4698ca40bc34d2978ad1061f0863284ab838 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Wed, 24 Jul 2019 13:27:05 -0400 Subject: [PATCH 2/8] add strict_version flag to get_flow --- openml/extensions/sklearn/extension.py | 31 ++++++++++++++++++++----- openml/flows/functions.py | 9 +++++-- tests/test_flows/test_flow_functions.py | 2 ++ 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/openml/extensions/sklearn/extension.py b/openml/extensions/sklearn/extension.py index 34e47c982..cd1826089 100644 --- a/openml/extensions/sklearn/extension.py +++ b/openml/extensions/sklearn/extension.py @@ -90,7 +90,9 @@ def can_handle_model(cls, model: Any) -> bool: ################################################################################################ # Methods for flow serialization and de-serialization - def flow_to_model(self, flow: 'OpenMLFlow', initialize_with_defaults: bool = False) -> Any: + def flow_to_model(self, flow: 'OpenMLFlow', + initialize_with_defaults: bool = False, + strict_version: Optional[bool] = True) -> Any: """Initializes a sklearn model based on a flow. Parameters @@ -103,11 +105,16 @@ def flow_to_model(self, flow: 'OpenMLFlow', initialize_with_defaults: bool = Fal If this flag is set, the hyperparameter values of flows will be ignored and a flow with its defaults is returned. + strict_version : bool, default=True + Whether to fail if version requirements are not fulfilled. + Returns ------- mixed """ - return self._deserialize_sklearn(flow, initialize_with_defaults=initialize_with_defaults) + return self._deserialize_sklearn( + flow, initialize_with_defaults=initialize_with_defaults, + strict_version=strict_version) def _deserialize_sklearn( self, @@ -115,6 +122,7 @@ def _deserialize_sklearn( components: Optional[Dict] = None, initialize_with_defaults: bool = False, recursion_depth: int = 0, + strict_version: Optional[bool] = True ) -> Any: """Recursive function to deserialize a scikit-learn flow. @@ -138,6 +146,9 @@ def _deserialize_sklearn( The depth at which this flow is called, mostly for debugging purposes + strict_version : bool, default=True + Whether to fail if version requirements are not fulfilled. + Returns ------- mixed @@ -238,6 +249,7 @@ def _deserialize_sklearn( flow=o, keep_defaults=initialize_with_defaults, recursion_depth=recursion_depth, + strict_version=strict_version ) else: raise TypeError(o) @@ -657,10 +669,12 @@ def _deserialize_model( flow: OpenMLFlow, keep_defaults: bool, recursion_depth: int, + strict_version: Optional[bool] = True ) -> Any: logging.info('-%s deserialize %s' % ('-' * recursion_depth, flow.name)) model_name = flow.class_name - self._check_dependencies(flow.dependencies) + self._check_dependencies(flow.dependencies, + strict_version=strict_version) parameters = flow.parameters components = flow.components @@ -721,7 +735,8 @@ def _deserialize_model( del parameter_dict[param] return model_class(**parameter_dict) - def _check_dependencies(self, dependencies: str) -> None: + def _check_dependencies(self, dependencies: str, + strict_version: Optional[bool] = True) -> None: if not dependencies: return @@ -749,9 +764,13 @@ def _check_dependencies(self, dependencies: str) -> None: else: raise NotImplementedError( 'operation \'%s\' is not supported' % operation) + message = ('Trying to deserialize a model with dependency ' + '%s not satisfied.' % dependency_string) if not check: - warnings.warn('Trying to deserialize a model with dependency ' - '%s not satisfied.' % dependency_string) + if strict_version: + raise ValueError(message) + else: + warnings.warn(message) def _serialize_type(self, o: Any) -> 'OrderedDict[str, str]': mapping = {float: 'float', diff --git a/openml/flows/functions.py b/openml/flows/functions.py index 53a1fdc0a..74e721d50 100644 --- a/openml/flows/functions.py +++ b/openml/flows/functions.py @@ -71,7 +71,8 @@ def _get_cached_flow(fid: int) -> OpenMLFlow: @openml.utils.thread_safe_if_oslo_installed -def get_flow(flow_id: int, reinstantiate: bool = False) -> OpenMLFlow: +def get_flow(flow_id: int, reinstantiate: bool = False, + strict_version: bool = True) -> OpenMLFlow: """Download the OpenML flow for a given flow ID. Parameters @@ -82,6 +83,9 @@ def get_flow(flow_id: int, reinstantiate: bool = False) -> OpenMLFlow: reinstantiate: bool Whether to reinstantiate the flow to a model instance. + strict_version : bool, default=True + Whether to fail if version requirements are not fulfilled. + Returns ------- flow : OpenMLFlow @@ -91,7 +95,8 @@ def get_flow(flow_id: int, reinstantiate: bool = False) -> OpenMLFlow: flow = _get_flow_description(flow_id) if reinstantiate: - flow.model = flow.extension.flow_to_model(flow) + flow.model = flow.extension.flow_to_model( + flow, strict_version=strict_version) return flow diff --git a/tests/test_flows/test_flow_functions.py b/tests/test_flows/test_flow_functions.py index 02d4b2a7d..3773446c5 100644 --- a/tests/test_flows/test_flow_functions.py +++ b/tests/test_flows/test_flow_functions.py @@ -289,3 +289,5 @@ def test_get_flow_reinstantiate_model_wrong_version(self): # 20 is scikit-learn ==0.20.0 # I can't find a != 0.20 permanent flow on the test server. self.assertRaises(ValueError, openml.flows.get_flow, flow_id=20, reinstantiate=True) + openml.flows.get_flow(flow_id=20, reinstantiate=True, + strict_version=False) From e593f2f8ae62cb0592aab9e9c7c4e4497fa003e3 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Wed, 24 Jul 2019 14:24:39 -0400 Subject: [PATCH 3/8] crying over here --- openml/extensions/sklearn/extension.py | 11 +++++++++-- tests/test_flows/test_flow_functions.py | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/openml/extensions/sklearn/extension.py b/openml/extensions/sklearn/extension.py index cad3a20d8..3cde894c7 100644 --- a/openml/extensions/sklearn/extension.py +++ b/openml/extensions/sklearn/extension.py @@ -301,13 +301,15 @@ def _deserialize_sklearn( rval = self._deserialize_function(value) elif serialized_type == 'component_reference': assert components is not None # Necessary for mypy - value = self._deserialize_sklearn(value, recursion_depth=depth_pp) + value = self._deserialize_sklearn(value, recursion_depth=depth_pp, + strict_version=strict_version) step_name = value['step_name'] key = value['key'] component = self._deserialize_sklearn( components[key], initialize_with_defaults=initialize_with_defaults, - recursion_depth=depth_pp + recursion_depth=depth_pp, + strict_version=strict_version ) # The component is now added to where it should be used # later. It should not be passed to the constructor of the @@ -334,12 +336,14 @@ def _deserialize_sklearn( components=components, initialize_with_defaults=initialize_with_defaults, recursion_depth=depth_pp, + strict_version=strict_version ), self._deserialize_sklearn( o=value, components=components, initialize_with_defaults=initialize_with_defaults, recursion_depth=depth_pp, + strict_version=strict_version ) ) for key, value in sorted(o.items()) @@ -351,6 +355,7 @@ def _deserialize_sklearn( components=components, initialize_with_defaults=initialize_with_defaults, recursion_depth=depth_pp, + strict_version=strict_version ) for element in o ] @@ -814,6 +819,7 @@ def _deserialize_model( components=components_, initialize_with_defaults=keep_defaults, recursion_depth=recursion_depth + 1, + strict_version=strict_version, ) parameter_dict[name] = rval @@ -828,6 +834,7 @@ def _deserialize_model( rval = self._deserialize_sklearn( value, recursion_depth=recursion_depth + 1, + strict_version=strict_version ) parameter_dict[name] = rval diff --git a/tests/test_flows/test_flow_functions.py b/tests/test_flows/test_flow_functions.py index 9dd4d6109..f18816201 100644 --- a/tests/test_flows/test_flow_functions.py +++ b/tests/test_flows/test_flow_functions.py @@ -288,7 +288,7 @@ def test_get_flow_reinstantiate_model_no_extension(self): def test_get_flow_reinstantiate_model_wrong_version(self): # Note that CI does not test against 0.19.1. openml.config.server = self.production_server - _, sklearn_major, _ = LooseVersion(sklearn.__version__).version + _, sklearn_major, _ = LooseVersion(sklearn.__version__).version[:3] flow = 8175 expected = 'Trying to deserialize a model with dependency sklearn==0.19.1 not satisfied.' self.assertRaisesRegex(ValueError, From c487a2cf1e3933a9c1ef39318e7e23287d9ffa77 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Wed, 24 Jul 2019 14:31:38 -0400 Subject: [PATCH 4/8] pass strict version through the cross-validation stuff --- openml/extensions/sklearn/extension.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openml/extensions/sklearn/extension.py b/openml/extensions/sklearn/extension.py index 3cde894c7..1744c16b7 100644 --- a/openml/extensions/sklearn/extension.py +++ b/openml/extensions/sklearn/extension.py @@ -323,7 +323,8 @@ def _deserialize_sklearn( rval = (step_name, component, value['argument_1']) elif serialized_type == 'cv_object': rval = self._deserialize_cross_validator( - value, recursion_depth=recursion_depth + value, recursion_depth=recursion_depth, + strict_version=strict_version ) else: raise ValueError('Cannot flow_to_sklearn %s' % serialized_type) @@ -1013,6 +1014,7 @@ def _deserialize_cross_validator( self, value: 'OrderedDict[str, Any]', recursion_depth: int, + strict_version: Optional[bool] = True ) -> Any: model_name = value['name'] parameters = value['parameters'] @@ -1024,6 +1026,7 @@ def _deserialize_cross_validator( parameters[parameter] = self._deserialize_sklearn( parameters[parameter], recursion_depth=recursion_depth + 1, + strict_version=strict_version ) return model_class(**parameters) From b750ca2960539a5776729e90f37d3971762cfac2 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Fri, 26 Jul 2019 11:02:55 -0400 Subject: [PATCH 5/8] don't try to create a 0.19.1 flow on 0.18 --- tests/test_flows/test_flow_functions.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_flows/test_flow_functions.py b/tests/test_flows/test_flow_functions.py index f18816201..10123782a 100644 --- a/tests/test_flows/test_flow_functions.py +++ b/tests/test_flows/test_flow_functions.py @@ -296,5 +296,7 @@ def test_get_flow_reinstantiate_model_wrong_version(self): openml.flows.get_flow, flow_id=flow, reinstantiate=True) - openml.flows.get_flow(flow_id=flow, reinstantiate=True, - strict_version=False) + if LooseVersion(sklearn.__version__) > "0.19.1": + # 0.18 actually can't deserialize this because of incompatible changes + openml.flows.get_flow(flow_id=flow, reinstantiate=True, + strict_version=False) From 0da581f472e0c5a3a6333e4b41d6d360fad69177 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Fri, 26 Jul 2019 14:46:19 -0400 Subject: [PATCH 6/8] reset flow if version mismatch --- openml/flows/functions.py | 5 +++++ tests/test_flows/test_flow_functions.py | 12 ++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/openml/flows/functions.py b/openml/flows/functions.py index 25088c5ea..2b327f6be 100644 --- a/openml/flows/functions.py +++ b/openml/flows/functions.py @@ -97,6 +97,11 @@ def get_flow(flow_id: int, reinstantiate: bool = False, if reinstantiate: flow.model = flow.extension.flow_to_model( flow, strict_version=strict_version) + if not strict_version: + # check if we need to return a new flow b/c of version mismatch + new_flow = flow.extension.model_to_flow(flow.model) + if new_flow.dependencies != flow.dependencies: + return new_flow return flow diff --git a/tests/test_flows/test_flow_functions.py b/tests/test_flows/test_flow_functions.py index 10123782a..941cb6a90 100644 --- a/tests/test_flows/test_flow_functions.py +++ b/tests/test_flows/test_flow_functions.py @@ -290,13 +290,17 @@ def test_get_flow_reinstantiate_model_wrong_version(self): openml.config.server = self.production_server _, sklearn_major, _ = LooseVersion(sklearn.__version__).version[:3] flow = 8175 - expected = 'Trying to deserialize a model with dependency sklearn==0.19.1 not satisfied.' + expected = ('Trying to deserialize a model with dependency' + ' sklearn==0.19.1 not satisfied.') self.assertRaisesRegex(ValueError, expected, openml.flows.get_flow, flow_id=flow, reinstantiate=True) if LooseVersion(sklearn.__version__) > "0.19.1": - # 0.18 actually can't deserialize this because of incompatible changes - openml.flows.get_flow(flow_id=flow, reinstantiate=True, - strict_version=False) + # 0.18 actually can't deserialize this because of incompatibility + flow = openml.flows.get_flow(flow_id=flow, reinstantiate=True, + strict_version=False) + # ensure that a new flow was created + assert flow.flow_id is None + assert "0.19.1" not in flow.dependencies From c63514c2d5863497f77d21bedf4ca370b8937a4f Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Fri, 16 Aug 2019 15:59:10 -0400 Subject: [PATCH 7/8] fix missing commas, don't be optional --- openml/extensions/sklearn/extension.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/openml/extensions/sklearn/extension.py b/openml/extensions/sklearn/extension.py index f5120e741..8ed13bb29 100644 --- a/openml/extensions/sklearn/extension.py +++ b/openml/extensions/sklearn/extension.py @@ -208,7 +208,7 @@ def remove_all_in_parentheses(string: str) -> str: def flow_to_model(self, flow: 'OpenMLFlow', initialize_with_defaults: bool = False, - strict_version: Optional[bool] = True) -> Any: + strict_version: bool = True) -> Any: """Initializes a sklearn model based on a flow. Parameters @@ -238,7 +238,7 @@ def _deserialize_sklearn( components: Optional[Dict] = None, initialize_with_defaults: bool = False, recursion_depth: int = 0, - strict_version: Optional[bool] = True + strict_version: bool = True, ) -> Any: """Recursive function to deserialize a scikit-learn flow. @@ -309,7 +309,7 @@ def _deserialize_sklearn( components[key], initialize_with_defaults=initialize_with_defaults, recursion_depth=depth_pp, - strict_version=strict_version + strict_version=strict_version, ) # The component is now added to where it should be used # later. It should not be passed to the constructor of the @@ -797,7 +797,7 @@ def _deserialize_model( flow: OpenMLFlow, keep_defaults: bool, recursion_depth: int, - strict_version: Optional[bool] = True + strict_version: bool = True ) -> Any: logging.info('-%s deserialize %s' % ('-' * recursion_depth, flow.name)) model_name = flow.class_name @@ -866,7 +866,7 @@ def _deserialize_model( return model_class(**parameter_dict) def _check_dependencies(self, dependencies: str, - strict_version: Optional[bool] = True) -> None: + strict_version: bool = True) -> None: if not dependencies: return @@ -1018,7 +1018,7 @@ def _deserialize_cross_validator( self, value: 'OrderedDict[str, Any]', recursion_depth: int, - strict_version: Optional[bool] = True + strict_version: bool = True ) -> Any: model_name = value['name'] parameters = value['parameters'] From 2852df9d369ec3aee2234719fad0261a2d2ce1f1 Mon Sep 17 00:00:00 2001 From: Andreas Mueller Date: Fri, 16 Aug 2019 16:02:05 -0400 Subject: [PATCH 8/8] add strict_version to base class --- openml/extensions/extension_interface.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openml/extensions/extension_interface.py b/openml/extensions/extension_interface.py index 6346cb0bf..d963edb1b 100644 --- a/openml/extensions/extension_interface.py +++ b/openml/extensions/extension_interface.py @@ -58,7 +58,9 @@ def can_handle_model(cls, model: Any) -> bool: # Abstract methods for flow serialization and de-serialization @abstractmethod - def flow_to_model(self, flow: 'OpenMLFlow', initialize_with_defaults: bool = False) -> Any: + def flow_to_model(self, flow: 'OpenMLFlow', + initialize_with_defaults: bool = False, + strict_version: bool = True) -> Any: """Instantiate a model from the flow representation. Parameters @@ -69,6 +71,9 @@ def flow_to_model(self, flow: 'OpenMLFlow', initialize_with_defaults: bool = Fal If this flag is set, the hyperparameter values of flows will be ignored and a flow with its defaults is returned. + strict_version : bool, default=True + Whether to fail if version requirements are not fulfilled. + Returns ------- Any