[MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning #8526

zxcvbnius · 2017-03-04T19:42:39Z

Reference Issue

What does this implement/fix? Explain your changes.

Fix test of SingleInheritanceEstimator to not raise DeprecationWarning.
Set sklearn.__version__ to the current version.

Any other comments?

jnothman · 2017-03-05T11:10:20Z

sklearn/tests/test_base.py

@@ -436,6 +436,7 @@ def __init__(self, attribute_pickled=5):

    def __getstate__(self):


Remind me, @HolgerPeters, why this method does not call super(...).__getstate__...?

Hi @jnothman thanks for your review!
I think it may not be appropriate to use super(...).__getstate__ because the next test case is test_pickling_works_when_getstate_is_overwritten_in_the_child_class.

@jnothman I think this was supposed to test the case where users have implemented an estimator that overrides __getstate__

Yes, that is the intention of the test case. We had written our own serialization logic for estimators, and scikit-learn 0.18 broke all of our modelling code. For what its worth since the BaseEstimator does not contain parameters (and does not contain estimated parameters) scikit-learn shouldn't expect that users inheriting from BaseEstimator and overwriting __getstate__ make that super(..).__getstate__ call.

lesteve · 2017-03-10T12:40:34Z

sklearn/tests/test_base.py

@@ -436,6 +436,7 @@ def __init__(self, attribute_pickled=5):

    def __getstate__(self):
        data = self.__dict__.copy()
+        data["_sklearn_version"] = sklearn.__version__


I am not entirely sure this is the right fix. I don't remember whether this test was supposed to test estimators outside scikit-learn or not. @HolgerPeters do you remember off the top of your head?

A closer look at #8324 may be needed.

The purpose of this test case is not specifically to test estimators outside of scikit-learn. here we test estimators outside of sklearn.

Yet, implicitly it probably does, because estimators within scikit-learn are probably refactored to work with BaseEstimator's new __getstate__/__setstate__ logic

I still think that BaseEstimator shouldn't overwrite __getstate__ and scikit-learn should probably just not try to do those version checks. I understand the intention of course, but basically scikit-learn user's must have a strategy in place for making sure that they don't unpickle estimators with the wrong scikit-learn version. To me seems scikit-learn tries here to address a general problem with pickle in a very scikit-learn specific way, which has unintended side-effects on user code that ran fine for years.

To really address the problem of serialization-versioning would probably mean to introduce special serialization code (which would mean A LOT of code writing). But then, you could start to maintain a versionizable serialization interface.

(@zxcvbnius this comment is not about your code in this PR, but about the background of the code you modify).

Thanks a lot for the details @HolgerPeters!

@zxcvbnius I think your change is fine. Add a comment on the previous line explaining that the line you added avoids a DeprecationWarning about unpickling an estimator from a different version.

@lesteve I reworked my comment and added it to the original ticket because this PR is probably not the best place for this discussion.

@zxcvbnius I also think the change is ok.

@lesteve ok!
@HolgerPeters , really thanks for your detail explanation!

jnothman · 2017-03-11T20:55:21Z

If the test is meant to reflect behaviour for an estimator outside of scikit-learn, then it's the wrong fix. Ignoring the warning, or hacking the __module__ would be better, with a comment, imo. This is just a side effect is us having tests within the package, is it not?

…

On 11 Mar 2017 1:34 am, "Holger Peters" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/tests/test_base.py <#8526 (comment)> : > @@ -436,6 +436,7 @@ def __init__(self, attribute_pickled=5): def __getstate__(self): data = self.__dict__.copy() + data["_sklearn_version"] = sklearn.__version__ @zxcvbnius <https://github.com/zxcvbnius> I also think the change is ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8526 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6xIHRgOwZHENxXnWeIAmDBlw5cWNks5rkV-HgaJpZM4MTMLu> .

lesteve · 2017-03-13T12:16:24Z

lesteve commented

Mar 13, 2017

If the test is meant to reflect behaviour for an estimator outside of scikit-learn, then it's the wrong fix

According to #8526 (comment), it looks like this is not the purpose of the test.

jnothman · 2017-03-13T12:18:36Z

I still suspect ignore_warnings is a more appropriate solution

lesteve · 2017-03-13T12:23:52Z

I still suspect ignore_warnings is a more appropriate solution

Fine with me. @zxcvbnius can add a @ignore_warnings to the test function please?

…onWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Ignore a DeprecationWarning about unpickling an estimator from a different version

jnothman · 2017-03-20T13:55:50Z

sklearn/tests/test_base.py

        data["_attribute_not_pickled"] = None
        return data


+@ignore_warnings(category=(DeprecationWarning, UserWarning))


why DeprecationWarning?

@jnothman I think I should not be confused by DeprecationWarning in the title.
According to the following error message:
UserWarning: Trying to unpickle estimator SingleInheritanceEstimator from version pre-0.18 when using version 0.19.dev0. ...
We just need UserWarning?!

jnothman · 2017-03-20T22:28:54Z

sklearn/tests/test_base.py

        data["_attribute_not_pickled"] = None
        return data


+@ignore_warnings(category=(DeprecationWarning, UserWarning))


Yes, I suspect DeprecationWarning was an error
…

On 21 Mar 2017 7:17 am, "zxcvbnius" ***@***.***> wrote: In sklearn/tests/test_base.py <#8526 (comment)> : > data["_attribute_not_pickled"] = None > return data > > > ***@***.***_warnings(category=(DeprecationWarning, UserWarning)) @jnothman <https://github.com/jnothman> I think I should not be confused by DeprecationWarning in the title. According to the following error message: *UserWarning: Trying to unpickle estimator SingleInheritanceEstimator from version pre-0.18 when using version 0.19.dev0. ...* We just need UserWarning?! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/scikit-learn/scikit-learn/pull/8526/files/6e527d117c57a1ea83b902a93b2af33c574a96b3..f9cd1070568cf7709e3475bf25bbb2af17b270f2#r107003281>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6-kpH6PIYsVTGBbQpyUr-tYCkF8Nks5rnt7ogaJpZM4MTMLu> .

…onWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Test of SingleInheritanceEstimator would raise UserWarning, not DeprecationWarning - Ignore a UserWarning about unpickling an estimator from a different version

jnothman · 2017-03-21T01:27:57Z

LGTM (I ran the tests).

PS: you are using very strange commit messages. They should describe what change the commit makes. MRG+1, MRG+2 should have nothing to do with your commit message.

zxcvbnius · 2017-03-21T02:17:12Z

@jnothman yes, you're right. Thanks for your reminder. I will be careful next time.

MechCoder · 2017-03-31T23:20:57Z

Thanks!

…onWarning (scikit-learn#8526) * Fix test of SingleInheritanceEstimator to not raise DeprecationWarning * [MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Ignore a DeprecationWarning about unpickling an estimator from a different version * [MRG+2] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Test of SingleInheritanceEstimator would raise UserWarning, not DeprecationWarning - Ignore a UserWarning about unpickling an estimator from a different version

…onWarning (scikit-learn#8526) * Fix test of SingleInheritanceEstimator to not raise DeprecationWarning * [MRG+1 102BB ] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Ignore a DeprecationWarning about unpickling an estimator from a different version * [MRG+2] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Test of SingleInheritanceEstimator would raise UserWarning, not DeprecationWarning - Ignore a UserWarning about unpickling an estimator from a different version

…onWarning (scikit-learn#8526) * Fix test of SingleInheritanceEstimator to not raise DeprecationWarning * [MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Ignore a DeprecationWarning about unpickling an estimator from a different version * [MRG+2] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Test of SingleInheritanceEstimator would raise UserWarning, not DeprecationWarning - Ignore a UserWarning about unpickling an estimator from a different version

zxcvbnius added 2 commits March 4, 2017 14:37

Fix test of SingleInheritanceEstimator to not raise DeprecationWarning

c400ec4

Merge branch 'master' into sprint

6e527d1

jnothman reviewed Mar 5, 2017

View reviewed changes

lesteve reviewed Mar 10, 2017

View reviewed changes

HolgerPeters mentioned this pull request Mar 10, 2017

Model persistence warnings across versions #7135

Closed

[MRG+1] Fix test of SingleInheritanceEstimator to not raise Deprecati…

f9cd107

…onWarning (scikit-learn#8526) Fix: test of SingleInheritanceEstimator to not raise DeprecationWarning (scikit-learn#8526) - Ignore a DeprecationWarning about unpickling an estimator from a different version

jnothman reviewed Mar 20, 2017

View reviewed changes

jnothman reviewed Mar 20, 2017
View reviewed changes

jnothman changed the title ~~[MRG] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning~~ [MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning Mar 21, 2017

MechCoder merged commit 54e8951 into scikit-learn:master Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning #8526

[MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning #8526

Uh oh!

10000 Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -436,6 +436,7 @@ def __init__(self, attribute_pickled=5):

		def __getstate__(self):

Uh oh!

[MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning #8526

[MRG+1] Fix test of SingleInheritanceEstimator to not raise DeprecationWarning #8526

Uh oh!

Conversation

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

10000 Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!