New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT, TST refactor pickle imports and tests #12133

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@pierreglaser
Copy link
Contributor

pierreglaser commented Oct 10, 2018

This PR is essentially the first commit of #12011. If you'd rather merge #12011 directly, feel free to close it.
It is a numpy-wide refactor of pickle importing and pickle.dump(s) function testing. Most notably:

  • Some pickle.dumps tests that I did not catch in #12090 that are now looped over for every pickle protocol
  • Some refactoring about how pickle is imported internally: since we don't want a big conditional block such as:
if sys.version_info[0] >= 3:
    if sys.version_info[1] in (6, 7):
        try:
            import pickle5 as pickle
        except ImportError:
            import pickle
    else:
        import pickle
else:
    import cPickle as pickle

each time we import pickle in python, we import it once this way, in numpy.core.numeric, and then each time pickle is used in other files, we import it from numpy.core.numeric. The only time we import pickle another way is in numpy/core/setup.py

Now, numpy should be clean pickle-wise, meaning:

  • there is no more np.ndarray.dump(s) call in the test suite.
  • there is a consistent pickle importing behavior
  • each time pickle.dumps is tested, it is tested for every protocol.
MAINT, TST import pickle from numpy.core.numeric
All imports of pickle from numpy modules are now done this way:
>>> from numpy.core.numeric import pickle

Also, some loops on protocol numbers are added over pickle tests that
were not caught from #12090
@mattip

mattip approved these changes Oct 10, 2018

Copy link
Member

mattip left a comment

I like this as a separate PR

@@ -3549,8 +3551,18 @@ def test_test_zero_rank(self):


class TestPickling(object):
def test_highest_available_pickle_protocol(self):

This comment has been minimized.

@charris

charris Oct 10, 2018

Member

This test is a bit strange, why is it needed?

This comment has been minimized.

@pierreglaser

pierreglaser Oct 10, 2018

Contributor

By the time I started writing this set of PRs, it was a sort of meta-test that made sure all pickle protocol test loops were actually including protocol 5 if possible (e.g if pickle5 was installed), especially for the ndarray class.
Now things have a bit changed: we loop over protocols in plenty of other test files, yet this test is only ran here. I agree that this test is less relevant now, and it can actually be removed for the sake of clarity and consistency.

f.close()
assert_array_equal(a, b)
for proto in range(2, pickle.HIGHEST_PROTOCOL + 1):
f = BytesIO()

This comment has been minimized.

@charris

charris Oct 10, 2018

Member

I think most of these types of tests should be changed to use BytesIO as a context manager, but that is out of scope for this PR.

@charris charris merged commit 9942b71 into numpy:master Oct 10, 2018

8 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No alert changes
Details
Shippable Run 828 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 94.28% of diff hit (target 85.04%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +9.24% compared to c156bc1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@charris

This comment has been minimized.

Copy link
Member

charris commented Oct 10, 2018

Thanks @pierreglaser .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment