TST, CI: add ARM64 Graviton 2 to CI

[tylerjereddy]

• start testing MDAnalysis on ARM64 using
  new AWS Graviton 2 architecture available
  in Travis CI

• testing on my fork shows only two failures
  in the MDAnalysis test suite using a fairly
  minimal set of dependencies; we can probably
  either fix those or skip them and open an issue

• the test failures are:

1. test_written_remarks_property
2. TestEncoreClustering.test_clustering_three_ensembles_two_identical

• run time for this CI entry is about
  23 minutes, which is solid for ARM; we save a lot
  of time using experimental upstream wheels in some
  cases and excluding (source builds of) dependencies
  where possible until the ARM64 binary wheel ecosystem
  matures a bit more

[codecov]

Codecov Report

Merging #2956 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2956   +/-   ##
========================================
  Coverage    93.05%   93.05%           
========================================
  Files          186      186           
  Lines        24611    24611           
  Branches      3188     3188           
========================================
  Hits         22902    22902           
  Misses        1661     1661           
  Partials        48       48           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beb5232...1deaff1. Read the comment docs.

[tylerjereddy]

Looks like subtle behavior differences in MDAnalysis/analysis/encore/clustering/src/ap.c on the platforms of the only test that really matters here (the other is already known to be a issue on other plaforms so we can probably xfail the remarks one on arm64 with the hypothesis case problems there).

Looking at the first few lines of a diff command on the debug output from that C file when running the test in the absence of scikit-learn on both platforms:

-6.07   -5.59   -5.17   -4.75   -4.33   -3.97   -3.56   -3.24 | -6.07   -5.59   -5.17   -4.75   -4.33   -3.97   -3.56   -3.24
tmp -0.20                                                     | tmp -0.21
tmp -0.20                                                     | tmp -0.21
-0.54   -0.46   -0.96   -1.42   -1.90   -2.33   -2.75   -3.14 | -0.54   -0.46   -0.96   -1.42   -1.90   -2.33   -2.75   -3.14
0.46    -0.46   -0.96   -1.42   -1.90   -2.33   -2.75   -3.14 | 0.46    -0.46   -0.96   -1.42   -1.90   -2.33   -2.75   -3.14
tmp -4.44                                                     | tmp -4.45
tmp -4.44                                                     | tmp -4.45

[tylerjereddy]

A bunch of float -> double changes in that source file allows the test to pass on an ARM64 gcc compile farm node; I'll see if I can reduce the size of the diff/changes.

[tylerjereddy]

Perhaps also useful to know if we really need float for "performance." Using double is often preferable anyway. Perhaps I could add a platform-conditional remap in the worst case.

[pep8speaks]

Hello @tylerjereddy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/setup.py:

Line 268:80: E501 line too long (81 > 79 characters)
Line 271:80: E501 line too long (81 > 79 characters)

• In the file testsuite/MDAnalysisTests/formats/test_libdcd.py:

Line 357:21: E127 continuation line over-indented for visual indent

Comment last updated at 2020-10-18 23:56:41 UTC

[tylerjereddy]

We'll see if this passes ARM64 CI now..

[tylerjereddy]

this doesn't actually fail on the gcc 115 gcc compile farm ARM64 machine, but the test is clearly already flaky on some other platforms--maybe depends on character encoding/support stuff?

[tylerjereddy]

hmm, I still need to iterate more it seems; the partial switch to double was harmful for another encore test

[tylerjereddy]

If I use gcc 4.x on ARM64 the failure is different than the 9.x series version in use in CI (and on the compile farm machine):

E       AssertionError: Unexpected result: <ClusterCollection with 42 clusters>
E       assert 42 == 40
E        +  where 42 = len(<ClusterCollection with 42 clusters>)

The "bad" value changes to 36 with gcc 9.x.

[tylerjereddy]

Selectively reducing compiler optimization level to -O1 for ARM64 machines allows the encore tests to pass in my hands. We'll see if the CI agrees.

[tylerjereddy]

@lilyminium @richardjgowers Can you live with the ARM64 shims here? CI is green now; the new job still runs in ~23 minutes.

I think we'll be able to bump up the optimization level a bit---O1 was my second try, but -O2 might be possible. We can always make that improvement later though.

[IAlibay]

It's interesting that the tests still run in a reasonably short amount of time even at O1. Even though they output incorrect values, are they substantially faster at 02/03?

It would be good to eventually work out if we can isolate the opt flag that's causing this. If anything it would help us understand how/if the code can be rewritten in a safer manner.

[tylerjereddy]

are they substantially faster at 02/03?

The test suite as a whole in CI is about the same runtime at -O1 on ARM64. You can see by comparing the time I reported for the ARM64 job when I first opened the PR at -O3 vs. current run time after realizing that aggressive optimization was causing problems.

[tylerjereddy]

Some performance loss in the testsuite may also be made up by reduced build time with optimizations turned down. In any case, I think correctness and portability trump the hunger for performance here, especially since I've isolated it to a single architecture.

Maybe open an issue to perfect things on ARM64 eventually, but that will be a separate task I think--the build/iteration times on ARM64 can still be slow on some of the openly available machines, and we're really only using a small subset of dependencies until that ecosystem matures anyway.

[richardjgowers]

Yeah this looks like a good step towards having Arm compatibility, thanks @tylerjereddy !

[IAlibay]

Thanks for all the work here @tylerjereddy Is the only thing that needs changing before we claim "minimal ARM support" the use of the scipy dev wheels? (mainly thinking as to whether or not a CHANGELOG entry is warranted)

[tylerjereddy]

Yeah, I released SciPy 1.5.3 over the weekend with "official" Linux ARM wheels so I'll try to clean that up in a follow-up