Skip to content

MAINT, TST refactor pickle imports and tests #12133

Merged
charris merged 1 commit intonumpy:masterfrom
pierreglaser:centralize-pickle-import
Oct 10, 2018
Merged

MAINT, TST refactor pickle imports and tests #12133
charris merged 1 commit intonumpy:masterfrom
pierreglaser:centralize-pickle-import

Conversation

@pierreglaser
Copy link
Copy Markdown
Contributor

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:

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.

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 numpy#12090
Copy link
Copy Markdown
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this as a separate PR



class TestPickling(object):
def test_highest_available_pickle_protocol(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@pierreglaser pierreglaser Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@charris
Copy link
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants