Skip to content
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

BLD: improve M1 build support #3595

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

tylerjereddy
Copy link
Member

Deals with part of #3592

PR Checklist

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

* the changes here allow me to build MDAnalysis and its
dependencies on an M1 Mac on the gcc compile farm using
`python3 -m pip install --user .`

* the full test suite passes, except for 1 extra `xfail`
I've added in here--which is also documented by gh-1389
(it has apparently been failing on "exotic platforms"
for almost 5 years)
@pep8speaks
Copy link

pep8speaks commented Apr 2, 2022

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

Line 448:80: E501 line too long (87 > 79 characters)

Comment last updated at 2022-04-03 18:27:39 UTC

@@ -444,6 +445,8 @@ def test_clustering_two_ensembles(self, ens1, ens2):
assert len(cluster_collection) == expected_value, "Unexpected " \
"results: {0}".format(cluster_collection)

@pytest.mark.xfail(platform.machine() == "arm64" and platform.system() == "Darwin",
reason="Fails on M1 Mac")
Copy link
Member Author

Choose a reason for hiding this comment

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

could also reference the GH issue proper here, I just didn't search for it until after I opened the PR

Copy link
Member

Choose a reason for hiding this comment

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

If possible that'd be great.

@IAlibay
Copy link
Member

IAlibay commented Apr 2, 2022

(it has apparently been failing on "exotic platforms" for almost 5 years)

It's not been an issue on ppc64le or aarch64 (non-M1) though, it might be good to supplement that original issue with a new one, it's not immediately obvious to me why this is failing, is encore_compile_args being set properly (to -O1 / equivalent) for M1 builds?

if platform.machine() == 'aarch64' or platform.machine() == 'ppc64le':

@codecov
Copy link

codecov bot commented Apr 2, 2022

Codecov Report

Merging #3595 (92073b0) into develop (bf95f9c) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    MDAnalysis/mdanalysis#3595      +/-   ##
===========================================
+ Coverage    94.20%   94.21%   +0.01%     
===========================================
  Files          190      190              
  Lines        24668    24725      +57     
  Branches      3313     3321       +8     
===========================================
+ Hits         23238    23295      +57     
+ Misses        1384     1382       -2     
- Partials        46       48       +2     
Impacted Files Coverage Δ
package/MDAnalysis/topology/guessers.py 99.21% <0.00%> (-0.02%) ⬇️
package/MDAnalysis/core/selection.py 98.80% <0.00%> (ø)
package/MDAnalysis/converters/RDKit.py 98.42% <0.00%> (+0.35%) ⬆️

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 bf95f9c...92073b0. Read the comment docs.

@tylerjereddy
Copy link
Member Author

is encore_compile_args being set properly (to -O1 / equivalent) for M1 builds?

I tried that--it actually made the situation worse on the M1. I can post what happens if you do that in an issue later maybe.

@IAlibay
Copy link
Member

IAlibay commented Apr 3, 2022

is encore_compile_args being set properly (to -O1 / equivalent) for M1 builds?

I tried that--it actually made the situation worse on the M1. I can post what happens if you do that in an issue later maybe.

How very odd. Yeah if you could either open a new issue or add to the old one that'd be great. Otherwise I'm happy to merge this.

* explicit reference the issue related to M1 Mac
`xfail` for `test_clustering_three_ensembles_two_identical()`
@tylerjereddy
Copy link
Member Author

Ok, I added a more detailed description of the M1 situation for this test in MDAnalysis/mdaencore#28, and revised this PR to reference that issue instead of vaguely referring to a test failure on M1.

@IAlibay
Copy link
Member

IAlibay commented Apr 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@IAlibay IAlibay merged commit 88628a1 into MDAnalysis:develop Apr 8, 2022
@tylerjereddy tylerjereddy deleted the treddy_issue_3592_m1_mac branch April 8, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants