Fix warnings with Python 3.10 #11962

saimn · 2021-07-20T07:00:23Z

Closes #11821, tests run locally, not sure when we will be able to add a job on Actions.

Fix the deprecation warnings and ignore new RuntimeWarning raised in convolution, lombscargle, modeling and units. We should probably address those by either fixing the code or catching them in tests but there are many of them (except for units, only 1 IIRC). So probably to address those when we have a 3.10 job.

What should be the milestone, probably 4.3 ?

Description

This pull request is to address ...

Fixes #

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?
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.
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 · 2021-07-20T14:12:25Z

astropy/convolution/convolve.py

+    with warnings.catch_warnings():
+        # distutils.sysconfig module is deprecated in Python 3.10
+        warnings.simplefilter('ignore', DeprecationWarning)
+        _convolve = load_library("_convolve", LIBRARY_PATH)


This fix does not look like it is going to work when the deprecated feature is removed. Is there no replacement for this?

Numpy has some plan : numpy/numpy#18588
But here we are using a smaller helper function that relies on distutils to get the shared library extension on different platforms. We could replace this with some custom code I guess.

So, instead of merging this, which would completely mask the real problem, should we wait a bit and see? Not like we can test against Python 3.10 in CI right now anyway...

I don't think they will fix this in a few weeks, and we/they have time until 3.12
Actions seems to support 3.10b4 now so I'm trying to add a job.

Okay, as long as we open follow-up issue about this, I guess I can live with this for now.

Yes, hopefully this can be an "upstream fix required"!

pllim · 2021-07-20T14:13:10Z

astropy/utils/introspection.py

+    with warnings.catch_warnings():
+        # distutils package is deprecated in Python 3.10
+        warnings.simplefilter('ignore', DeprecationWarning)
+        from distutils.version import LooseVersion


Oh no... How do we replace this?

You should know : #11714 ;)

Yes, it seems time to get back to that one!!

pllim · 2021-07-20T14:13:47Z

setup.cfg

+    # RuntimeWarning from convolution, lombscargle, modeling
+    ignore:invalid value encountered in true_divide:RuntimeWarning
+    # RuntimeWarning from units, lombscargle
+    ignore:divide by zero encountered in:RuntimeWarning


I am not comfortable with blanket ignore like this. They should be ignored on a case-by-case basis.

Yes, but it's more work, and at least for lombscargle I have no idea how to fix it.

I still have a bad feeling about this. This will ignore such warnings for the entire repo, not just the affected subpackages.

saimn · 2021-07-20T16:47:41Z

Just as a remainder, Python 3.10's release schedule:

    3.10.0 beta 4: Saturday, 2021-07-10

Expected:

    3.10.0 candidate 1: Monday, 2021-08-02
    3.10.0 candidate 2: Monday, 2021-09-06 (if necessary)
    3.10.0 final: Monday, 2021-10-04

saimn · 2021-07-20T17:05:37Z

For the record, convolution has 573 failed tests, I think all because of the same warning:

>               normalized_kernel = kernel / kernel_scale
E               RuntimeWarning: invalid value encountered in true_divide

astropy/convolution/convolve.py:691: RuntimeWarning

where kernel_scale is (1+0j).

modeling has only one failing test, also because of a division by 0j.

lombscargle fails on something like numerator / denominator where numerator is complex and denominator is real, so again division by 0j but not the same warning ...

saimn · 2021-07-20T17:06:30Z

And the 3.10 job passes 🎉

pllim · 2021-07-20T17:16:16Z

Re: #11962 (comment) -- Can we localize the warning ignore to the places you have identified instead?

pllim · 2021-07-20T17:38:17Z

setup.cfg

+    ignore:divide by zero encountered in true_divide:RuntimeWarning:astropy.timeseries.periodograms.lombscargle
+    ignore:divide by zero encountered in true_divide:RuntimeWarning:astropy.units.quantity
+    ignore:invalid value encountered in true_divide:RuntimeWarning:astropy.convolution.convolve
+    ignore:invalid value encountered in true_divide:RuntimeWarning:astropy.modeling.functional_models


Okay, I guess this is a good compromise. 😸 (And need a follow-up issue.)

pllim · 2021-07-20T17:39:14Z

.github/workflows/ci_workflows.yml

@@ -58,6 +58,11 @@ jobs:
            python: 3.9
            toxenv: py39-test

+          - name: Python 3.10 with minimal dependencies


Originally when I attempted this, I put this in devdeps job.

I also wonder if "minimal dependencies" is enough to catch everything...

Also would need a follow up issue to test with proper Python 3.10 when it is released for real.

Yes we'll need to update for rc1 and the final release.
I didn't try with all other dependencies because I'm afraid this will fail somewhere. But it's fine for now, I mean the goal here is to test that we are compatible with 3.10, not to test that other dependencies also are.

Long term I think it's worth bundling this with dev dependencies, especially if they provide a nightly build with dev/rc python.

mhvk

Not bad, actually! Overall, looks good. I'm surprised by the warning on the one quantity test, though...

mhvk · 2021-07-20T19:12:26Z

astropy/units/core.py

@@ -473,7 +473,7 @@ def set_enabled_equivalencies(equivalencies):
        >>> from astropy import units as u
        >>> with u.set_enabled_equivalencies(u.dimensionless_angles()):
        ...     phase = 0.5 * u.cycle
-        ...     np.exp(1j*phase)  # doctest: +FLOAT_CMP
+        ...     np.exp(1j*phase)  # doctest: +FLOAT_CMP +IGNORE_WARNINGS


Hmm, I'd be interested to know what the actual warning was. I cannot find this in #11821. Is there perhaps a log I can look at? (I've yet to install a beta version of python; if that's easy, I'll do that!)

