brandon-rhodes / python-jplephem

Python version of NASA DE4xx ephemerides, the basis for the Astronomical Alamanac
MIT License
110 stars 29 forks source link

Make SPK class close mmaps when exiting context manager #25

Closed jdavies-st closed 5 years ago

jdavies-st commented 5 years ago

This fixes an issue where the ephemeris file is left open because mmaps are not closed. This came up in the recent astropy 3.1 release testing.

This behavior can by simply reproduced by using pytest to run the unit tests and using the pytest-openfiles plugin (available via pip install). Output below (trimmed for clarity):

$ pytest jplephem/test.py --open-files
===================================================================== test session starts ======================================================================
platform darwin -- Python 3.7.1, pytest-4.0.0, py-1.7.0, pluggy-0.8.0
rootdir: /Users/jdavies/dev/python-jplephem, inifile:
plugins: requests-mock-1.5.2, xdist-1.24.1, remotedata-0.3.1, openfiles-0.3.0, forked-0.2, doctestplus-0.2.0, cov-2.6.0, arraydiff-0.2, ci-watson-0.2
collected 38 items                                                                                                                                             

test.py .........E..E..E..E..E...E..................                                                                                                     [100%]

============================================================================ ERRORS ============================================================================
_________________________________________________________ ERROR at teardown of SPKTests.test_array_tdb _________________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

___________________________________________________ ERROR at teardown of SPKTests.test_array_tdb_scalar_tdb2 ___________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

________________________________________________________ ERROR at teardown of SPKTests.test_scalar_tdb _________________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

____________________________________________________ ERROR at teardown of SPKTests.test_scalar_tdb2_keyword ____________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

____________________________________________________ ERROR at teardown of SPKTests.test_scalar_tdb_keyword _____________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

______________________________________________________ ERROR at teardown of SPKTests.test_too_early_date _______________________________________________________

E           AssertionError: File(s) not closed:
E             /Users/jdavies/dev/python-jplephem/jplephem/de421.bsp

============================================================== 38 passed, 6 error in 0.46 seconds ==============================================================

I've used try/except blocks to close the mmaps and references to them. I'm not sure this is the best way. I had originally coded it up with lots of hasattr checks. Would be happy to change if that is preferred.

I've also added __enter__ and __exit__ methods to the SPK class so that it can be used with a with context manager. Very simple and useful - one doesn't have to remember to explicitly call SPK.close() in order to make sure the file gets closed. I've updated one of the tests to use the with context manager so this is covered.

With these fixes, one can see that files are no longer left open:

$ pytest jplephem/test.py --open-files
===================================================================== test session starts ======================================================================
platform darwin -- Python 3.7.1, pytest-4.0.0, py-1.7.0, pluggy-0.8.0
rootdir: /Users/jdavies/dev/python-jplephem, inifile:
plugins: requests-mock-1.5.2, xdist-1.24.1, remotedata-0.3.1, openfiles-0.3.0, forked-0.2, doctestplus-0.2.0, cov-2.6.0, arraydiff-0.2, ci-watson-0.2
collected 38 items                                                                                                                                             

test.py ......................................                                                                                                           [100%]

================================================================== 38 passed in 0.32 seconds ===================================================================
brandon-rhodes commented 5 years ago

(And, let's ignore the Appveyor results for now; I'll see if I can remove them or get them working again soon.)

jdavies-st commented 5 years ago

FWIW, we're using jplephem over in the JWST pipeline code to do barycentric correction. And of course it is used in astropy. In both cases, I believe the __enter__ and __exit__ functionality will come in handy. Many thanks!