MNT: Compatibility with Python 3.11 by Cadair · Pull Request #13614 · astropy/astropy

Cadair · 2022-09-08T16:49:53Z

github-actions · 2022-09-08T16:50:43Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

mhvk · 2022-09-08T20:29:33Z

Good to try, though scipy is not installing, so I guess this needs a bit more time!?

Cadair · 2022-09-09T11:13:37Z

Ok, after building scipy from source I have got actual errors in our test suite that are presumably down to Python 3.11:

 FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/coordinates/tests/test_pickle.py::test_simple_object[0-StaticMatrixTransform-args6-kwargs6-False]
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/coordinates/tests/test_pickle.py::test_simple_object[1-StaticMatrixTransform-args6-kwargs6-False]
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/coordinates/tests/test_pickle.py::test_simple_object[-1-StaticMatrixTransform-args6-kwargs6-False]
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_base.py::TestFLRW::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_lambdacdm.py::TestLambdaCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_lambdacdm.py::TestFlatLambdaCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_w0cdm.py::TestwCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_w0cdm.py::TestFlatwCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_w0wacdm.py::Testw0waCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_w0wacdm.py::TestFlatw0waCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_w0wzcdm.py::Testw0wzCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/flrw/tests/test_wpwazpcdm.py::TestwpwaCDM::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/cosmology/tests/test_core.py::TestCosmology::test_name
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/table/tests/test_pprint.py::TestColumnsShowHide::test_remove[pprint_exclude_names]
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/table/tests/test_pprint.py::TestColumnsShowHide::test_remove[pprint_include_names]
FAILED ../../.tox/py311-test-devdeps/lib/python3.11/site-packages/astropy/time/tests/test_mask.py::test_mask_not_writeable

So that's errors in coordinates @eteq @adrn @eerovaher, time @mhvk @taldcroft, cosmology @astropy/cosmology and table

pllim · 2022-09-09T13:26:10Z

astropy/coordinates/tests/test_pickle.py

>       dict_a = a.__getstate__() if hasattr(a, '__getstate__') else a.__dict__
E       TypeError: unbound method object.__getstate__() needs an argument

https://docs.python.org/3.11/library/pickle.html#object.getstate

Changed in version 3.11: Added the default implementation of the __getstate__() method in the object class.

cosmology

Maybe just a matter of changing the expected error message for Python 3.11? Or maybe this is a symptom of something else, not sure.

        with pytest.raises(AttributeError, match="can't set"):
>           cosmo.name = None
E           AttributeError: property 'name' of 'SubFLRW' object has no setter

table/tests/test_pprint.py

Looks like del is broken?

        t = self.t
        t_hide_show = getattr(t, attr)
        t_hide_show.set(['a', 'b'])
        del t['a']
>       assert t_hide_show() == ('b',)
E       AssertionError: assert ('a', 'b') == ('b',)
E         At index 0 diff: 'a' != 'b'
E         Left contains one more item: 'b'
E         Full diff:
E         - ('b',)
E         + ('a', 'b')

astropy/time/tests/test_mask.py

The error message changed. Maybe same problem as cosmology above.

        t = Time('2000:001')
        with pytest.raises(AttributeError) as err:
            t.mask = True
>       assert "can't set attribute" in str(err.value)
E       assert "can't set attribute" in "property 'mask' of 'Time' object has no setter"

nstarman · 2022-09-09T15:36:36Z

cosmology

Maybe just a matter of changing the expected error message for Python 3.11? Or maybe this is a symptom of something else, not sure.
        with pytest.raises(AttributeError, match="can't set"):
>           cosmo.name = None
E           AttributeError: property 'name' of 'SubFLRW' object has no setter

@pllim. That appears to be the case. property appears to have changed its error message.

Cadair · 2022-09-09T15:37:59Z

I am very happy for people to push to this PR if we want to use it as the place to address all these issues.

mhvk · 2022-09-09T21:01:50Z

@nstarman - if you have a chance to fix the cosmology error message change, could you do the time one too?

nstarman · 2022-09-09T21:02:46Z

@nstarman - if you have a chance to fix the cosmology error message change, could you do the time one too?

Cadair#24

dhomeier · 2022-09-10T00:47:18Z

Maybe just a matter of changing the expected error message for Python 3.11? Or maybe this is a symptom of something else, not sure.

No, seems to be all: python/cpython#31311

So in principle they could be all dealt with by defining a custom _no_setter error string depending on sys.version_info >= (3, 11) or not. Is that something that could go into astropy.tests.helper?

dhomeier · 2022-09-10T00:56:54Z

https://docs.python.org/3.11/library/pickle.html#object.__getstate__

Changed in version 3.11: Added the default implementation of the __getstate__() method in the object class.

So previously

astropy/astropy/tests/helper.py

Line 412 in 5b6619a

dict_a = a.__getstate__() if hasattr(a, '__getstate__') else a.__dict__

was simply falling back to a.__dict__.
Should it check now if a is a class, rather than an instance thereof (how?) and then call either a().__getstate__() or a.__getstate__(a)?

taldcroft · 2022-09-11T12:53:03Z

See #13637 for discussion of the test_pprint problem.

pllim · 2022-09-11T15:51:00Z

Not sure what is going on. I don't see the Python 3.11 job run after merging Nathan's fix.

pep8speaks · 2022-09-12T10:40:19Z

Hello @Cadair 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2022-09-13 14:14:26 UTC

pllim · 2022-09-12T13:36:41Z

Less failures now.

The time warning message just needs to be updated like cosmology.
The scipy deprecation warning is unrelated to Python 3.11.

So, the remaining compatibility issue we need to figure out is the __getstate__ usage in astropy/coordinates/tests/test_pickle.py. @adrn ?

adrn · 2022-09-12T13:55:30Z

It looks like the coordinates failure actually comes from calling astropy.tests.helper.check_pickling_recovery(), see the first line of:

astropy/astropy/tests/helper.py

Line 412 in f3ce7b2

dict_a = a.__getstate__() if hasattr(a, '__getstate__') else a.__dict__

I don't have time to look at this today but that is where I would start.

dhomeier · 2022-09-12T13:57:01Z

It looks like the coordinates failure actually comes from calling astropy.tests.helper.check_pickling_recovery(), see the first line of:

Yes, see #13614 (comment).
Just not sure about the best way to treat classes vs. class instances here.

nstarman · 2022-09-12T13:58:49Z

Should it check now if a is a class, rather than an instance thereof (how?) and then call either a().__getstate__() or a.__getstate__(a)?

https://docs.python.org/3.8/library/inspect.html#inspect.isclass ? Is this what you mean?

dhomeier · 2022-09-12T14:08:27Z

Should it check now if a is a class, rather than an instance thereof (how?) and then call either a().__getstate__() or a.__getstate__(a)?

https://docs.python.org/3.8/library/inspect.html#inspect.isclass ? Is this what you mean?

Yes, thanks! As there is no general info on the correct initialisation signature for a(), this should probably fork to a.__getstate__(a).

Cadair · 2022-09-12T14:14:46Z

I just rebased this to clear up the history. More pushes or PRs to main are welcome.

eerovaher · 2022-09-12T15:36:08Z

+    if hasattr(a, '__getstate__'):
+        dict_a = a.__getstate__(a) if inspect.isclass(a) else a.__getstate__()
+    else:
+        dict_a = a.__dict__


It would be good to add a comment here to clarify that the hasattr check is only needed for Python < 3.11.

For class inputs. I don't know if the function might ever be called with some other a input that does not implement __getstate__. Could use some comment though – in theory I guess this could also be called in Python < 3.11 for some class defining their own custom __getstate__.

If Python 3.11 implemented __getstate__() in object then hasattr(a, "__getstate__") will be true for any a. The check is therefore only needed until 3.11 is the minimum supported Python version. A comment would record that it's safe to simplify once support for older versions of Python is dropped.

some class defining their own custom __getstate__

Though obviously defining one without parameters would break stuff in 3.11+...

OK, I misremembered, thought it was only implemented for class.
But in that case it is certainly more likely to get an a with a different __getstate__ signature – e.g. in the original example, astropy.coordinates.builtin_frames.ICRS.__getstate__ has one required argument, while astropy.coordinates.builtin_frames.ICRS().__getstate__ takes no arguments.

You mean for using some auto-refactoring tool?

More like grep heh heh

Well, grep should find the comment as well; humans might not ;-)

Oh, well, if we do it Eero's way, the code will actually error when we remove that flag from compat, so maybe that is better.

I pushed a commit that would hopefully settle this debate once and for all.

dhomeier · 2022-09-12T15:39:44Z

This must be a new one for scipy-dev

>       warnings.warn(msg, category=DeprecationWarning,
E       DeprecationWarning: scipy.misc.ascent has been deprecated in SciPy v1.10.0; and will be completely removed in SciPy v1.12.0. Dataset methods have moved into the scipy.datasets module. Use scipy.datasets.ascent instead.

Just as a note for the final setup: this is obviously not Python 3.11 with minimal dependencies using py311-test-devdeps.

Cadair · 2022-09-12T15:52:15Z

Yeah it definitely isn't, I was just merrily hacking away at the CI to get it down to one job.

Co-authored-by: Derek Homeier <dhomeie@gwdg.de>

Cadair · 2022-09-14T12:39:16Z

I tired to group the commits into the changes, hopefully the commits all make sense by themselves now.

pllim

LGTM! What a beautiful collaboration. Thank you, everyone!

Stuart, thanks for getting rid of my "ooops" commit message. 😆

pllim · 2022-09-14T13:41:05Z

Of course... autobackport would be too easy...

pllim · 2022-09-14T15:39:39Z

I will do the manual backports, FYI.

Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)

pllim · 2022-09-14T16:24:13Z

-    devdeps,mpldev: git+https://github.com/matplotlib/matplotlib.git#egg=matplotlib
+    devdeps: scipy>=0.0.dev0
+    devdeps: numpy>=0.0.dev0
+    devdeps,mpldev: matplotlib>=0.0.dev0


@Cadair , this picked up matplotlib 3.6.0rc2 , not a dev version. Is this supposed to happen?

hmmm where?

https://github.com/astropy/astropy/actions/runs/3053254195/jobs/4923644273 (the job on main after merging this)

…5.1.x Backport PR #13614 on branch v5.1.x (MNT: Compatibility with Python 3.11)

Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)

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

dhomeier · 2022-11-16T17:56:28Z

Does anyone know what the scipy==1.10.0.dev0 installed by the new devdeps resolves to? Unlike e.g. numpy>=0.0.dev0 this just installs under that label, which however is unknown to my pip, so how to figure out which commit or tag the tests are run against?

pllim · 2022-11-16T18:17:35Z

The test header should say. This is from the last successful run on main:

2022-11-16T07:15:38.6203833Z Package versions: 
2022-11-16T07:15:38.6204055Z Numpy: 1.24.0.dev0+1068.g6aacc5167
2022-11-16T07:15:38.6204634Z Scipy: 1.10.0.dev0+2188.58e90e3

dhomeier · 2022-11-16T18:47:41Z

Thanks! Looked too high up, which was apparently still the header from pip.

github-actions Bot added the testing label Sep 8, 2022

pllim mentioned this pull request Sep 9, 2022

Test failures with Python 3.11 #13522

Closed

taldcroft mentioned this pull request Sep 11, 2022

Decide how to handle Python 3.11 table test failure #13637

Closed

taldcroft mentioned this pull request Sep 12, 2022

Fix Python 3.11 issue for pprint_include/exclude_names #13639

Merged

11 tasks done

Cadair force-pushed the 311_testing branch from 6930df3 to e00f53c Compare September 12, 2022 10:40

Cadair force-pushed the 311_testing branch from 47347d0 to 5ae31e2 Compare September 12, 2022 14:13

eerovaher reviewed Sep 12, 2022

View reviewed changes

pllim mentioned this pull request Sep 12, 2022

MNT: Compatibility with Python 3.11 #13649

Closed

7 of 11 tasks

eerovaher reviewed Sep 12, 2022

View reviewed changes

Comment thread astropy/time/tests/test_mask.py Outdated

Cadair and others added 2 commits September 14, 2022 13:37

Use Python 3.11 RC2 and small clean up of workflow/tox.

c8474ce

Co-authored-by: Derek Homeier <dhomeie@gwdg.de>

Update io.ascii BaseOutputter to CVE-2020-10735 fix

26bbaca

Cadair force-pushed the 311_testing branch from 5fb8c1f to 985880f Compare September 14, 2022 12:38

pllim approved these changes Sep 14, 2022

View reviewed changes

dhomeier reviewed Sep 14, 2022

View reviewed changes

Comment thread docs/changes/13614.other.rst Outdated

Add change log

990925c

Cadair force-pushed the 311_testing branch from 8df140c to 990925c Compare September 14, 2022 13:01

Cadair added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Sep 14, 2022

pllim merged commit 616f382 into astropy:main Sep 14, 2022

This comment was marked as resolved.

Sign in to view

lumberbot-app Bot added the Still Needs Manual Backport label Sep 14, 2022

This comment was marked as resolved.

Sign in to view

Cadair deleted the 311_testing branch September 14, 2022 13:39

pllim added a commit to pllim/astropy that referenced this pull request Sep 14, 2022

Backport PR astropy#13614: MNT: Compatibility with Python 3.11

5950c94

Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)

pllim mentioned this pull request Sep 14, 2022

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

Merged

11 tasks done

pllim added a commit to pllim/astropy that referenced this pull request Sep 14, 2022

Backport PR astropy#13614: MNT: Compatibility with Python 3.11

26d4da6

Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)

pllim mentioned this pull request Sep 14, 2022

Backport PR #13614 on branch v5.1.x (MNT: Compatibility with Python 3.11) #13665

Merged

11 tasks done

pllim removed the Still Needs Manual Backport label Sep 14, 2022

pllim reviewed Sep 14, 2022

View reviewed changes

pllim mentioned this pull request Sep 14, 2022

TST: devdeps should always install dev, not RC #13666

Closed

pllim added a commit that referenced this pull request Sep 15, 2022

Merge pull request #13665 from pllim/manual-backport-of-pr-13614-on-v…

6565ad9

…5.1.x Backport PR #13614 on branch v5.1.x (MNT: Compatibility with Python 3.11)

pllim added a commit to pllim/astropy that referenced this pull request Sep 15, 2022

Backport PR astropy#13614: MNT: Compatibility with Python 3.11

bb0946d

Merge pull request astropy#13614 from Cadair/311_testing MNT: Compatibility with Python 3.11 (cherry picked from commit 616f382)

pllim mentioned this pull request Sep 15, 2022

BUG: Fix logic in generic_recursive_equality_test #13675

Merged

10 tasks done

pllim added a commit that referenced this pull request Sep 15, 2022

Merge pull request #13664 from pllim/manual-backport-of-pr-13614-on-v…

73c13a7

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

Conversation

Cadair commented Sep 8, 2022 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 8, 2022

Uh oh!

mhvk commented Sep 8, 2022

Uh oh!

Cadair commented Sep 9, 2022

Uh oh!

pllim commented Sep 9, 2022 • edited by dhomeier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

astropy/coordinates/tests/test_pickle.py

cosmology

table/tests/test_pprint.py

astropy/time/tests/test_mask.py

Uh oh!

nstarman commented Sep 9, 2022

cosmology

Uh oh!

Cadair commented Sep 9, 2022

Uh oh!

mhvk commented Sep 9, 2022

Uh oh!

nstarman commented Sep 9, 2022

Uh oh!

dhomeier commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhomeier commented Sep 10, 2022

Uh oh!

taldcroft commented Sep 11, 2022

Uh oh!

pllim commented Sep 11, 2022

Uh oh!

pep8speaks commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-09-13 14:14:26 UTC

Uh oh!

pllim commented Sep 12, 2022

Uh oh!

adrn commented Sep 12, 2022

Uh oh!

dhomeier commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhomeier commented Sep 12, 2022

Uh oh!

Cadair commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhomeier Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhomeier commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cadair commented Sep 12, 2022

Uh oh!

Cadair commented Sep 8, 2022 •

edited by pllim

Loading

pllim commented Sep 9, 2022 •

edited by dhomeier

Loading

dhomeier commented Sep 10, 2022 •

edited

Loading

pep8speaks commented Sep 12, 2022 •

edited

Loading

dhomeier commented Sep 12, 2022 •

edited

Loading

nstarman commented Sep 12, 2022 •

edited

Loading

dhomeier Sep 12, 2022 •

edited

Loading

dhomeier commented Sep 12, 2022 •

edited

Loading