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

Unit Test Failure #256

Closed
tylerjereddy opened this issue Apr 18, 2015 · 7 comments
Closed

Unit Test Failure #256

tylerjereddy opened this issue Apr 18, 2015 · 7 comments

Comments

@tylerjereddy
Copy link
Member

Unit tests are not producing an 'OK' result using the latest development branch (or other recent stable releases) on a mac (maybe linux as well?), even when run in serial (not parallel). See the discussion mailing list for some of the details: https://groups.google.com/forum/#!topic/mdnalysis-discussion/86gnD3mIGj0

I used git bisect to identify the first bad commit when testing on my mac:
cad2cf9 [Tue Jul 8 19:12:55 2014]

I think it would be sensible to restore an 'OK' test result to the development branch (and future releases) by addressing whatever issues are causing errors and failures. People should be able to see that 'OK' to verify the integrity of their installation. I'm not sure how much overlap this issue may have with other unit testing issues (i.e., #124).

Also note that although the above commit is clearly causing some issues, this does not mean that other issues with producing an OK unit test result don't get independently introduced later.

@tylerjereddy
Copy link
Member Author

Here's the nosetest log output with a non-user install from latest development branch: https://www.dropbox.com/s/jggqg7anqw4ti1m/nose_output.txt?dl=0

And with a --user flag install with latest development branch I seem to get the same result as well:

python setup.py build
python setup.py install --user

I guess if it is a bit confusing for me to get the nosetests to work perfectly that's a sign that maybe we could do something a bit better, especially for an end user.

@richardjgowers
Copy link
Member

I can get them all working in linux (running nosetests from within /MDAnalysisTests/), so it's possible to get 100%. From the log I think Oliver is right on the dev board, that it's just file permissions. So possibly the install method for mac.

@tylerjereddy
Copy link
Member Author

It's definitely not file permissions. I just confirmed that the full set of tests pass on OS X if I increase the limit on the number of open file descriptors allowed with: ulimit -S -n 10000

The default is 256. One question is--why are so many being left open? Another question--how can we temporarily increase the system limit by default so that the end user doesn't encounter this problem?

I tried to increase the soft limit in __init__.py with:

import resource
current_hard_limit_open_file_descriptors = resource.getrlimit(resource.RLIMIT_NOFILE)[-1]
resource.setrlimit(resource.RLIMIT_NOFILE, (10000, current_hard_limit_open_file_descriptors))

But it didn't work (well, nosetests failed). That only applies to the given process. But maybe we could get something like that to work (rather than just telling people to set the fd limit to a larger value?).

@richardjgowers
Copy link
Member

That's weird. Maybe the tearDown in each Testcase isn't properly closing the file handle? Iirc Universe doesn't actually have a del, so you could try adding "self.u.trajectory.close()" to a tearDown and see if it lets another test pass?

@tylerjereddy tylerjereddy self-assigned this Apr 19, 2015
@tylerjereddy
Copy link
Member Author

Alright, I'll assign it to myself and take a closer look when I have time. Actually if that is indeed the problem writing a context manager to handle the close might be more pythonic?

@richardjgowers
Copy link
Member

It would, #239 is already about that I think.

@tylerjereddy
Copy link
Member Author

Statements closing the trajectory objects in a few locations has resolved the issue on my machine. I've pushed the fix to develop. I'm going to close this and leave a proper context manager implementation to another issue, like the one you've quoted. There's probably a nicer way to do this than putting a close() statement in the teardown method, but it works and only had to be done in a small number of locations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants