Repository navigation
Backport PR #13614 on branch v5.0.x (MNT: Compatibility with Python 3.11) - #13664
Conversation
|
@nstarman , the cosmology patch does not seem to apply to v5.0.x, does that sound right to you? |
|
Okaaay... it tries to build numpy from source too. I guess we cannot use Stuart's new way for wheels here. |
|
Okay, it wasn't scipy or numpy... It was astropy... But why? https://github.com/astropy/astropy/actions/runs/3054485869/jobs/4926446296 |
432ba98 to
121b1d7
Compare
|
@eerovaher @nstarman , the patch for pickling isn't compatible with the cosmology code in LTS. How can we fix this? Thanks! https://github.com/astropy/astropy/actions/runs/3054566720/jobs/4926621567 |
| # NOTE: The call may need to be adapted if other objects implementing a __getstate__ | ||
| # with required argument(s) are passed to this function. | ||
| # For a class that has no instance __dict__ and no __slots__, the default state is None | ||
| dict_a = a.__getstate__(a) if inspect.isclass(a) else a.__getstate__() or dict() |
There was a problem hiding this comment.
@dhomeier , do we also need this in main and v5.1.x ?
There was a problem hiding this comment.
Yes, we should – I think in principle any function or other thing that returns a state of None might be pickled.
There was a problem hiding this comment.
Reading about the Python 3.11 object.__getstate__() I am getting the impression that even the patched code here might not behave as expected in the case where a class uses __slots__. Should we patch this code even further?
There was a problem hiding this comment.
Do you know an example of something that implements __slots__?
For a class that has
__slots__and no instance__dict__, the default state is a tuple whose first item is None and whose second item is a dictionary mapping slot names to slot values described in the previous bullet.
It looks like we could use a.__getstate__()[1] in that case...
There was a problem hiding this comment.
There are classes in astropy that use __slots__:
$ git grep __slots__
CHANGES.rst:4077: because the class now uses ``__slots__`` any attributes on the class have to
astropy/cosmology/funcs/comparison.py:46: __slots__ = ("wrapped", )
astropy/table/bst.py:67: __slots__ = ('val',)
astropy/table/bst.py:108: __slots__ = ('key', 'data', 'left', 'right')
astropy/table/soco.py:17: __slots__ = ('key', 'value')
astropy/utils/data_info.py:237: dct.setdefault('__slots__', [])
astropy/utils/data_info.py:288: __slots__ = ['_parent_cls', '_parent_ref', '_attrs']
astropy/utils/data_info.py:534: __slots__ = ['_format_funcs', '_copy_indices']There was a problem hiding this comment.
And hopefully more will use slots in py3.10+ with dataclass(slots=True)!
There was a problem hiding this comment.
All those cases also have __dict__, so that takes precedence. However there is a weird exception astropy.utils.data_info:
from astropy.utils.data_info import DataInfo
DataInfo.__dict__.keys()
dict_keys(['__module__', '__doc__', '_stats', 'attrs_from_parent', 'attr_names', '_attr_defaults', '_attrs_no_copy', '_info_summary_attrs', '__slots__', '_represent_as_dict_attrs', '_construct_from_dict_args', '_represent_as_dict_primary_data', '__init__', '_parent', '__get__', '__set__', '__getstate__', '__setstate__', '_represent_as_dict', '_construct_from_dict', 'info_summary_attributes', 'info_summary_stats', '__call__', '__repr__', '_attrs', '_parent_cls', '_parent_ref', 'description', 'dtype', 'name', 'meta', 'format', 'unit'])
DataInfo.__getstate__(DataInfo)
<member '_attrs' of 'DataInfo' objects>That member_descriptor has neither keys nor is it iterable, no idea what use to make of that...
There was a problem hiding this comment.
Could force it somehow to fall back to a.__dict__ of course
If it is compiling numpy then there isn't a wheel for it so the alternative is that it errors. |
Darn. The file |
It is passing in https://github.com/astropy/astropy/actions/runs/3055240601/jobs/4928078581, isn't it? |
I am a bit confused now – this is the The pickling tests seem to have moved elsewhere like into Or is there another backport that is still failing? |
|
Ok, taking a look... Everything's passing! That was easy 😆 |
|
It does but looks like a7190c4 fixed it? Is that good enough? I see there are still some discussions. |
|
|
Okay... Ping me when there is a consensus. Thanks! |
|
|
|
Do you mind if I squash the commits? |
|
Good idea to squash, but updates made here to |
Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)
Make pickling tests more robust `astropy.tests.helper.generic_recursive_equality_test()`, which is used in many pickling tests, is now more robust against objects with `__slots__` or without `__dict__`. Co-authored-by: Eero Vaher <eero.vaher@astro.lu.se>
965269d to
fd7d1f2
Compare
|
Okay, squashed. The forward port is at #13675 |
|
Huh... @nstarman , do we really need exact match here? Can we use |
|
All close is sufficient! |
and remove unused imports
|
CI finally green again. Thank you, all! |
Description
This pull request is to manually backport #13614 to v5.0.x branch.
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.