Skip to content

Optimize the numpy hook#542

Merged
mmckerns merged 4 commits intouqfoundation:masterfrom
leogama:numpy-hook
Aug 13, 2022
Merged

Optimize the numpy hook#542
mmckerns merged 4 commits intouqfoundation:masterfrom
leogama:numpy-hook

Conversation

@leogama
Copy link
Contributor

@leogama leogama commented Aug 11, 2022 •
edited
Loading

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 •
edited
Loading

@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

mmckerns commented Aug 12, 2022

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
@leogama
Remove try-except clauses to catch ReferenceError
505d63c 
- 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

mmckerns commented Aug 13, 2022

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 •
edited
Loading

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

mmckerns commented Aug 13, 2022

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

Reviewers

@mmckerns mmckerns mmckerns approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

dill-0.3.6

Development

Successfully merging this pull request may close these issues.

2 participants

@leogama @mmckerns