8000 TST: Use NPY_DISABLE_CPU_FEATURES for numpy 1.22 by pllim · Pull Request #12684 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

TST: Use NPY_DISABLE_CPU_FEATURES for numpy 1.22 #12684

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
Closed
wants to merge 10 commits into from
Closed

TST: Use NPY_DISABLE_CPU_FEATURES for numpy 1.22 #12684

wants to merge 10 commits into from

Conversation

pllim
Copy link
Member
@pllim pllim commented Jan 4, 2022 •
edited
Loading

Description

This pull request is to disable numpy avx512 instructions in CI.

  • Open follow-up issues.

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. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • 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 no-changelog-entry-needed Extra CI Run cron CI as part of PR 💤 backport-v5.0.x on-merge: backport to v5.0.x labels Jan 4, 2022
@pllim pllim added this to the v5.0.1 milestone Jan 4, 2022
@github-actions
Copy link
Contributor
github-actions bot commented Jan 4, 2022

👋 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?

@pllim pllim mentioned this pull request Jan 4, 2022
Merged
10 tasks
@WilliamJamieson WilliamJamieson mentioned this pull request Jan 4, 2022
Merged
@pllim
Copy link
Member Author
pllim commented Jan 4, 2022

@mhvk , do you know what is going on here in pyinstaller? If not, I guess we still have to pin numpy for the pyinstaller job.

  File "astropy/units/quantity_helper/function_helpers.py", line 42, in <module>
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "PyInstaller/loader/pyimod03_importers.py", line 476, in exec_module
  File "numpy/lib/recfunctions.py", line 16, in <module>
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "PyInstaller/loader/pyimod03_importers.py", line 476, in exec_module
  File "numpy/testing/__init__.py", line 17, in <module>
NameError: name '_private' is not defined

@pllim
Copy link
Member Author
pllim commented Jan 4, 2022

Actually, looking at https://github.com/astropy/astropy/runs/4704960655?check_suite_focus=true , I am not sure if this env var is doing anything for us. @dstansby , what do you think?

@pllim @mhvk
TST: Relax tolerance in coordinate tests.
6d0dc0b 
Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca>
@pllim
Copy link
Member Author
pllim commented Jan 4, 2022 •
edited
Loading

Looks like maybe the env var did render the skipping of test_copy_vla unnecessary? 🤷

WilliamJamieson and others added 2 commits January 4, 2022 13:38
@WilliamJamieson @pllim
Made test more robust.
e23cf1c 
Added back in original assert, ignoring only for Gaussian2D.
@pllim @WilliamJamieson
TST: Unrelax model deriv comparison.
d7937e5 
Co-authored-by: William Jamieson <wjamieson@stsci.edu>
@dstansby
Copy link
Contributor
dstansby commented Jan 4, 2022 •
edited
Loading

There's a line in the failing test that says:

:228: RuntimeWarning: During parsing environment variable 'NPY_DISABLE_CPU_FEATURES':
You cannot disable CPU features ("AVX512F AVX512VL AVX512BW AVX512DQ AVX512_SKX"), since they are not part of the dispatched optimizations

I don't know exactly what this means, but disabling doesn't seem to have an effect.

@dstansby
Copy link
Contributor
dstansby commented Jan 4, 2022

This actually seems to be happening on all builds, with the extra line below that I missed in my previous comment:

(SSSE3 SSE41 POPCNT SSE42 AVX F16C FMA3 AVX2 AVX512F AVX512CD AVX512_KNL AVX512_KNM AVX512_SKX AVX512_CLX AVX512_CNL AVX512_ICL).

I'm guessing this is the list of available CPU features, so maybe try disabling all the ones in this list that start with AVX512? I have no idea why this is different to the list we disabled in MPL though...

dstansby
dstansby reviewed Jan 4, 2022
View reviewed changes
Copy link
Contributor
@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

From looking at the numpy PR (numpy/numpy#19478), I think this is the only feature that needs disabling.

WilliamJamieson
WilliamJamieson reviewed Jan 4, 2022
View reviewed changes
@pllim @dstansby @WilliamJamieson
Disable avx512 properly, relax modeling test a little
3719f30 
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: William Jamieson <wjamieson@stsci.edu>
pllim added a commit to pllim/astropy that referenced this pull request Jan 4, 2022
@pllim
EXP: Minimal fix version of astropy#12684
2de7848
@pllim pllim mentioned this pull request Jan 4, 2022
Closed
9 tasks
@pllim
Copy link
Member Author
pllim commented Jan 4, 2022 
    <frozen importlib._bootstrap>:241: RuntimeWarning: During parsing environment variable 'NPY_DISABLE_CPU_FEATURES':
    You cannot disable CPU features ("AVX512_SKX"), since they are not part of the dispatched optimizations
    (SSSE3 SSE41 POPCNT SSE42 AVX F16C FMA3 AVX2 AVX512F AVX512CD AVX512_KNL AVX512_KNM AVX512_SKX AVX512_CLX AVX512_CNL AVX512_ICL).

This makes no sense to me. 🤯

dstansby
dstansby reviewed Jan 4, 2022
View reviewed changes
Copy link
Contributor
@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Perhaps the quote marks are being included in the environment variable?

pllim and others added 2 commits January 4, 2022 15:50
@pllim 8000 @dstansby
Remove quotes from env var
6761a40 
Co-authored-by: David Stansby <dstansby@gmail.com>
@pllim
Skip test_copy_vla for now
fbbd9a5
@pllim
Copy link
Member Author
pllim commented Jan 4, 2022

Hmm. Looks like we don't have to touch most of the tests if we go with #12685 . What do you think?

@WilliamJamieson , do you think your modeling patch is still worth going in despite numpy issues?

@WilliamJamieson
Copy link
Contributor

@plim I suggest that we go with #12685 and add part of my modeling patch in a separate PR (without the statement which excludes the Gaussian2D model). This way we can isolate future failures to "equivalent" parameters rather than broken model fits.

@pllim pllim closed this in #12682 Jan 5, 2022
@pllim pllim deleted the and-we-will-all-float-on-okay branch January 5, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstansby dstansby dstansby left review comments

@WilliamJamieson WilliamJamieson WilliamJamieson left review comments

@saimn saimn Awaiting requested review from saimn saimn will be requested when the pull request is marked ready for review saimn is a code owner

Assignees
No one assigned
Labels
Extra CI Run cron CI as part of PR no-changelog-entry-needed testing 💤 backport-v5.0.x on-merge: backport to v5.0.x
Projects
None yet
Milestone
v5.0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@pllim @dstansby @WilliamJamieson
0