Skip to content

WIP, CI: add newer msvc to Azure CI#3399

Merged
IAlibay merged 5 commits intoMDAnalysis:developfrom
tylerjereddy:treddy_azure_latest_win
Jan 21, 2022
Merged

WIP, CI: add newer msvc to Azure CI#3399
IAlibay merged 5 commits intoMDAnalysis:developfrom
tylerjereddy:treddy_azure_latest_win

Conversation

@tylerjereddy
Copy link
Member

PR Checklist

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

@codecov
Copy link

codecov bot commented Aug 21, 2021

Codecov Report

Merging #3399 (78f1786) into develop (3322ab2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3399   +/-   ##
========================================
  Coverage    94.06%   94.06%           
========================================
  Files          176      176           
  Lines        23282    23282           
  Branches      3302     3302           
========================================
  Hits         21901    21901           
  Misses        1335     1335           
  Partials        46       46           

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 3322ab2...78f1786. Read the comment docs.

@tylerjereddy tylerjereddy changed the title CI: add newer msvc to Azure CI WIP, CI: add newer msvc to Azure CI Aug 21, 2021
@tylerjereddy
Copy link
Member Author

ah, still using an older msvc (C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.2702); I'll try to mess with it a bit on my fork maybe

@tylerjereddy tylerjereddy force-pushed the treddy_azure_latest_win branch from 17289d7 to 2d7425b Compare August 22, 2021 00:08
@IAlibay
Copy link
Member

IAlibay commented Aug 22, 2021

Yup, it's picking up the augment error. How very interesting, we could try to lower the optimization to 01 for windows? Although that's kinda annoying given it's probably a test that relies on too accurate a float precision rather than an actual issue...

@tylerjereddy
Copy link
Member Author

@IAlibay there we go--I got MSVC 2019 running for one Azure matrix entry and the two failures you mention are reproduced.

The raw log is here, and I've pasted the two tracebacks below the fold since the logs can purge out after 10 days.

Details
2021-08-22T00:24:56.9133409Z ================================== FAILURES ===================================
2021-08-22T00:24:56.9134389Z __________________________ test_augment[q5-res5-b0] ___________________________
2021-08-22T00:24:56.9135050Z [gw1] win32 -- Python 3.8.10 C:\hostedtoolcache\windows\Python\3.8.10\x64\python.exe
2021-08-22T00:24:56.9135407Z 
2021-08-22T00:24:56.9135916Z b = array([10., 10., 10., 90., 90., 90.], dtype=float32)
2021-08-22T00:24:56.9136393Z q = array([[1., 1., 1.]], dtype=float32)
2021-08-22T00:24:56.9137007Z res = [[1.1, 0.1, 0.1], [0.1, 1.1, 0.1], [0.1, 0.1, 1.1], [0.1, 1.1, 1.1], [1.1, 1.1, 0.1], [1.1, 0.1, 1.1], ...]
2021-08-22T00:24:56.9137390Z 
2021-08-22T00:24:56.9137877Z     @pytest.mark.parametrize('b', (
2021-08-22T00:24:56.9138501Z                              np.array([10, 10, 10, 90, 90, 90], dtype=np.float32),
2021-08-22T00:24:56.9139125Z                              np.array([10, 10, 10, 45, 60, 90], dtype=np.float32)
2021-08-22T00:24:56.9139611Z                              ))
2021-08-22T00:24:56.9140117Z     @pytest.mark.parametrize('q, res', qres)
2021-08-22T00:24:56.9140636Z     def test_augment(b, q, res):
2021-08-22T00:24:56.9141067Z         radius = 1.5
2021-08-22T00:24:56.9141575Z         q = transform_StoR(np.array(q, dtype=np.float32), b)
2021-08-22T00:24:56.9142105Z         if q.shape == (3, ):
2021-08-22T00:24:56.9142599Z             q = q.reshape((1, 3))
2021-08-22T00:24:56.9143026Z         q = apply_PBC(q, b)
2021-08-22T00:24:56.9143539Z         aug, mapping = augment_coordinates(q, b, radius)
2021-08-22T00:24:56.9143998Z         if aug.size > 0:
2021-08-22T00:24:56.9144502Z             aug = np.sort(aug, axis=0)
2021-08-22T00:24:56.9144973Z         else:
2021-08-22T00:24:56.9145373Z             aug = list()
2021-08-22T00:24:56.9145860Z         if len(res) > 0:
2021-08-22T00:24:56.9146335Z             cs = transform_StoR(np.array(res, dtype=np.float32), b)
2021-08-22T00:24:56.9146925Z             cs = np.sort(cs, axis=0)
2021-08-22T00:24:56.9147339Z         else:
2021-08-22T00:24:56.9147791Z             cs = list()
2021-08-22T00:24:56.9148298Z >       assert_almost_equal(aug, cs, decimal=5)
2021-08-22T00:24:56.9148821Z E       AssertionError: 
2021-08-22T00:24:56.9149357Z E       Arrays are not almost equal to 5 decimals
2021-08-22T00:24:56.9149790Z E       
2021-08-22T00:24:56.9151198Z E       Mismatched elements: 4 / 21 (19%)
2021-08-22T00:24:56.9151861Z E       Max absolute difference: 10.
2021-08-22T00:24:56.9152437Z E       Max relative difference: 10.
2021-08-22T00:24:56.9152916Z E        x: array([[ 1.,  1., -9.],
2021-08-22T00:24:56.9153625Z E              [ 1.,  1., -9.],
2021-08-22T00:24:56.9154074Z E              [ 1.,  1.,  1.],...
2021-08-22T00:24:56.9154586Z E        y: array([[ 1.,  1.,  1.],
2021-08-22T00:24:56.9155095Z E              [ 1.,  1.,  1.],
2021-08-22T00:24:56.9155542Z E              [ 1.,  1.,  1.],...
2021-08-22T00:24:56.9155818Z 
2021-08-22T00:24:56.9156374Z D:\a\1\s\testsuite\MDAnalysisTests\lib\test_augment.py:91: AssertionError
2021-08-22T00:24:56.9156964Z __________________________ test_augment[q5-res5-b1] ___________________________
2021-08-22T00:24:56.9157621Z [gw1] win32 -- Python 3.8.10 C:\hostedtoolcache\windows\Python\3.8.10\x64\python.exe
2021-08-22T00:24:56.9158034Z 
2021-08-22T00:24:56.9158546Z b = array([10., 10., 10., 45., 60., 90.], dtype=float32)
2021-08-22T00:24:56.9159120Z q = array([[1.5      , 1.7071068, 0.5      ]], dtype=float32)
2021-08-22T00:24:56.9159713Z res = [[1.1, 0.1, 0.1], [0.1, 1.1, 0.1], [0.1, 0.1, 1.1], [0.1, 1.1, 1.1], [1.1, 1.1, 0.1], [1.1, 0.1, 1.1], ...]
2021-08-22T00:24:56.9160160Z 
2021-08-22T00:24:56.9160575Z     @pytest.mark.parametrize('b', (
2021-08-22T00:24:56.9161209Z                              np.array([10, 10, 10, 90, 90, 90], dtype=np.float32),
2021-08-22T00:24:56.9161786Z                              np.array([10, 10, 10, 45, 60, 90], dtype=np.float32)
2021-08-22T00:24:56.9162348Z                              ))
2021-08-22T00:24:56.9162801Z     @pytest.mark.parametrize('q, res', qres)
2021-08-22T00:24:56.9163336Z     def test_augment(b, q, res):
2021-08-22T00:24:56.9163888Z         radius = 1.5
2021-08-22T00:24:56.9164418Z         q = transform_StoR(np.array(q, dtype=np.float32), b)
2021-08-22T00:24:56.9164963Z         if q.shape == (3, ):
2021-08-22T00:24:56.9165418Z             q = q.reshape((1, 3))
2021-08-22T00:24:56.9165908Z         q = apply_PBC(q, b)
2021-08-22T00:24:56.9166379Z         aug, mapping = augment_coordinates(q, b, radius)
2021-08-22T00:24:56.9166906Z         if aug.size > 0:
2021-08-22T00:24:56.9167357Z             aug = np.sort(aug, axis=0)
2021-08-22T00:24:56.9167847Z         else:
2021-08-22T00:24:56.9168467Z             aug = list()
2021-08-22T00:24:56.9168958Z         if len(res) > 0:
2021-08-22T00:24:56.9169449Z             cs = transform_StoR(np.array(res, dtype=np.float32), b)
2021-08-22T00:24:56.9170006Z             cs = np.sort(cs, axis=0)
2021-08-22T00:24:56.9170443Z         else:
2021-08-22T00:24:56.9170909Z             cs = list()
2021-08-22T00:24:56.9171359Z >       assert_almost_equal(aug, cs, decimal=5)
2021-08-22T00:24:56.9171881Z E       AssertionError: 
2021-08-22T00:24:56.9172341Z E       Arrays are not almost equal to 5 decimals
2021-08-22T00:24:56.9172828Z E       
2021-08-22T00:24:56.9173336Z E       Mismatched elements: 11 / 21 (52.4%)
2021-08-22T00:24:56.9173898Z E       Max absolute difference: 14.142136
2021-08-22T00:24:56.9174451Z E       Max relative difference: 10.
2021-08-22T00:24:56.9174940Z E        x: array([[-3.5    , -5.36396, -4.5    ],
2021-08-22T00:24:56.9175490Z E              [ 1.5    , -5.36396, -4.5    ],
2021-08-22T00:24:56.9175967Z E              [ 6.5    ,  1.70711,  0.5    ],...
2021-08-22T00:24:56.9176519Z E        y: array([[ 1.5    ,  1.70711,  0.5    ],
2021-08-22T00:24:56.9177000Z E              [ 6.5    ,  8.77817,  0.5    ],
2021-08-22T00:24:56.9177535Z E              [ 6.5    ,  8.77817,  0.5    ],...
2021-08-22T00:24:56.9177827Z 
2021-08-22T00:24:56.9178415Z D:\a\1\s\testsuite\MDAnalysisTests\lib\test_augment.py:91: AssertionError

@tylerjereddy
Copy link
Member Author

I'll leave the "WIP" in the title for now since I'm not sure if you'll want to just merge the fix in to here when ready or xfail/merge/remove the xfail, etc.

@IAlibay
Copy link
Member

IAlibay commented Aug 22, 2021

I'll leave the "WIP" in the title for now since I'm not sure if you'll want to just merge the fix in to here when ready or xfail/merge/remove the xfail, etc.

Thanks for working on this @tylerjereddy !

@richardjgowers - thoughts on just xfailing the test? I think you mentioned it probably wasn't a problem? It is using StoR stuff, which I think @hmacdope mentioned was quite flaky?

@hmacdope
Copy link
Member

@IAlibay i can't say for sure, but early indications are that StoR may lose a fair bit of precision. If you are having issues with a numerical match here could be the case. See issue #3361.

@richardjgowers
Copy link
Member

@IAlibay Yeah it's not fantastic obviously but not a show stopper, I don't think we actually use these functions often. If we're dropping precision it might be possible to drop to O2 and see if that fixes it for now?

@pep8speaks
Copy link

pep8speaks commented Aug 27, 2021

Hello @tylerjereddy! 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 2022-01-21 20:31:58 UTC

@tylerjereddy tylerjereddy force-pushed the treddy_azure_latest_win branch from 8c60aaf to 5f21527 Compare August 27, 2021 02:11
@tylerjereddy
Copy link
Member Author

@richardjgowers I've pushed in a change to drop to -O2 on Windows. It might be possible to scope that change to i.e., a specific version range of msvc or something, but not sure how easy that is with distutils/setuptools stuff. Anyway, I guess we'll see if the fix works before we worry about the scope.

@hmacdope
Copy link
Member

@tylerjereddy does -O2 banish the failure? If so curious, I wonder what kind of shortcut is throwing away precision. Definitely worth investigating further in #3361, I'll try and get to it sometime soon.

@tylerjereddy
Copy link
Member Author

No idea, I'm testing by CI so we will see soon

@tylerjereddy
Copy link
Member Author

It looks like the answer is "no" to optimization level drop helping; also, results are slightly more confusing because of the h5py release causing a new windows failure I think.

@hmacdope
Copy link
Member

For the use of StoR in #3360 I just had to drop the number of decimals I was matching to. :(

@IAlibay
Copy link
Member

IAlibay commented Aug 27, 2021

@tylerjereddy - sorry I just did a commit to see what happened if we dropped to O1 (just to rule out an optimisation being the cause here).

@IAlibay
Copy link
Member

IAlibay commented Aug 27, 2021

No dice - I guess there's a deeper issue here.

tylerjereddy and others added 5 commits January 21, 2022 11:55
* try to capture the test error described in
MDAnalysisgh-3248 by using the newest possible Windows
base image/compiler toolset to run a matrix
entry

* I have not tried overriding with `imageName`
in a matrix entry like this before, but it seems
it is documented:
https://docs.microsoft.com/en-us/azure/devops/pipelines/get-started-multiplatform?view=azure-devops
* try lowering the compiler optimization level on
Windows to deal with precision loss with msvc 2019
This reverts commit 6e2b1d1.
* bump to new Azure windows image
more broadly

* `xfail` `test_augment` on Windows
per MDAnalysisgh-3248
@tylerjereddy tylerjereddy force-pushed the treddy_azure_latest_win branch from 6e2b1d1 to 78f1786 Compare January 21, 2022 20:31
@tylerjereddy
Copy link
Member Author

@IAlibay Ok, I pushed in the changes to apply the Windows image bump more broadly and temporarily xfail the problematic test on Windows until the issue is resolved. I guess we'll see if the CI is happy.

I probably could have scoped the xfail to only the subset of parameters that actually cause the Windows failures, but I went with the more convenient solution. Still gets tested on Mac/Linux at least. Feel free to adjust of course.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Assuming CI returns green - lgtm! Thanks so much @tylerjereddy !

@IAlibay IAlibay merged commit d593993 into MDAnalysis:develop Jan 21, 2022
@tylerjereddy tylerjereddy deleted the treddy_azure_latest_win branch January 21, 2022 22:40
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.

5 participants