CI: add 32-bit Win to CI #2696

tylerjereddy · 2020-05-24T18:54:44Z

Supersedes #2695

Add 32-bit Windows testing via Azure CI (not Appveyor because of the lack of parallel jobs there)

It is up to the team if we want to just place fixes in this PR before merging, switch to "allowed failure" somehow, or just merge in when ready and have "red CI" for a bit until the issues are fixed/skipped/xfailed, etc.

this is a pared-down version of the 32-bit Windows testing I set up for NumPy/SciPy--probably could pare it down even more, but seems to catch the test failures we'd expect

Related to:
#2693
#2687
#2689
#2346

tylerjereddy · 2020-05-24T18:59:07Z

Ok, it is running now. I tried to delete the spurious/duplicate CI (1) entry for Azure, so that should go away in the future.

* changes needed to pass full test suite on 32-bit Windows * primarily, use `np.intp` indexing types in appropriate locations instead of forcing `np.int64`

tylerjereddy · 2020-05-25T00:48:06Z

@lilyminium @richardjgowers I pushed in a commit that allows 32-bit Windows and 64-bit Linux test suites to pass locally.

We'll see if the CI agrees, but you'll probably want to review carefully anyway. In short, mostly np.int64 -> np.intp changes, and I think I avoided any coercion to int64.

tylerjereddy · 2020-05-25T00:50:01Z

testsuite/MDAnalysisTests/analysis/test_density.py

@@ -398,6 +398,8 @@ class TestNotWithin(object):
    def u():
        return mda.Universe(GRO)

+    @pytest.mark.skipif(sys.maxsize <= 2**32,
+                        reason="non-kdtree density too large for 32-bit")


memory allocation issue specifically, I think

tylerjereddy · 2020-05-25T00:53:33Z

testsuite/MDAnalysisTests/topology/test_tprparser.py

@@ -55,7 +55,7 @@ def test_moltypes(self, top):
    def test_molnums(self, top):
        molnums = top.molnums.values
        assert_equal(molnums, self.ref_molnums)
-        assert molnums.dtype == np.int64
+        assert molnums.dtype == np.intp


While nobody likes changing unit tests to make them pass, I think the proposed approach (from Richard and maybe Lily?) for portability is that integer indexing will now be platform-sized rather than coerced to int64, so some tests will have platform-specific typing.

tylerjereddy · 2020-05-25T02:15:23Z

There's only 1 32-bit test failure left in Azure CI--it is an unreliable failure according to hypothesis. I'll let the team decide what to do with that one.

richardjgowers · 2020-05-25T13:41:20Z

Ok this is cool. So the one failure is something string related that hypothesis found. Can we tag this from pytest is a knownfail with win32 platforms then we can include this in our test matrix?

* we now conditionally `xfail` a test that is problematic on 32-bit Windows: `MDAnalysisTests/formats/test_libdcd.py::test_written_remarks_property` * `hypothesis` only occasionally produces a failure for this test on 32-bit Windows

tylerjereddy · 2020-05-25T17:52:16Z

Can we tag this from pytest is a knownfail with win32 platforms then we can include this in our test matrix?

@richardjgowers Ok, I added a third commit to xfail that specific test on 32-bit windows. I tried to reproduce locally, but my own 32-bit Windows setup is happy to pass that test. I suspect we can just investigate this later--maybe I have a few extra add-ons installed that bare-bones Azure doesn't.

codecov · 2020-05-25T20:13:27Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@a0bd19c). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             develop    #2696   +/-   ##
==========================================
  Coverage           ?   91.15%           
==========================================
  Files              ?      175           
  Lines              ?    23719           
  Branches           ?     3104           
==========================================
  Hits               ?    21620           
  Misses             ?     1480           
  Partials           ?      619

Impacted Files	Coverage Δ
package/MDAnalysis/core/selection.py	`99.47% <100.00%> (ø)`
package/MDAnalysis/core/topologyattrs.py	`97.48% <100.00%> (ø)`
package/MDAnalysis/lib/NeighborSearch.py	`83.33% <100.00%> (ø)`
package/MDAnalysis/lib/_augment.pyx	`100.00% <100.00%> (ø)`
package/MDAnalysis/lib/_cutil.pyx	`98.81% <100.00%> (ø)`
package/MDAnalysis/lib/distances.py	`97.47% <100.00%> (ø)`
package/MDAnalysis/lib/nsgrid.pyx	`83.66% <100.00%> (ø)`
package/MDAnalysis/lib/pkdtree.py	`89.13% <100.00%> (ø)`
package/MDAnalysis/analysis/base.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/selections/jmol.py	`100.00% <0.00%> (ø)`
... and 173 more

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 a0bd19c...0b560f5. Read the comment docs.

lilyminium · 2020-05-25T22:29:15Z

testsuite/MDAnalysisTests/analysis/test_hole2.py

@@ -73,6 +73,8 @@ def test_relative(self):
            fixed = check_and_fix_long_filename(abspath)
            assert fixed == self.filename

+    @pytest.mark.skipif(os.name == 'nt' and sys.maxsize <= 2**32,
+                        reason="FileNotFoundError on Win 32-bit")
    def test_symlink_dir(self, tmpdir):


Was this also because of not having symlinking privileges, or is it some weirdness with Windows paths and os.path.join?

(Just out of interest; I'm not sure hole2 even supports Windows.)

lilyminium · 2020-05-25T22:44:05Z

package/MDAnalysis/core/selection.py

@@ -226,7 +226,7 @@ class ByResSelection(UnarySelection):

    def apply(self, group):
        res = self.sel.apply(group)
-        unique_res = unique_int_1d(res.resindices.astype(np.int64))
+        unique_res = unique_int_1d(res.resindices.astype(np.intp))


Is this astype necessary? I think it required coercion earlier as resindices are intp typically.

I've pushed in a commit to remove it--32-bit Windows suite passed locally without it. I didn't have time to run the suite for 64-bit linux as well, but fingers crossed.

lilyminium · 2020-05-25T22:48:51Z

Thanks @tylerjereddy! This looks great. I don't know much about win32 but only 5 xfails seems really good; I suppose the non-kdtree densities could have warnings, same with the symlinking (I guess symlinking privileges would be a Windows-wide issue?).

* remove an extraneous type coercion in `selection.py` based on reviewer feedback

lilyminium

Thank you @tylerjereddy!

orbeckst · 2020-05-27T23:17:57Z

Great work, @tylerjereddy !

orbeckst · 2020-05-27T23:18:55Z

Are we now in a position to officially support 32-bit Windows?

Should there be something in CHANGELOG?

tylerjereddy · 2020-05-27T23:35:49Z

Should there be something in CHANGELOG?

We could add something--I don't know if I'd go as far as "official support," but pretty close.

tylerjereddy · 2020-05-28T01:41:05Z

FWIW, I'd suggest not using a single plain text changelog for entries since it can be a source of merge conflicts. A simple wiki page in between releases can work well enough. Some projects use fancy tools to add separate notes that get merged later or whatever.

Anyway, I'll try to push something in shortly.

tylerjereddy · 2020-05-28T01:47:13Z

Ok, I've pushed in a changelog entry--I put the recent change at top, but not my name at top--mostly to avoid reformatting the author list, but also technically I've made other changes for the release already without recording the name so that's my excuse.

tylerjereddy · 2020-05-28T14:52:24Z

Alright, CI is green, 2 core dev approvals and I added a short release note, so merging

orbeckst · 2020-05-28T16:34:47Z

FWIW, I'd suggest not using a single plain text changelog for entries since it can be a source of merge conflicts.

It is true that most merge conflicts come from our custom to add new changes at the top of each category. Maybe it would be better to add at the bottom.

A simple wiki page in between releases can work well enough.

However, I don't like using a Wiki because then we need to check something separately when reviewing PRs, much better to have everything in one place, especially with new contributors.

Some projects use fancy tools to add separate notes that get merged later or whatever.

Yes, I've seen these. Is it worth the extra work?

CI: add 32-bit Win to CI.

336d3ff

tylerjereddy added OpSys-Windows 32 bit Continuous Integration labels May 24, 2020

tylerjereddy closed this May 24, 2020

tylerjereddy reopened this May 24, 2020

MAINT: 32-bit windows support

522f9ae

* changes needed to pass full test suite on 32-bit Windows * primarily, use `np.intp` indexing types in appropriate locations instead of forcing `np.int64`

tylerjereddy commented May 25, 2020

View reviewed changes

MAINT: PR 2696 revisions

2392c35

* we now conditionally `xfail` a test that is problematic on 32-bit Windows: `MDAnalysisTests/formats/test_libdcd.py::test_written_remarks_property` * `hypothesis` only occasionally produces a failure for this test on 32-bit Windows

tylerjereddy changed the title ~~WIP, CI: add 32-bit Win to CI.~~ CI: add 32-bit Win to CI May 25, 2020

lilyminium reviewed May 25, 2020

View reviewed changes

MAINT: PR 2696 revisions

d6ea367

* remove an extraneous type coercion in `selection.py` based on reviewer feedback

richardjgowers approved these changes May 26, 2020

View reviewed changes

lilyminium approved these changes May 27, 2020

View reviewed changes

DOC: relnote for gh-2696.

0b560f5

tylerjereddy merged commit 186cebf into develop May 28, 2020

tylerjereddy deleted the add_win32_ci branch May 28, 2020 14:52

fiona-naughton added the maintainability label Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: add 32-bit Win to CI #2696

CI: add 32-bit Win to CI #2696

tylerjereddy commented May 24, 2020

tylerjereddy commented May 24, 2020

tylerjereddy commented May 25, 2020

tylerjereddy May 25, 2020

tylerjereddy May 25, 2020

tylerjereddy commented May 25, 2020

richardjgowers commented May 25, 2020

tylerjereddy commented May 25, 2020

codecov bot commented May 25, 2020 •

edited

Loading

lilyminium May 25, 2020

lilyminium May 25, 2020

tylerjereddy May 26, 2020

lilyminium commented May 25, 2020

lilyminium left a comment

orbeckst commented May 27, 2020

orbeckst commented May 27, 2020

tylerjereddy commented May 27, 2020

tylerjereddy commented May 28, 2020

tylerjereddy commented May 28, 2020

tylerjereddy commented May 28, 2020

orbeckst commented May 28, 2020

CI: add 32-bit Win to CI #2696

CI: add 32-bit Win to CI #2696

Conversation

tylerjereddy commented May 24, 2020

tylerjereddy commented May 24, 2020

tylerjereddy commented May 25, 2020

tylerjereddy May 25, 2020

Choose a reason for hiding this comment

tylerjereddy May 25, 2020

Choose a reason for hiding this comment

tylerjereddy commented May 25, 2020

richardjgowers commented May 25, 2020

tylerjereddy commented May 25, 2020

codecov bot commented May 25, 2020 • edited Loading

Codecov Report

lilyminium May 25, 2020

Choose a reason for hiding this comment

lilyminium May 25, 2020

Choose a reason for hiding this comment

tylerjereddy May 26, 2020

Choose a reason for hiding this comment

lilyminium commented May 25, 2020

lilyminium left a comment

Choose a reason for hiding this comment

orbeckst commented May 27, 2020

orbeckst commented May 27, 2020

tylerjereddy commented May 27, 2020

tylerjereddy commented May 28, 2020

tylerjereddy commented May 28, 2020

tylerjereddy commented May 28, 2020

orbeckst commented May 28, 2020

codecov bot commented May 25, 2020 •

edited

Loading