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

feat!: remove deprecations for 1.4.0 release #2688

Merged
merged 11 commits into from
Sep 4, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Sep 2, 2023

Per the wiki:

  • typestr argument will be removed from Form objects.
  • Form.type_from_behavior(...) will be removed in favour of Form.type.
  • ak.typetracer.empty_if_typetracer will be removed in favour of ak.typetracer.length_zero_if_typetracer.
  • UnionArray.simplified's merge argument will be removed.
  • The built-in string and categorical behaviors will be removed.

This reverts commit 9218a77. This is required because it these classes are likely serialised in some behavior dictionaries
@agoose77 agoose77 temporarily deployed to docs-preview September 2, 2023 15:45 — with GitHub Actions Inactive
@agoose77
Copy link
Collaborator Author

agoose77 commented Sep 2, 2023

Hmm, looks like we probably shouldn't remove the ak.behaviors.string module (or categorical) just yet; any pickles whose array class is defined in these modules will thus fail to load. I propose to wait, and eventually we'll be able to remove these outright. Let's bump the removal to 6-12 months?

@agoose77 agoose77 temporarily deployed to docs-preview September 2, 2023 16:01 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #2688 (da12418) into main (a5efdfe) will increase coverage by 0.05%.
The diff coverage is 84.84%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/__init__.py 96.96% <ø> (-0.18%) ⬇️
src/awkward/behaviors/categorical.py 0.00% <0.00%> (-71.43%) ⬇️
src/awkward/forms/form.py 82.12% <ø> (+0.43%) ⬆️
src/awkward/typetracer.py 87.09% <ø> (+2.48%) ⬆️
src/awkward/types/optiontype.py 81.48% <60.00%> (+2.79%) ⬆️
src/awkward/types/unknowntype.py 84.00% <71.42%> (+4.58%) ⬆️
src/awkward/behaviors/string.py 100.00% <100.00%> (+36.36%) ⬆️
src/awkward/contents/unionarray.py 85.57% <100.00%> (+0.07%) ⬆️
src/awkward/types/listtype.py 95.74% <100.00%> (+3.29%) ⬆️
src/awkward/types/numpytype.py 94.68% <100.00%> (+1.68%) ⬆️
... and 4 more

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Thanks for making each deprecated feature removal a separate commit; it was much easier to review each commit separately!

Hmm, looks like we probably shouldn't remove the ak.behaviors.string module (or categorical) just yet; any pickles whose array class is defined in these modules will thus fail to load. I propose to wait, and eventually we'll be able to remove these outright. Let's bump the removal to 6-12 months?

Good point—to maintain backward pickle compatibility, we can never fully remove them. But they can become hollow shells of their former selves, like the ak._ext.Form and subclasses in

from awkward.forms import (
BitMaskedForm,
ByteMaskedForm,
EmptyForm,
Form,
IndexedForm,
IndexedOptionForm,
ListForm,
ListOffsetForm,
NumpyForm,
RecordForm,
RegularForm,
UnionForm,
UnmaskedForm,
)

If all we're trying to do is have pickle.load not fail on old data, the ak.behaviors.string.StringBehavior etc. classes must continue to exist at that address, but they don't have to be special classes. We can keep the string.py and categorical.py modules in behaviors/, but StringBehavior etc. can be alternate names for ak.Array (i.e. no special class for them). After unpickling, the arrays would no longer carry any residue of pickle having declared that they should have these classes.

If anyone asks why we have these pass-throughs, the answer can be the same as for _ext.py: to preserve old pickles. (A comment can explain that in-situ.) That would minimize cognitive overhead.

@agoose77
Copy link
Collaborator Author

agoose77 commented Sep 4, 2023

Hmm, the thought had occurred to me but I dismissed it as non-viable. I'm not sure why, though; it's a good idea!

Now this can merge :)

@agoose77 agoose77 enabled auto-merge (squash) September 4, 2023 18:22
@agoose77 agoose77 temporarily deployed to docs-preview September 4, 2023 18:39 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit aaf09c1 into main Sep 4, 2023
34 checks passed
@agoose77 agoose77 deleted the agoose77/feat!-remove-deprecations-1.4.0 branch September 4, 2023 18:43
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.

None yet

2 participants