Skip to content
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

Fix-up test_pickle_empty #5303

Merged
merged 8 commits into from
Sep 10, 2021
Merged

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Sep 9, 2021

Builds on and extends PR ( #4595 ). Should address a CI failure seen in PR ( cloudpipe/cloudpickle#432 ).

Switches to using the internal utility class MemoryviewHolder instead of NumPy to allow testing without NumPy. Calls serialize directly with pickle as the only option (instead of calling pickle_dumps). Though also asserts pickling did occur. Also includes some other minor fixes.

Only tries manipulating writability of frames with pickle protocol 5 in use. This doesn't work for earlier pickle protocols as there is only one bytes object containing everything (and no way to inspect what went into it, let alone reproduce it).

cc @madsbk @pierreglaser

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed / isort distributed

@jakirkham
Copy link
Member Author

@dask/maintenance could someone please review? 🙂

This is needed to fix cloudpickle's CI, which runs these tests.

@jakirkham
Copy link
Member Author

Planning on merging EOD tomorrow if no comments

@jacobtomlinson jacobtomlinson merged commit 0774365 into dask:main Sep 10, 2021
@jakirkham jakirkham deleted the fixup_tst_pickle branch September 10, 2021 14:51
@jakirkham
Copy link
Member Author

Thanks Jacob! 😄

@jakirkham
Copy link
Member Author

To help catch the kind of issue seen in cloudpickle's CI earlier, am adding explicit testing of pickle protocol 4 & 5 in PR ( #5313 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants