Skip to content

TST: Some fixes & refactoring around glibc-dependent skips in test_umath.py #20274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 16, 2021
Merged

TST: Some fixes & refactoring around glibc-dependent skips in test_umath.py #20274

merged 4 commits into from
Nov 16, 2021

Conversation

h-vetinari
Copy link
Contributor

@h-vetinari h-vetinari commented Nov 2, 2021 •
edited
Loading

I was looking at the test skips in the numpy feedstock by chance, and thought of fixing #15179, now that there's a glibc-version check that was introduced in #19209.

First step is taking that check and turning it into a reusable boolean. The second commit applies that also to a different glibc-relevant test (though, strictly speaking, it's left open in 8b2b7a2 whether this also works for 2.17 & 2.18 - I can parametrize the check of course, but not sure that's worth the effort?)

Finally, applying the fix suggested by @r-devulap (which has been tested in a conda-forge PR to work) conditionally on the glibc-version, so that the test in the numpy-feedstock can be switched on unconditionally.

Fixes #15179

@h-vetinari
Copy link
Contributor Author

The second commit applies that also to a different glibc-relevant test (though, strictly speaking, it's left open in 8b2b7a2 whether this also works for 2.17 & 2.18 - I can parametrize the check of course, but not sure that's worth the effort?)

I decided to fix this, because the correct inclusion/exclusion was actually off for the glibc-version in manylinux 2014.

@h-vetinari
Copy link
Contributor Author

Not sure what the timeline for 1.22 is, but it would be good if this PR could be looked at before that (and merged, ideally, as it would allow conda-forge to remove a test skip - which are hardly ever looked at and prone to bit rot).

CC @charris @seberg @rgommers

@seberg seberg added this to the 1.22.0 release milestone Nov 12, 2021
charris
charris reviewed Nov 15, 2021
View reviewed changes
numpy/core/tests/test_umath.py
glibc_newerthan_2_17 = pytest.mark.xfail(
glibcver != '0.0' and glibcver < '2.17',
reason="Older glibc versions may not raise appropriate FP exceptions")
glibc_older_than = lambda x: (glibcver != '0.0' and glibcver < x)

Copy link
Member

Choose a reason for hiding this comment

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

The use of alphabetical order works, but it bothers me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree, but then, it was this way before already, and I didn't want to overengineer something for 3 tests...

What would you prefer? Comparing tuples? Using something like distutils.version.LooseVersion?

charris
charris reviewed Nov 15, 2021
View reviewed changes
numpy/core/tests/test_umath.py
@@ -3439,13 +3433,14 @@ def check(x, rtol):
x_series = np.logspace(-20, -3.001, 200)
x_basic = np.logspace(-2.999, 0, 10, endpoint=False)

if dtype is np.longcomplex:
if glibc_older_than("2.19") and dtype is np.longcomplex:
Copy link
Member

Choose a reason for hiding this comment

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

"2.19" or "2.18"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what that would do? If it's older than 2.18, ít's definitely older than 2.19?

Copy link
Member

@charris charris Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I was looking at the older comment that it was OK for glibc > 2.17.

Copy link
Member

Choose a reason for hiding this comment

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

    skip_longcomplex_msg = ('Trig functions of np.longcomplex values known to be '
                            'inaccurate on aarch64 for some compilation '
                            'configurations, should be fixed by building on a '
                            'platform using glibc>2.17')

Copy link
Contributor Author

@h-vetinari h-vetinari Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For the if-branches here, the pertinent part (being deleted) is:

            # Can use 2.1 for > Ubuntu LTS Trusty (2014), glibc = 2.19.
            if skip_longcomplex_msg:
                pytest.skip(skip_longcomplex_msg)

I take from this comment that anything from glibc 2.19 onwards should be fine to take the else-clause (with the 2.1*eps). I cannot very (easily or quickly) how far back in terms of glibc-versions things would work, so I didn't try to change the version-boundaries as stated (or commented) in the code.

Copy link
Contributor Author

@h-vetinari h-vetinari Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

[...] should be fixed by building on a platform using glibc>2.17'

Right, I couldn't evaluate that corner case (2.18), so I went with the conservative choice, of still running the check (resp. the looser tolerance) with 2.18.

Before the last commit, I actually had this as glibc_older_than_2_17, but then realized that this would do the wrong thing (due to glibc > 2.17) for manylinux 2014.

Copy link
Member

@charris charris Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

OK, probably not worth worrying about. Manylinux2014 itself is getting towards end of life.

@h-vetinari
Copy link
Contributor Author

Thanks a lot for taking the time to review @charris! :)

@charris charris merged commit 444a721 into numpy:main Nov 16, 2021
@charris
Copy link
Member

charris commented Nov 16, 2021

Thanks @h-vetinari .

@h-vetinari h-vetinari deleted the fix_15179 branch November 16, 2021 01:38
lithomas1 added a commit to lithomas1/numpy that referenced this pull request Nov 18, 2021
@lithomas1
Add 64-bit Windows wheels [wheel build]
bf8d234 
commit 30228578ac65ec6b90961700a5783c23d8e1b879
Author: Thomas Li <47963215+lithomas1@users.noreply.github.com>
Date:   Wed Nov 17 16:21:27 2021 -0800

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Update LICENSE_win32.txt

    Update LICENSE_win32.txt

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Update LICENSE_win32.txt

    Update LICENSE_win32.txt

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Update LICENSE_win32.txt

    Update LICENSE_win32.txt

    Add Windows config to GHA

    update script [wheel build]

    typo [wheel build]

    fix typo? [wheel build]

    fix linux builds? [wheel build]

    typo [wheel build]

    add license and pin to windows 2016

    skip tests [wheel build]

    pin to windows 2019 instead [wheel build]

    try to find out the error on windows [wheel build]

    maybe fix? [wheel build]

    maybe fix? [wheel build]

    fix? [wheel build]

    cleanup [wheel build]

    Update LICENSE_win32.txt

    Update LICENSE_win32.txt

commit 4bd12df
Author: Thomas Li <47963215+lithomas1@users.noreply.github.com>
Date:   Mon Nov 15 17:28:47 2021 -0800

    # This is a combination of 14 commits.
    # This is the 1st commit message:

    Add Windows config to GHA
    # This is the commit message numpy#2:

    update script [wheel build]
    # This is the commit message numpy#3:

    typo [wheel build]
    # This is the commit message numpy#4:

    fix typo? [wheel build]
    # This is the commit message numpy#5:

    fix linux builds? [wheel build]
    # This is the commit message numpy#6:

    typo [wheel build]
    # This is the commit message numpy#7:

    add license and pin to windows 2016
    # This is the commit message numpy#8:

    skip tests [wheel build]
    # This is the commit message numpy#9:

    pin to windows 2019 instead [wheel build]
    # This is the commit message numpy#10:

    try to find out the error on windows [wheel build]
    # This is the commit message numpy#11:

    maybe fix? [wheel build]
    # This is the commit message numpy#12:

    maybe fix? [wheel build]
    # This is the commit message numpy#13:

    fix? [wheel build]
    # This is the commit message numpy#14:

    cleanup [wheel build]

commit 444a721
Merge: 376ad69 22448b4
Author: Charles Harris <charlesr.harris@gmail.com>
Date:   Mon Nov 15 17:47:23 2021 -0700

    Merge pull request numpy#20274 from h-vetinari/fix_15179

    TST: Some fixes & refactoring around glibc-dependent skips in test_umath.py

commit 376ad69
Merge: b75fe57 546c47a
Author: Charles Harris <charlesr.harris@gmail.com>
Date:   Mon Nov 15 17:31:41 2021 -0700

    Merge pull request numpy#20327 from seberg/rename-interpolation

    BUG,DEP: Fixup quantile/percentile and rename interpolation->method

commit 546c47a
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 15 16:13:50 2021 -0600

    DOC: Fixups for interpolation rename comments from review

    Co-authored-by: Charles Harris <charlesr.harris@gmail.com>

commit b75fe57
Merge: 7310c09 cbc25d2
Author: Warren Weckesser <warren.weckesser@gmail.com>
Date:   Mon Nov 15 17:27:08 2021 -0500

    Merge pull request numpy#20369 from bilderbuchi/missing_diagnostics_newlines

    MAINT: Fix newlines in diagnostics output of numpy.f2py.

commit cbc25d2
Author: Christoph Buchner <bilderbuchi@phononoia.at>
Date:   Sun Nov 14 08:36:03 2021 +0100

    MAINT: Fix newlines in diagnostics output of numpy.f2py.

    Linebreaks were not consistently added to errmess/outmess
    arguments, which led to very long lines and wrong
    concatenation with compiler messages in f2py console output.

commit be15716
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Fri Nov 12 12:10:20 2021 -0600

    DOC: Add release not for quantile `interpolation` rename

    Also updates the old release note to include the info (and generally
    tweaks it a bit)

commit 7d8a8e7
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Fri Nov 12 11:57:22 2021 -0600

    DOC: Update percentile/quantile docs

    Mainly fixes the method list slightly, tones down the warning a
    bit and fixes the link to the paper (I did not realize that the
    link failed to work due only because the reference was missing
    from nanquantile/nanpercentile).

commit 5bd71fb
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Tue Nov 9 09:48:59 2021 -0600

    DOC: Add ticks to quantile interpolation/method error

    Co-authored-by: abel <aoun@cerfacs.fr>

commit 0d5fb81
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 20:39:50 2021 -0600

    DOC: Remove reference to paper from quantile `method` kwarg

    Apparently, sphinx does not resolve references to footnotes from
    parameter descriptions.

commit 8437663
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 18:25:37 2021 -0600

    MAINT: Rename interpolation to method in percentile stubs

commit 8cfb6b5
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 16:47:27 2021 -0600

    TST: Add deprecation testcase for quantile interpolation rename

commit a5ac5a5
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 16:41:24 2021 -0600

    DOC: Fixup the percentile methods plot

commit 85f3dda
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 16:37:41 2021 -0600

    BUG: quantile discrete methods ended up using -1 as index sometimes

    Also, the closest-observation did not correctly support multiple
    quantiles calculated at the same time (broadcasting error).

commit 3993408
Author: Sebastian Berg <sebastian@sipsolutions.net>
Date:   Mon Nov 8 15:38:30 2021 -0600

    API,DEP: Rename percentile/quantile `interpolation=` to `method=`

commit 22448b4
Author: H. Vetinari <h.vetinari@gmx.com>
Date:   Tue Nov 2 16:51:59 2021 +1100

    TST: parametrize glibc-version check in test_umath.py

commit 12923c2
Author: H. Vetinari <h.vetinari@gmx.com>
Date:   Tue Nov 2 14:21:01 2021 +1100

    TST: skip coverage of large elements in sincos_float32 for ancient glibc

    Fixes numpy#15179

commit 56268d5
Author: H. Vetinari <h.vetinari@gmx.com>
Date:   Tue Nov 2 14:19:24 2021 +1100

    TST: use existence of glibc-version to clean up a test

commit 01443e8
Author: H. Vetinari <h.vetinari@gmx.com>
Date:   Tue Nov 2 14:18:52 2021 +1100

    TST: turn glibc_older_than_2.17 into boolean instead of xfail

    this anticipates reuse of this boolean within the test module
@charris charris mentioned this pull request Nov 21, 2021
Closed
charris added a commit to charris/numpy that referenced this pull request Nov 21, 2021
@charris
MAINT: Fix longdouble precision check.
a5d002e 
This is a partial reversion of numpy#20274. My guess is that
`glibc_older_than("2.19")` is not working correctly on manylinux2014
on aarch64.

Closes numpy#20426.
@charris charris mentioned this pull request Nov 21, 2021
Merged
charris added a commit to charris/numpy that referenced this pull request Nov 21, 2021
@charris
MAINT: Fix longdouble precision check.
5a4ecc0 
This is a partial reversion of numpy#20274. My guess is that
`glibc_older_than("2.19")` is not working correctly on manylinux2014
on aarch64.

Closes numpy#20426.
@charris charris mentioned this pull request Nov 21, 2021
Merged
h-vetinari added a commit to h-vetinari/numpy-feedstock that referenced this pull request Nov 23, 2021
@h-vetinari
remove test skip that was taken care of in numpy/numpy#20274
04251c9
This was referenced Nov 24, 2021
Closed
Closed
h-vetinari added a commit to h-vetinari/numpy-feedstock that referenced this pull request Dec 10, 2021
@h-vetinari
remove test skip that was taken care of in numpy/numpy#20274
e882ad9
h-vetinari added a commit to h-vetinari/numpy-feedstock that referenced this pull request Dec 21, 2021
@h-vetinari
remove test skip that was taken care of in numpy/numpy#20274
db87f6f
h-vetinari added a commit to h-vetinari/numpy-feedstock that referenced this pull request Dec 21, 2021
@h-vetinari
remove test skip that was taken care of in numpy/numpy#20274
71529a0
h-vetinari added a commit to regro-cf-autotick-bot/numpy-feedstock that referenced this pull request Jan 1, 2022
@h-vetinari
remove some obsolete skips
c0c8d23 
- test_sincos_float32 taken care of in numpy/numpy#20274
- TestF77Mismatch removed upstream in numpy/numpy#20457
- test_unary_PyUFunc_O_O_method_full fixed in 1.22 (numpy/numpy#19926)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charris charris charris left review comments

Assignees
No one assigned
Labels
05 - Testing
Projects
None yet
Milestone
1.22.0 release
Development

Successfully merging this pull request may close these issues.

TestAVXFloat32Transcendental test failure
3 participants
@h-vetinari @charris @seberg