Same problem as all others failures, dividing a complex by a scalar now raises a warning because of the division by 0j:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 465 list of equivalent pairs, e.g., as returned by 466 `~astropy.units.equivalencies.dimensionless_angles`. 467 468 Examples 469 -------- 470 Exponentiation normally requires dimensionless quantities. To avoid 471 problems with complex phases:: 472 473 >>> from astropy import units as u 474 >>> with u.set_enabled_equivalencies(u.dimensionless_angles()): UNEXPECTED EXCEPTION: RuntimeWarning('divide by zero encountered in cdouble_scalars') Traceback (most recent call last): File "/home/sconseil/.pyenv/versions/3.10.0b4/lib/python3.10/doctest.py", line 1348, in __run exec(compile(example.source, filename, "single", File "<doctest astropy.units.core.set_enabled_equivalencies[1]>", line 3, in <module> File "/home/sconseil/dev/astropy/astropy/astropy/units/quantity.py", line 504, in __array_ufunc__ arrays.append(converter(input_) if converter else input_) File "/home/sconseil/dev/astropy/astropy/astropy/units/core.py", line 988, in convert return func(_condition_arg(v) / scale1) * scale2 RuntimeWarning: divide by zero encountered in cdouble_scalars /home/sconseil/dev/astropy/astropy/astropy/units/core.py:474: UnexpectedException >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > /home/sconseil/dev/astropy/astropy/astropy/units/core.py(988)convert() -> return func(_condition_arg(v) / scale1) * scale2 (Pdb) _condition_arg(v) 0.5j (Pdb) scale1 0.15915494309189535 (Pdb) _condition_arg(v) / scale1 *** RuntimeWarning: divide by zero encountered in cdouble_scalars

Is this python 3.10 specific? This is completely unphysical, so I'd want to raise a bug report...

mhvk · 2021-07-20T19:13:08Z

astropy/utils/introspection.py

+    with warnings.catch_warnings():
+        # distutils package is deprecated in Python 3.10
+        warnings.simplefilter('ignore', DeprecationWarning)
+        from distutils.version import LooseVersion


Yes, it seems time to get back to that one!!

mhvk · 2021-07-20T19:14:12Z

astropy/convolution/convolve.py

+    with warnings.catch_warnings():
+        # distutils.sysconfig module is deprecated in Python 3.10
+        warnings.simplefilter('ignore', DeprecationWarning)
+        _convolve = load_library("_convolve", LIBRARY_PATH)


Yes, hopefully this can be an "upstream fix required"!

pllim

LGTM though we need to open several follow-up issues.

I won't merge yet in case subpackage maintainers want to review all the warnings in modeling (@nden , @perrygreenfield), timeseries (@astrofrog, @bsipocz), units (@adrn, @mhvk), and convolution (@larrybradley, @adonath, @keflavich).

bsipocz · 2021-07-20T19:51:59Z

.github/workflows/ci_workflows.yml

@@ -58,6 +58,11 @@ jobs:
            python: 3.9
            toxenv: py39-test

+          - name: Python 3.10 with minimal dependencies


Long term I think it's worth bundling this with dev dependencies, especially if they provide a nightly build with dev/rc python.

saimn · 2021-07-20T20:03:39Z

To sum up, all warnings are caused by some kind of division of a complex by a scalar:

>>> np.complex128(1j) / 2
<stdin>:1: RuntimeWarning: divide by zero encountered in cdouble_scalars
0.5j

>>> np.array([1j]) / np.array([2])
<stdin>:1: RuntimeWarning: divide by zero encountered in true_divide
array([0.+0.5j])

>>> np.array([1 + 0j]) / 1+0j
<stdin>:1: RuntimeWarning: invalid value encountered in true_divide
array([1.+0.j])

mhvk · 2021-07-20T20:49:33Z

Weird, so it is a numpy problem, but one that only comes up in python 3.10? I just installed python 3.10 via debian experimental, and indeed cannot reproduce that error with just 0.5j/2., so it is not directly a python problem. Unfortunately, I cannot seem to use this installation with venv or virtualenv, so it is not much use for further testing... But I did check that I do not get your warnings on numpy-dev/python 3.9.

One question: perhaps we should try with numpy-dev here as well? They seem to have made quite a few fixes for python 3.10 (though nothing that looks really relevant...). If this also gives problems with numpy-dev, they really should know about this...

saimn · 2021-07-21T12:59:06Z

Yes, it only happens on Python 3.10 and with Numpy complex values, not with Python ones. Maybe we can try with Numpy's nightly.

pllim · 2021-07-21T13:26:23Z

ERROR: Could not find a version that satisfies the requirement numpy (from versions: none)

Looks like Numpy dev not compatible with Python 3.10? 🤷

saimn · 2021-07-21T13:29:52Z

No, probably they do not build the nightly wheels for 3.10 yet.

pllim · 2021-07-21T15:20:36Z

Hmm passed on Python 3.10 and numpy-dev, it seems. But that is with the ignore still in place, right?

saimn · 2021-07-21T15:30:38Z

I commented the filters in the previous commit, so yeah it seems to be OK with numpy dev !
Maybe I revert the two last commits, and we remove the filters when the next version of numpy is out ?

pllim · 2021-07-21T15:34:33Z

Or... we just stick Python 3.10 into devdeps job and be done with it?

saimn · 2021-07-21T15:41:45Z

Installing Numpy from the git repo makes the job longer, compiling Numpy. It's an option but we can also wait for Numpy to publish nightly wheels for 3.10.

pllim · 2021-09-09T16:25:06Z

@saimn , there is conflict now. FYI

Fix issues with test_logger and io.fits tests.

saimn · 2021-09-09T19:08:49Z

All green now :)

pllim

All the cron stuff are broken for different and unrelated reasons, so no point trying to run them here. Let's get this in. Thanks!

lumberbot-app · 2021-09-09T19:52:15Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v4.3.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 23404eeb2caeea315cea89b6cb8b9b2fee4f13be

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #11962: Fix warnings with Python 3.10'

Push to a named branch :

git push YOURFORK v4.3.x:auto-backport-of-pr-11962-on-v4.3.x

Create a PR against branch v4.3.x, I would have named this PR:

"Backport PR #11962 on branch v4.3.x (Fix warnings with Python 3.10)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

pllim · 2021-09-09T19:55:20Z

ops... Do we need to backport?

saimn · 2021-09-09T20:17:59Z

Having a 4.3.x compatible with Python 3.10 would be nice (it will out in ~1 month). I will look at the manual backport later.

vstinner · 2021-09-09T20:19:35Z

Thanks for the fix @saimn!

saimn added the 💤 backport-v4.3.x on-merge: backport to v4.3.x label Jul 20, 2021

saimn added this to the v4.3 milestone Jul 20, 2021

github-actions bot added convolution installation io.fits testing utils labels Jul 20, 2021

saimn mentioned this pull request Jul 20, 2021

Test faillures with python 3.10.0a6 #11821

Closed

pllim added the Python3.10 label Jul 20, 2021

pllim reviewed Jul 20, 2021

View reviewed changes

saimn force-pushed the 3.10 branch from e07b456 to a373303 Compare July 20, 2021 16:52

pllim mentioned this pull request Jul 20, 2021

TST: Python 3.10 #11561

Closed

2 tasks

pllim reviewed Jul 20, 2021

View reviewed changes

mhvk reviewed Jul 20, 2021

View reviewed changes

pllim approved these changes Jul 20, 2021

View reviewed changes

bsipocz approved these changes Jul 20, 2021

View reviewed changes

saimn mentioned this pull request Sep 8, 2021

ENH: Use importlib in minversion #11714

Merged

saimn added 12 commits September 9, 2021 19:10

Ignore distutils deprecation warnings

bc5bbdf

Fix use of deprecated threading functions

f14535f

Fix issues with test_logger and io.fits tests.

Fix use of deprecated find_module

d926c98

Ignore new RuntimeWarning

47c826c

Add changelog entry

aeca621

Add job with 3.10b4

19c6bc5

Forgot tox config

9443050

Use more specific filters

0e37461

Run with Numpy dev

45bc3e1

Update to rc1

a0b5e32

Numpy now has wheels for 3.10rc1

7d3645f

Use 3.10 stable when available

ef022da

saimn force-pushed the 3.10 branch from d21f3bb to ef022da Compare September 9, 2021 17:11

pllim approved these changes Sep 9, 2021

View reviewed changes

pllim merged commit 23404ee into astropy:main Sep 9, 2021

lumberbot-app bot added the Still Needs Manual Backport label Sep 9, 2021

pllim mentioned this pull request Sep 9, 2021

Remove warning catching in convolve.py (replace numpy.ctypeslib.load_library) #12157

Closed

saimn deleted the 3.10 branch September 9, 2021 20:16

saimn pushed a commit to saimn/astropy that referenced this pull request Sep 10, 2021

Backport PR astropy#11962: Fix warnings with Python 3.10

1627ddd

saimn mentioned this pull request Sep 10, 2021

Backport PR #11962: Fix warnings with Python 3.10 #12159

Closed

9 tasks

saimn pushed a commit to saimn/astropy that referenced this pull request Oct 14, 2021

Backport PR astropy#11962: Fix warnings with Python 3.10

dea513d

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

Revert outdated change from astropy#11962

d88b802

saimn removed the Still Needs Manual Backport label Apr 27, 2022

Fix warnings with Python 3.10 #11962

Fix warnings with Python 3.10 #11962

Conversation

saimn commented Jul 20, 2021 • edited by pllim

Description

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saimn commented Jul 20, 2021

saimn commented Jul 20, 2021

saimn commented Jul 20, 2021

pllim commented Jul 20, 2021

pllim Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saimn commented Jul 20, 2021

mhvk commented Jul 20, 2021

saimn commented Jul 21, 2021

pllim commented Jul 21, 2021

saimn commented Jul 21, 2021

pllim commented Jul 21, 2021

saimn commented Jul 21, 2021

pllim commented Jul 21, 2021

saimn commented Jul 21, 2021

pllim commented Sep 9, 2021

saimn commented Sep 9, 2021

pllim left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Sep 9, 2021

pllim commented Sep 9, 2021

saimn commented Sep 9, 2021

vstinner commented Sep 9, 2021

saimn commented Jul 20, 2021 •

edited by pllim

pllim Jul 20, 2021 •

edited