CI: Add new workflow/action for testing universal intrinsics on armv7#20206
CI: Add new workflow/action for testing universal intrinsics on armv7#20206mattip merged 2 commits intonumpy:mainfrom
Conversation
60d60b6 to
e2a0023
Compare
e2a0023 to
9872f76
Compare
| g++ --version && | ||
| pip3 --version && | ||
| python3 --version && | ||
| cd /numpy && pip3 install -v ./ |
There was a problem hiding this comment.
| 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!.
There was a problem hiding this comment.
Using bare pip3 is dangerous when there may be several Pythons installed, python -mpip is safer.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
Would it be worthwhile to use the system blas libraries via |
9872f76 to
b545183
Compare
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).
b545183 to
f559951
Compare
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 |
|
Thanks @seiko2plus. I missed that this is only running the SIMD tests, which makes sense. |
| typedef long double npy_longdouble; | ||
| #define NPY_LONGDOUBLE_FMT "Lg" | ||
| #endif | ||
| typedef long double npy_longdouble; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sorry, I think this was my mistake in not looking carefully enough at the PR. I agree we should back out this change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Previously, for platforms and compilers where
double == long double, at Numpy compile time, such as MSVC, this would givenpy_longdoubleasdouble. This is useful in the case when we are compiling using Numpy, but with a compiler where it is not true thatdouble == 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?
There was a problem hiding this comment.
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;
#endifexplanation: 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"
#endifThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, @seberg opened a fresh issue for this one, maybe best to continue there.
matthew-brett
left a comment
There was a problem hiding this comment.
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. |
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.