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 deepcopy of TaggedList #788

Merged

Conversation

eslavich
Copy link
Contributor

The current behavior:

In [1]: from asdf.tagged import TaggedList; from copy import deepcopy

In [2]: foo = TaggedList([1, 2, 3])

In [3]: deepcopy(foo)
Out[3]: [1, 2, 3, 1, 2, 3]

In [4]: deepcopy(deepcopy(foo))
Out[4]: [1, 2, 3, 1, 2, 3, 1, 2, 3, 1, 2, 3]

This has something to do with the TaggedList class inheriting from both list and UserList, but we can't drop list, because code elsewhere is probably relying upon this:

In [5]: isinstance(foo, list)
Out[5]: True

Which isn't the case when we inherit from UserList only -- users are expected to check collections.abc.Sequence instead. I'm in favor of dropping list eventually, but that seems like a 3.0 change.

This PR overrides __deepcopy__ to correct the issue with UserList. It does the same on UserDict, even though there wasn't a problem there, since the custom __deepcopy__ ended up being ~ 60% faster than whatever was happening without it. Overriding __copy__ offers no improvement, so I just left that alone.

Thanks to @jdavies-st for suggesting both __deepcopy__ and profiling UserDict!

@eslavich eslavich added the bug label Apr 29, 2020
@eslavich eslavich added this to the 2.6.1 milestone Apr 29, 2020
@eslavich eslavich force-pushed the eslavich-fix-tagged-deepcopy branch from f64413d to 61c7f56 Compare May 4, 2020 21:35
@eslavich
Copy link
Contributor Author

eslavich commented May 4, 2020

Turns out we also need a __copy__ override -- in Python < 3.8, copying the objects will result in MemoryError.

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #788 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
+ Coverage   93.79%   93.87%   +0.07%     
==========================================
  Files          43       43              
  Lines        5028     5041      +13     
==========================================
+ Hits         4716     4732      +16     
+ Misses        312      309       -3     
Impacted Files Coverage Δ
asdf/tagged.py 93.54% <100.00%> (+7.83%) ⬆️

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 2d64844...61c7f56. Read the comment docs.

Copy link
Contributor

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

I wouldn't have guessed that changed between python 3.7 and 3.8. Is it documented somewhere?

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

Must be fun exploring these weird corners of behavior.

@perrygreenfield perrygreenfield merged commit 37ab3d4 into asdf-format:master May 5, 2020
@eslavich eslavich modified the milestones: 2.6.1, 2.7.0 Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants