Remove and deprecate unused functions

[jrbourbeau]

From what I can tell it appears these functions aren't used anywhere and the test suite has coverage for Tornado coroutines, Ellipsis, and NotImplemented.

cloudpickle/tests/cloudpickle_test.py

Lines 798 to 804 in 343da11

	def test_Ellipsis(self):
	self.assertEqual(Ellipsis,
	pickle_depickle(Ellipsis, protocol=self.protocol))
	def test_NotImplemented(self):
	ExcClone = pickle_depickle(NotImplemented, protocol=self.protocol)
	self.assertEqual(NotImplemented, ExcClone)

cloudpickle/tests/cloudpickle_test.py

Lines 810 to 816 in 343da11

	def test_EllipsisType(self):
	res = pickle_depickle(type(Ellipsis), protocol=self.protocol)
	self.assertEqual(type(Ellipsis), res)
	def test_NotImplementedType(self):
	res = pickle_depickle(type(NotImplemented), protocol=self.protocol)
	self.assertEqual(type(NotImplemented), res)

cloudpickle/tests/cloudpickle_test.py

Line 964 in 343da11

def test_tornado_coroutine(self):

This PR proposes we remove these unused functions, though do let me know if I'm missing something and these utilities are in fact needed for some external code paths.

[codecov]

Codecov Report

Merging #444 (d9ffb4f) into master (fca7b22) will decrease coverage by 2.67%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   92.36%   89.68%   -2.68%     
==========================================
  Files           4        4              
  Lines         720      698      -22     
  Branches      150      147       -3     
==========================================
- Hits          665      626      -39     
- Misses         34       62      +28     
+ Partials       21       10      -11     

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	82.96% <66.66%> (-5.35%)	⬇️
cloudpickle/__init__.py	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fca7b22...d9ffb4f. Read the comment docs.

[ogrisel]

Probably a left over of a previous refactoring. +1 for cleaning up.

@pierreglaser any opinion?

[pierreglaser]

So, I believe that _rebuild_tornado_corutine, and the _gen_ functions may be used to re-build pickles containing tornado coroutines, or NotImplemented etc... created using older versions of cloudpickle. Maybe worth adding a small deprecation cycle, as we did for _make_skel_func for instance?

By the way, _make_skel_func can be now removed, since we're past the end of its deprecation cycle 1.7.0.

As for is_tornado_coroutine, I guess this is semi-public, and I'm not sure whether we have been consistent in handling the removal of such functions. My guess is that we can remove it.

[jrbourbeau]

Maybe worth adding a small deprecation cycle, as we did for _make_skel_func for instance?

Thanks for the tip, I've added a deprecation message to these functions

By the way, _make_skel_func can be now removed, since we're past the end of its deprecation cycle 1.7.0.

Hmm this has caused several of the tests/test_backward_compat.py tests to start failing with

AttributeError: Can't get attribute '_make_skel_func' on <module 'cloudpickle.cloudpickle' from '/home/runner/work/cloudpickle/cloudpickle/cloudpickle/cloudpickle.py'>

It looks like there's a tests/generate_old_pickles.py script for generating the old pickle files used by these tests. I'm wondering what an appropriate "old" version of cloudpickle would be for these tests after removing _make_skel_func?

EDIT: Based on the deprecation message in _make_skel_func, perhaps we should regenerate the static test files using cloudpickle 1.5.0?

[pierreglaser]

EDIT: Based on the deprecation message in _make_skel_func, perhaps we should regenerate the static test files using cloudpickle 1.5.0?

Yes. The pickles that we generate using cloudpickle 1.5.0 should not rely on _make_skel_func, regenerating the pickles should work :)

[jrbourbeau]

Alright, @pierreglaser I think this is ready for another look. Recent commits added a GitHub actions workflow to help automatically generate the static pickle files used in tests/test_backward_compat.py and upload them as GitHub actions artifacts. This will hopefully make it easier to regenerate them in the future.

That said, I'm not sure how often this will be needed. We may want to comment out the workflow to avoid it running on each PR and just uncomment it when needed. Alternatively, we could take an approach similar to the downstream CI builds

cloudpickle/.github/workflows/testing.yml

Line 141 in b959d80

if: "contains(github.event.pull_request.labels.*.name, 'ci distributed') || contains(github.event.pull_request.labels.*.name, 'ci downstream')"

and add a "generate-pickles" label (or some other name) to determine when the workflow is run. No strong opinion from me one way or the other.

[jrbourbeau]

Since _make_skel_func has been removed we no longer raise a warning in most cases for the tests in this module. Because of this I decided to change check_deprecation_warning to be False by default, so checking deprecation warnings will need to be done explicitly. Happy to try something else out though

[ogrisel]

This PR still @lgtm besides the following minor point.

@pierreglaser and second review?

[ogrisel]

Indeed, I think we should not run this at each PR. Let's use a manual trigger instead: https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow

[pierreglaser]

Thanks @jrbourbeau. @ogrisel manually triggering the pickle generation workflow sounds good to me. I just have one question on the current way artifacts are stored across workflows.

[pierreglaser]

IIUC, Storing pickles as artifacts implies that these pickles will only be available for 90 days (see https://github.com/actions/upload-artifact#retention-period), so we need maintainers to re-run this workflow every 90 days. Are people fine with it? @ogrisel @jrbourbeau

[ogrisel]

Closing now that #517 was merged with similar changes and further simplification (because of the drop of Python 3.6 and 3.7 support).

[ogrisel]

Actually this PR also add the automation for the generation of old pickle files while in #517 I checked them into the git repo.

@jrbourbeau / @pierreglaser feel free to reopen a PR with that change if you prefer the automated way of dealing with this.