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

Optimize the numpy hook #542

Merged
merged 4 commits into from
Aug 13, 2022
Merged

Optimize the numpy hook #542

merged 4 commits into from
Aug 13, 2022

Conversation

leogama
Copy link
Contributor

@leogama leogama commented Aug 11, 2022

As this is now called from Pickler.save(), which can be called from a dozen to thousands of times for a single "dump":

  • Moved repeated logic to outside the numpy test functions
  • Only create a save function once for each type
  • Remove logic related to Python 2 old style classes
  • Substituted black magic like comparing id(obj1.__bound_method__) == id(obj2.__bound_method__)* by type(obj1).__unbound_method__ is type(obj2).__unbound_method__)

(*) I was surprised that it worked at all, must be some kind of optimization but this is not described in the language.

Amend to #540

@leogama
Copy link
Contributor Author

leogama commented Aug 12, 2022

@mmckerns: The tests are failing because CLibraryLoaderType (ctypes.cdll of type ctypes.LibraryLoader) is registered as unpickleable for CPython < 3.11b3, but it pickles just fine. This class has __reduce__() and __reduce_ex__() methods implemented.

dill/dill/_objects.py

Lines 191 to 193 in bf0f4fa

z = (sys.platform[:3] == 'win' or sys.platform[:6] == 'darwin') # non-'nux
z = a if (sys.hexversion >= 0x30b00b3 and not z) else x
z['CLibraryLoaderType'] = ctypes.cdll

But why on Earth are the tests of other PRs passing? Seems like a heisenbug... I see this same error on my machine.

@mmckerns
Copy link
Member

Obviously, the types should just be registered the once, so yeah checking if they are in the dispatch table is the right thing to do. Thanks for the PR.

@mmckerns mmckerns added this to the dill-0.3.6 milestone Aug 12, 2022
- Only type(obj) is passed to the numpy helper functions, and calling type()
    on a proxy of a deleted weakly-referenced object is safe.
- Similarly, using "%r" for string substitution with a proxy of a deleted
    object is safe, so there's no need to have a 'R3' case in save_weakproxy()
- Removed test cases from test_weakref.py related to Python 2 old-style classes
- Fixed a bad use of 'NoReturn' instead of 'None' in logger.py
@mmckerns
Copy link
Member

The tests are failing because CLibraryLoaderType (ctypes.cdll of type ctypes.LibraryLoader) is registered as unpickleable

It was failing due to getattr(cdll, 'mro', int.mro) throwing a OSError in the numpy* check functions. Fixed in: 43fd2ca

@leogama
Copy link
Contributor Author

leogama commented Aug 13, 2022

It was failing due to getattr(cdll, 'mro', int.mro) throwing a OSError in the numpy* check functions.

This doesn't make sense... The old versions of the numpy* functions were effectively calling getattr(cdll.__class__, 'mro', int.mro) (on the cdll class, not on the object). Furthermore, the lines that were generating the error are removed in this PR. So if the supposed failure to pickle cdll has anything to do with that functions, just the adjustment to _objects.py should be enough to solve the test error.

Note: on my machine, with all Python versions, cdll pickles just fine, so I can't track down this issue.

@mmckerns
Copy link
Member

Note: on my machine, with all Python versions, cdll pickles just fine, so I can't track down this issue.

I have VMs for all supported platforms. The OSError didn't occur on linux, but did otherwise... so, yes, once that was adjusted for, then cdll pickled... and adjusting _objects.py enables the tests to pass.

@mmckerns mmckerns merged commit 01bab78 into uqfoundation:master Aug 13, 2022
@leogama leogama deleted the numpy-hook branch August 13, 2022 20:42
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.

2 participants