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.

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

Closed
wants to merge 10 commits into from

Conversation

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

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

Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca>
@pllim
Copy link
Member Author
pllim commented Jan 4, 2022

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
Added back in original assert, ignoring only for Gaussian2D.
Co-authored-by: William Jamieson <wjamieson@stsci.edu>
@dstansby
Copy link
Contributor
dstansby commented Jan 4, 2022

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

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.

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

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
Co-authored-by: David Stansby <dstansby@gmail.com>
@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
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
Development

Successfully merging this pull request may close these issues.

3 participants
0