Skip to content

CI: Add new workflow/action for testing universal intrinsics on armv7#20206

Merged
mattip merged 2 commits intonumpy:mainfrom
seiko2plus:ci_armv7_simd_test
Oct 28, 2021
Merged

CI: Add new workflow/action for testing universal intrinsics on armv7#20206
mattip merged 2 commits intonumpy:mainfrom
seiko2plus:ci_armv7_simd_test

Conversation

@seiko2plus
Copy link
Copy Markdown
Member

@seiko2plus seiko2plus commented Oct 27, 2021

The new action uses qemu to emulate a full container of armv7/ubuntu on x86_64.

To speed up the compilation a bridge has been made between the host and the
emulated container to ship x86_64 binaries of the GCC cross-compiler to
be executed natively during the build.

@seiko2plus seiko2plus changed the title CI: Add new test for armv7 to test universal intrinsics CI: Add new action for armv7 to test universal intrinsics Oct 27, 2021
@seiko2plus seiko2plus force-pushed the ci_armv7_simd_test branch 12 times, most recently from 60d60b6 to e2a0023 Compare October 27, 2021 11:44
@seiko2plus seiko2plus marked this pull request as ready for review October 27, 2021 11:46
@seiko2plus seiko2plus changed the title CI: Add new action for armv7 to test universal intrinsics CI: Add new workflow/action for testing universal intrinsics on armv7 Oct 27, 2021
@seiko2plus seiko2plus requested a review from mattip October 27, 2021 12:54
Comment thread .github/workflows/build_test.yml Outdated
g++ --version &&
pip3 --version &&
python3 --version &&
cd /numpy && pip3 install -v ./
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Suggested change
cd /numpy && pip3 install -v ./
cd /numpy && python3 setup.py install

I wonder if there's a way to filter pip3 log to force printing only the build log!.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using bare pip3 is dangerous when there may be several Pythons installed, python -mpip is safer.

Copy link
Copy Markdown
Member Author

@seiko2plus seiko2plus Oct 27, 2021

Choose a reason for hiding this comment

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

I agree, but this armv7 container is guaranteed to contain only one version of python3. However, with or without wouldn't reduce the output of pip3. It seems there's no way to filter the verbose mode, I just want to keep pip3/python3 -mpip since it was part of #19820 investigation.

Copy link
Copy Markdown
Member Author

@seiko2plus seiko2plus Oct 27, 2021

Choose a reason for hiding this comment

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

would it be fine to just use python3 setup.py install instead? it seems to me that building NumPy via pip3 works just fine on armv7. just one bug was discovered related to static casting affected by #19713

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would it be fine to just use python3 setup.py install

I think these days the recommendation is python3 -m pip install .. That should be future-proof even if we change the build system. As of now, of course, it ends up calling setup.py underneath.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

alright, I stuck to python3 setup.py install for exposing the build log only since the main goal behind this test is to check the sanity of universal intrinsics.

@mattip
Copy link
Copy Markdown
Member

mattip commented Oct 27, 2021

Nice. The runs I saw take about 18 minutes, which is around the time of the pypy38 run. The longest CI run is the "full" one at 27 minutes. It seems all the CI runs in parallel so this is not slowing down the overall CI time.

@mattip
Copy link
Copy Markdown
Member

mattip commented Oct 27, 2021

Would it be worthwhile to use the system blas libraries via apt?

  The new action uses qemu to emulate a full container of armv7/ubuntu on x86_64.
  To speed up the compilation a bridge has been made between the host and the
  emulated container to ship x86_64 binaries of the GCC cross-compiler to
  be executed natively during the build.
  These errors/warnings occurs when sizeof(long double) == sizeof(double).
@seiko2plus
Copy link
Copy Markdown
Member Author

Would it be worthwhile to use the system blas libraries via apt?

I'm not sure if that would make any change, keep in mind only SIMD tests are running at the current moment, we can expand it later to cover other tests such as umath but qemu is extremely slow and not reliable. the build is kinda fast since the compiler is running natively(mixing between x86_64, armhf binaries).

@mattip
Copy link
Copy Markdown
Member

mattip commented Oct 28, 2021

Thanks @seiko2plus. I missed that this is only running the SIMD tests, which makes sense.

@seiko2plus seiko2plus linked an issue Oct 28, 2021 that may be closed by this pull request
@seiko2plus seiko2plus deleted the ci_armv7_simd_test branch November 5, 2021 16:56
typedef long double npy_longdouble;
#define NPY_LONGDOUBLE_FMT "Lg"
#endif
typedef long double npy_longdouble;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can I revisit this one? @carlkl pointed out that this is a rather significant change in behavior. Previously, for platforms and compilers where double == long double, at Numpy compile time, such as MSVC, this would give npy_longdouble as double. This is useful in the case when we are compiling using Numpy, but with a compiler where it is not true that double == long double. One example is compiling Scipy with mingw-w64, where, by default long double is Float80. Could y'all say more about why this change is needed? Why do we need to force npy_longdouble to be the compiler long double, rather than Numpy long double?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I think this was my mistake in not looking carefully enough at the PR. I agree we should back out this change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this change was needed to pass the c++ build when double == long double, since undefined cast causes compiling errors on C++, also remove the massive cast warnings with C sources.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this pr also linked with #20210

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I completely believe that this change fixed the C++ build, but this is a backwards-incompatible change in the Numpy include headers used by all projects compiling against Numpy, so I do think we need to devote some serious thought to whether this is the right way to fix the C++ build.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously, for platforms and compilers where double == long double, at Numpy compile time, such as MSVC, this would give npy_longdouble as double. This is useful in the case when we are compiling using Numpy, but with a compiler where it is not true that double == long double.

Can we spell the broken workflow(s) out a bit more (sorry, this stuff is confusing)? Is the broken case "build NumPy with MSVC, then SciPy with Mingw"? And does "build both NumPy and SciPy with Mingw" still work?

Copy link
Copy Markdown
Member

@carlkl carlkl Nov 10, 2021

Choose a reason for hiding this comment

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

What we need is an additional branch like that:

#if defined __MINGW32__ && __LDBL_DIG__ != __DBL_DIG__
  typedef double npy_longdouble;
#else
  typedef long double npy_longdouble;
#endif

explanation: works in case of using the mingw-w64 toolchain regardless if gcc is used with an additional -mlong-double-64 switch or not. This would also correctly work for the the Msys2 toolchains, where long double is defined as extended precision (FLOAT80).
EDIT: this has to be put inside the
#if NPY_SIZEOF_LONGDOUBLE == NPY_SIZEOF_DOUBLE
branch of course:

#if NPY_SIZEOF_LONGDOUBLE == NPY_SIZEOF_DOUBLE
#if defined __MINGW32__ && __LDBL_DIG__ != __DBL_DIG__
        typedef double npy_longdouble;
#else
        typedef long double npy_longdouble;
#endif
        #define NPY_LONGDOUBLE_FMT "g"
#else
        typedef long double npy_longdouble;
        #define NPY_LONGDOUBLE_FMT "Lg"
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that it fixed the universal2 build, we can't just back out this change. It'd be great to figure out what the right thing to do is though here, because it would be great to not release 1.22.0 with something that's going to prevent building SciPy with Mingw.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, @seberg opened a fresh issue for this one, maybe best to continue there.

Copy link
Copy Markdown
Contributor

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

A worry about the Numpy include change here.

@seiko2plus
Copy link
Copy Markdown
Member Author

seiko2plus commented Nov 10, 2021

@matthew-brett,

A worry about the Numpy include change here.

this change was needed to pass this workflow, so I thought at that time it was fine to include it here.

@rgommers rgommers added the component: SIMD Issues in SIMD (fast instruction sets) code or machinery label Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

05 - Testing 36 - Build Build related PR component: SIMD Issues in SIMD (fast instruction sets) code or machinery

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Build failure for macos universal2 wheels

6 participants