Skip to content

Backport PR #13614 on branch v5.0.x (MNT: Compatibility with Python 3.11) - #13664

Merged
pllim merged 4 commits into
astropy:v5.0.xfrom
pllim:manual-backport-of-pr-13614-on-v5.0.x
Sep 15, 2022
Merged

pllim merged 4 commits into
astropy:v5.0.xfrom
pllim:manual-backport-of-pr-13614-on-v5.0.x

Conversation

@pllim

@pllim pllim commented Sep 14, 2022 •

Copy link
Copy Markdown
Member

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Do the proposed changes need the code-style fixed?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@pllim pllim added skip-changelog-checks Tells bot to skip changlog checks skip-basebranch-check Skip base branch check for direct backports labels Sep 14, 2022
@pllim pllim added this to the v5.0.5 milestone Sep 14, 2022
@pllim

pllim commented Sep 14, 2022

Copy link
Copy Markdown
Member Author

@nstarman , the cosmology patch does not seem to apply to v5.0.x, does that sound right to you?

@pllim

pllim commented Sep 14, 2022

Copy link
Copy Markdown
Member Author

Okaaay... it tries to build numpy from source too. I guess we cannot use Stuart's new way for wheels here.

@pllim

pllim commented Sep 14, 2022

Copy link
Copy Markdown
Member Author

Okay, it wasn't scipy or numpy... It was astropy... But why?

https://github.com/astropy/astropy/actions/runs/3054485869/jobs/4926446296

      astropy/table/_np_utils.c:196:12: fatal error: longintrepr.h: No such file or directory
        196 |   #include "longintrepr.h"
            |            ^~~~~~~~~~~~~~~
      compilation terminated.
      error: command '/usr/bin/gcc' failed with exit code 1

@pllim
pllim force-pushed the manual-backport-of-pr-13614-on-v5.0.x branch from 432ba98 to 121b1d7 Compare September 14, 2022 16:47
@pllim

pllim commented Sep 14, 2022 •

Copy link
Copy Markdown
Member Author

@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

    def generic_recursive_equality_test(a, b, class_history):
        """
        Check if the attributes of a and b are equal. Then,
        check if the attributes of the attributes are equal.
        """
        if PYTHON_LT_3_11:
            dict_a = a.__getstate__() if hasattr(a, '__getstate__') else a.__dict__
        else:
            # NOTE: The call may need to be adapted if other objects implementing a __getstate__
            # with required argument(s) are passed to this function.
            dict_a = a.__getstate__(a) if inspect.isclass(a) else a.__getstate__()
        dict_b = b.__dict__
>       for key in dict_a:
E       TypeError: 'NoneType' object is not iterable
FAILED astropy/cosmology/tests/test_pickle.py::test_flrw[0-FLRW-False]
FAILED astropy/cosmology/tests/test_pickle.py::test_flrw[1-FLRW-False]
FAILED astropy/cosmology/tests/test_pickle.py::test_flrw[-1-FLRW-False]

Comment thread astropy/tests/helper.py Outdated
Comment thread astropy/tests/helper.py Outdated
# 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()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dhomeier , do we also need this in main and v5.1.x ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, we should – I think in principle any function or other thing that returns a state of None might be pickled.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@dhomeier dhomeier Sep 14, 2022 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And hopefully more will use slots in py3.10+ with dataclass(slots=True)!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could force it somehow to fall back to a.__dict__ of course

Comment thread astropy/tests/helper.py Outdated
@Cadair

Cadair commented Sep 14, 2022

Copy link
Copy Markdown
Member

Okaaay... it tries to build numpy from source too. I guess we cannot use Stuart's new way for wheels here.

If it is compiling numpy then there isn't a wheel for it so the alternative is that it errors.

@nstarman

Copy link
Copy Markdown
Member

@eerovaher @nstarman , the patch for pickling isn't compatible with the cosmology code in LTS. How can we fix this? Thanks!

Darn. The file astropy/cosmology/tests/test_pickle.py has been removed since. I can take a look in a bit.

@dhomeier

Copy link
Copy Markdown
Contributor

Darn. The file astropy/cosmology/tests/test_pickle.py has been removed since. I can take a look in a bit.

It is passing in https://github.com/astropy/astropy/actions/runs/3055240601/jobs/4928078581, isn't it?

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

@dhomeier , this test only exists in LTS branch, not sure why.

@nstarman , any advise on how to fix this properly since you were the main author for that test, if I remember correctly?

@dhomeier

Copy link
Copy Markdown
Contributor

@dhomeier , this test only exists in LTS branch, not sure why.

I am a bit confused now – this is the astropy/cosmology/tests/test_pickle.py that was previously failing here in
https://github.com/astropy/astropy/actions/runs/3054566720/jobs/4926621567 ?

The pickling tests seem to have moved elsewhere like into test_realizations.py, but the models apparently no longer involve function properties or other things that would raise on the None __getstate__().

Or is there another backport that is still failing?

@nstarman

nstarman commented Sep 15, 2022 •

Copy link
Copy Markdown
Member

Ok, taking a look... Everything's passing! That was easy 😆

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

It does but looks like a7190c4 fixed it? Is that good enough? I see there are still some discussions.

@dhomeier

Copy link
Copy Markdown
Contributor

It does but looks like a7190c4 fixed it? Is that good enough? I see there are still some discussions.

generic_recursive_equality_test does handle the cosmology pickle now; the discussion is just about yet more obscure cases it might need to handle.

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

Okay... Ping me when there is a consensus. Thanks!

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

test_biweight_midvariance_masked failure is unrelated.

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

Do you mind if I squash the commits?

@eerovaher

Copy link
Copy Markdown
Member

Good idea to squash, but updates made here to generic_recursive_equality_test() should be easily applicable to the other branches after squashing.

pllim and others added 3 commits September 15, 2022 12:33
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>
@pllim
pllim force-pushed the manual-backport-of-pr-13614-on-v5.0.x branch from 965269d to fd7d1f2 Compare September 15, 2022 16:35
@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

Okay, squashed. The forward port is at #13675

@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

Huh... @nstarman , do we really need exact match here? Can we use allclose?

_______________________ TestFlatwCDM.test_toformat_model _______________________

self = <astropy.cosmology.tests.test_flrw.TestFlatwCDM object at 0x7fcf997acb50>
cosmo = FlatwCDM(name="ABCMeta", H0=70 km / (Mpc s), Om0=0.27, w0=-1, Tcmb0=3 K, Neff=3.04, m_nu=[0. 0. 0.] eV, Ob0=None)
to_format = <astropy.cosmology.connect.CosmologyToFormat object at 0x7fcf78113ee0>
method_name = 'Ode'

    def test_toformat_model(self, cosmo, to_format, method_name):
        """Test cosmology -> astropy.model."""
        if method_name is None:  # no test if no method
            return
    
        model = to_format("astropy.model", method=method_name)
        assert isinstance(model, _CosmologyModel)
    
        # Parameters
        expect = tuple([n for n in cosmo.__parameters__ if getattr(cosmo, n) is not None])
        assert model.param_names == expect
    
        # scalar result
        args = np.arange(model.n_inputs) + 1
    
        got = model.evaluate(*args)
        expected = getattr(cosmo, method_name)(*args)
        assert np.all(got == expected)
    
        got = model(*args)
        expected = getattr(cosmo, method_name)(*args)
>       assert np.all(got == expected)
E       assert False
E        +  where False = <function all at 0x7fcfaf7971c0>(0.2523877726514899 == 0.25238777265148993)
E        +    where <function all at 0x7fcfaf7971c0> = np.all

.../cosmology/io/tests/test_model.py:119: AssertionError

@nstarman

Copy link
Copy Markdown
Member

All close is sufficient!

@pllim
pllim merged commit 73c13a7 into astropy:v5.0.x Sep 15, 2022
@pllim
pllim deleted the manual-backport-of-pr-13614-on-v5.0.x branch September 15, 2022 22:10
@pllim

pllim commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

CI finally green again. Thank you, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

io.ascii skip-basebranch-check Skip base branch check for direct backports skip-changelog-checks Tells bot to skip changlog checks testing time utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants