Skip failing windows unittests, and omit max_rss on windows (+ correctly report for OS X)

[hyanwong]

Also add in other packages to get windows compilation working

[hyanwong]

This is mostly there now I think, but still fails on Windows AppVeyor images with "lmdb error There is not enough space on the disk.". This google post says "On Windows the OS sets the filesize equal to the mapsize", and tensorpack/tensorpack#1209 (comment) recommends "starting with a small map_size on windows but increasing gradually when the dataset is full". Not sure if this helps

[jeromekelleher]

Great stuff, thanks @hyanwong! I agree this is good enough for now - how about we put int a skipIf(windows) like here for the failing tests and open an issue to track the ones that look fixable?

Re psutil, we could just leave this out - it's a minor feature that windows users can live without. Or, if psutil is a light and easy dependency, we just go with this for everything.

[hyanwong]

Great stuff, thanks @hyanwong! I agree this is good enough for now - how about we put int a skipIf(windows) like here for the failing tests and open an issue to track the ones that look fixable?

Yes, that sounds a quick work-around. I'll check if it works on Windows with enough disk space, then add the hack with a comment.

Or, if psutil is a light and easy dependency, we just go with this for everything.

Annoyingly, although psutil makes rss available cross platform, the equivalent of max_rss in psutil (which is definitely what we want to report) is only available under windows (details as to why, here). So the only way to remain relatively cross-platform is to retain resource for unix-y systems and psutil for windows. If we are really bothered, we could somehow include a conditional dependency on psutil for windows only, but I don't know how to do that. Or as you say, we could drop reporting this for windows and simply leave a comment in the code that we could implement it using psutil if there is sufficient demand.

[jeromekelleher]

Annoyingly, although psutil makes rss available cross platform, the equivalent of max_rss in psutil (which is definitely what we want to report) is only available under windows (details as to why, here). So the only way to remain relatively cross-platform is to retain resource for unix-y systems and psutil for windows. If we are really bothered, we could somehow include a conditional dependency on psutil for windows only, but I don't know how to do that. Or as you say, we could drop reporting this for windows and simply leave a comment in the code that we could implement it using psutil if there is sufficient demand.

Makes sense, thanks. I would drop it personally - I can't see anyone doing really large scale stuff on windows, which is where kind of reporting is useful.

[hyanwong]

Agree. Will cut it out.

[codecov]

Codecov Report

Merging #166 into master will decrease coverage by 0.21%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
- Coverage   93.94%   93.72%   -0.22%     
==========================================
  Files          11       11              
  Lines        2874     2885      +11     
  Branches      504      507       +3     
==========================================
+ Hits         2700     2704       +4     
- Misses        144      148       +4     
- Partials       30       33       +3

Impacted Files	Coverage Δ
tsinfer/formats.py	95.49% <50%> (-0.27%)	⬇️
tsinfer/cli.py	93.7% <64.28%> (-1.83%)	⬇️

---

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 98ee2ec...c04a6c9. Read the comment docs.

[hyanwong]

LMDB/lmdb@fb5a768 implies that it should be possibly to dynamically grow the lmdb database on windows, so I don't know why it's a problem. Investigating further.

[hyanwong]

So it looks like the patch to make this work on windows simply hasn't made it into LMDB 0.9.22 (or even 0.9.23). See mozilla/lmdb-rs#23 and mozilla/lmdb-rs#40

Looks complicated! I don't know if there's a way to fix this, other than by having some sort of conditional code which decreases map_size when running in windows (yuck)

[jeromekelleher]

Let's just try reducing the map size for now (on windows) to get CI working, and skipIf any tests that don't behave. There's still value in getting this mostly working on Windows, we can figure out a good way of dealing with it later.

[hyanwong]

I set the map_size to 1GiB on Windows only. Now the AppVeyor build is mainly failing on permissions errors, presumably because windows doesn't like accessing an already-open file. E.g.

======================================================================
4449ERROR: test_overwrite_partial (test_formats.TestSampleData)
4450----------------------------------------------------------------------
4451Traceback (most recent call last):
4452  File "C:\projects\tsinfer\tests\test_formats.py", line 699, in test_overwrite_partial
4453    data.add_site(j, [0, 1])
4454  File "C:\Miniconda36-x64\lib\tempfile.py", line 807, in __exit__
4455    self.cleanup()
4456  File "C:\Miniconda36-x64\lib\tempfile.py", line 811, in cleanup
4457    _shutil.rmtree(self.name)
4458  File "C:\Miniconda36-x64\lib\shutil.py", line 494, in rmtree
4459    return _rmtree_unsafe(path, onerror)
4460  File "C:\Miniconda36-x64\lib\shutil.py", line 389, in _rmtree_unsafe
4461    onerror(os.unlink, fullname, sys.exc_info())
4462  File "C:\Miniconda36-x64\lib\shutil.py", line 387, in _rmtree_unsafe
4463    os.unlink(fullname)
4464PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tsinf_format_testpvw843w6\\samples.tmp'

and

ERROR: test_load (test_formats.TestSampleData)
4436----------------------------------------------------------------------
4437Traceback (most recent call last):
4438  File "C:\projects\tsinfer\tests\test_formats.py", line 45, in test_load
4439    self.assertRaises(IsADirectoryError, self.tested_class.load, "/")
4440  File "C:\Miniconda36-x64\lib\unittest\case.py", line 733, in assertRaises
4441    return context.handle('assertRaises', args, kwargs)
4442  File "C:\Miniconda36-x64\lib\unittest\case.py", line 178, in handle
4443    callable_obj(*args, **kwargs)
4444  File "C:\projects\tsinfer\tsinfer\formats.py", line 326, in load
4445    with open(path, "r"):
4446PermissionError: [Errno 13] Permission denied: '/'

There's also this:

======================================================================
4478FAIL: test_tree_sequence_builder (test_low_level.TestOutOfMemory)
4479----------------------------------------------------------------------
4480Traceback (most recent call last):
4481  File "C:\projects\tsinfer\tests\test_low_level.py", line 36, in test_tree_sequence_builder
4482    max_edges=1)
4483AssertionError: MemoryError not raised by TreeSequenceBuilder

[jeromekelleher]

I set the map_size to 1GiB on Windows only. Now the AppVeyor build is mainly failing on permissions errors, presumably because windows doesn't like accessing an already-open file. E.g.

Just skipIf these - it's not worth worrying about them. Otherwise looks good!

[hyanwong]

Yay. This now passes @jeromekelleher. I've rebased. Not sure how to deal with the coverage though, as it's only windows-specific - do you want a test to see if the max_rss is correctly reported in Unix vs absent in win32?

[jeromekelleher]

Thanks @hyanwong, this is fantastic! Let's not worry about the codecov report. Merging!