Fix arch option in setup.py to work properly + not trigger on MSVC

[hmacdope]

Fixes #3428

Changes made in this Pull Request:

• fixes removes march option in setup.cfg
• checks for MSVC (WIP??)
• only allows if not on MSVC
• allow any additional compiler flags to be used for build excluding encore

Questions:

• Do we want to check if the flag march is accepted rather than using the compiler identity? E.g what about icc, pgi etc etc
• Do we want to improve documentation of this option?
• Do we want to add this option to CI? My guess is probably not?

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-01 09:15:14 UTC

[hmacdope]

Tagging @IAlibay for CI question. :)

Also @tylerjereddy, how is the MSVC check done upstream in numpy/scipy? I couldn't see if there was a consistent and compact way it is done.

General question: test for compiler identity or flag support?

[codecov]

Codecov Report

Merging #3429 (32f10ad) into develop (9b37cd6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3429   +/-   ##
========================================
  Coverage    93.77%   93.77%           
========================================
  Files          176      176           
  Lines        23219    23219           
  Branches      3308     3308           
========================================
  Hits         21773    21773           
  Misses        1395     1395           
  Partials        51       51           

---

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 9b37cd6...32f10ad. Read the comment docs.

[tylerjereddy]

Also @tylerjereddy, how is the MSVC check done upstream in numpy/scipy? I couldn't see if there was a consistent and compact way it is done.

SciPy is on the verge of switching to meson for builds, but digging through the code a bit I see stuff like this:

That seems to be a mixture of both approaches (looking for compiler by name and also using a try.. style function to conservatively add a flag after.

NumPy seems to have bunch of stuff--after all, it provides the numpy.distutils build system proper. This one stands out for being clean and simple and seems to be used in some prominent places (NumPy core itself and the build for random, and both are used tons of course):

That appears to be in response to problems with mingw compiling issues on Windows. Part of me wonders if you could use one compiler to build your Python interpreter and another one to build MDAnalysis (just trying to figure out why one wouldn't be able to use that), but if it used in those crucial places in NumPy I suppose it is likely to be robust.

[IAlibay]

So I know the march option existed, albeit undocumented & broken, but I do have to query its need / existence:

1. Our tests are quite flaky on -O3 already, do we have any sense on if they'd pass with more advanced instructions
2. Would using it actually provide a noticeable benefit to runtime execution speed? My understanding is that the majority of our costs were "things moving in memory".

The latter is pretty important, if there's no benefits then maybe we should kill off the option rather than try to fix it?

[hmacdope]

Fair enough to question whether worth fixing.

AFAIK The use of instructions shouldn't change the level of accuracy on most things. AFAIK no standard breaking or unsafe floating point arithmetic is allowed by AVX or SSE or whatever.

Let me do some tests on my work computer RE speed improvements and get back to you.

[hmacdope]

Thanks @tylerjereddy and @IAlibay :)

[hmacdope]

@tylerjereddy @IAlibay Apologies for screenshot rather than graph, but appears as though performance gains are significant, esp for distances when you can use AVX/AVX2. This mirrors what I found last time and also what @richardjgowers and I have found over at distopia, that the compiler autovectorisation is a significant improvement.

Here we can see that some of the distance calculations are almost 3X faster with march=native enabled, run on my AVX2 capable workstation.

With that in mind I would probably advocate for continued life of the march option but would of course value any opinion for @MDAnalysis/coredevs and others.

[IAlibay]

Do all the tests still pass?

[hmacdope]

Only a single Encore failure on Ubuntu MacOS and Win appears fine (see most recent CI). Tests are flakier on my local attempts but can try and get to bottom of that.

[hmacdope]

Was a problem with my test env, all tests seem to pass. :)

[hmacdope]

@IAlibay thanks for the heads up, removing march=native from the encore flags fixed the only failure.

[IAlibay]

@IAlibay thanks for the heads up, removing march=native from the encore flags fixed the only failure.

I suspect even with this you would still trigger failures on aarch64 and ppc64le, but we run CI for those on a weekly cron job. As long as you don't end up doing a lot of commits, if you want to temporarily change the travis yaml file to remove the cron job limitation, I think it should trigger a build.

[hmacdope]

Thats a great idea @IAlibay, I will try and find the right thingo and confirm with you. Once enable I won't add any more commits until people have had a chance to discuss.

[hmacdope]

ARM = OK

but flags have different meaning, prefers mcpu=native see: https://community.arm.com/developer/tools-software/tools/b/tools-software-ides-blog/posts/compiler-flags-across-architectures-march-mtune-and-mcpu.

PowerPC=no build

needs "mcpu=native mtune=native" instead

[hmacdope]

Fixed by adding x86 and arm specific paths only.

[hmacdope]

Did we reach a consensus on supporting this option? 3x speedup for distances is quite attractive (to me). Key will be documenting an "advanced" install option, which I am happy to do, just need to know either way.

[richardjgowers]

Definitely writing a guide on how to achieve this with a manual install is an easy win.

…

On Fri, Oct 15, 2021 at 06:59, Hugo MacDermott-Opeskin < ***@***.***> wrote: Did we reach a consensus on supporting this option? 3x speedup for distances is quite attractive (to me). Key will be documenting an "advanced" install option, which I am happy to do, just need to know either way. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3429 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB5FZAEIOWVGV62KGETUG6YJ5ANCNFSM5E3XIAHA> .

[hmacdope]

I will add an example to the user guide, and update the installation wiki page post merge.

[hmacdope]

See MDAnalysis/UserGuide#177 for docs. Also note that changes to CI and actual enabling of the march=native option will be removed, but are here for now to show CI passing.

[IAlibay]

See MDAnalysis/UserGuide#177 for docs. Also note that changes to CI and actual enabling of the march=native option will be removed, but are here for now to show CI passing.

Can you raise an issue about this? I'll eventually add it to our weekly cron job at some point.

[IAlibay]

Couple of comments

[IAlibay]

two small things and this should be done

[IAlibay]

There's something wrong with codecov at the moment but I'm not seeing what anything in either actions or codecov's status updates.. I restarted the runners we'll see what happens

[IAlibay]

Assuming CI returns green lgtm, thanks @hmacdope !