Close ascii files with unified file read interface by larrybradley · Pull Request #11809 · astropy/astropy

larrybradley · 2021-06-02T16:45:45Z

The regions package recently added the unified file read/write interface for regions (ascii) files. While running tests, I noticed that region files read with the unified I/O give a ResourceWarning about the file being unclosed. This exact issue was reported in #8673.

Starting with the hints provided by @taldcroft and @saimn in #8673 and #8675, I have a workaround/solution (based on code in click: https://github.com/pallets/click/blob/9da166957f5848b641231d485467f6140bca2bc0/src/click/_compat.py#L61). Essentially TextIOWrapper always closes the wrapped file when it is garbage collected and this somehow messes up the file closing by the context manager. The solution is to detach the underlying stream from TextIOWrapper before it is deleted (i.e., in its __del__ method). The same issue appears to apply to BufferedReader because both were necessary to close the files.

Those changes fixed most of the failing io.ascii tests due to files being left open (i.e., after no longer ignoring the ResourceWarnings in setup.cfg). The remaining test failures in the tests and documentation (doctests) were simple cases of files being opened, but not closed (fixed here).

There was one remaining test case that left a file open, but I could not fix. I removed this test because the fix requires a fix in an upstream package (jplephem). Essentially jplephem opens an invalid ephem file and then immediately raises a ValueError before closing the file. There's no way to fix that on the astropy side.

Closes #8673.

Addresses part of #9619 .

pllim

I applaud your bravery to jump into the resource warning rabbit hole. 👏

pllim · 2021-06-02T16:47:55Z

    Test guessing a file object.  Fixes #3013 and similar issue noted in #3019.
    """
-    t = ascii.read(open('data/ipac.dat.bz2', 'rb'))
+    fd = open('data/ipac.dat.bz2', 'rb')


For all the cases that calls open, can we not use context manager so it still exists gracefully if something went wrong with the test itself?

Probably a good idea, but I didn't want to change the tests too much.

Well, I guess we can always revisit in follow-up PR(s) if you don't want to address this here.

I did a few at least. In one, the diff does become substantially larger as the whole test is indented. I decided not to follow through too much on the fits hdul tests...

mhvk

Nice to actually have dived into this! But it still feels like an ugly hack! Also strange, that open itself has a closefd argument that would seem so helpful, but unfortunately BufferedReader does not. It still feels like that should be possible to use, but it got messy fast (I tried open(fileobj.fileno(), encoding=encoding, closefd=False) which helps for many cases, but then others do not have a working fileno(), etc.)

Anyway, I suggested an alternative in #11810, which is also hacky, but a bit more localized. Note that both PRs still have a failing test.

…test

larrybradley · 2021-06-02T22:05:08Z

This turned out to be a bigger rabbit hole than I originally expected. All of the tests were passing for me locally, but I neglected to run them with remote data. :(. The tests that require remote data are the ones that are failing now. I fixed several of them in the last two commits.

There's a set of tests still failing due to coordinates/solar_system.py. The root cause of those goes back to jplephem again. The _get_kernel function opens the file, but never closes it:

https://github.com/astropy/astropy/blob/main/astropy/coordinates/solar_system.py#L170
and https://github.com/astropy/astropy/blob/main/astropy/coordinates/solar_system.py#L179

Adding a kernel.close() at the end of the _get_body_barycentric_posvel function fixed all but one of the failing tests. The remaining failing test complained about trying to operate on a closed file, so my solution wasn't correct (or something else is needed). Perhaps a coordinates maintainer can take a look? In any case the issues with coordinates/solar_system.py are unrelated to the original issue of the Unified I/O ascii read leaving files open.

Also, I'm happy to go with the alternate solution by @mhvk in #11810. Thanks, @mhvk!

mhvk · 2021-06-02T22:48:11Z

@larrybradley - I'll have a look at solar_system_ephemeris. In principle, it does try to close the file, in the public method at

astropy/astropy/coordinates/solar_system.py

Lines 118 to 130 in 7507523

    
           @classmethod 
        
           def get_kernel(cls, value): 
        
               # ScienceState only ensures the `_value` attribute is up to date, 
        
               # so we need to be sure any kernel returned is consistent. 
        
               if cls._kernel is None or cls._kernel.origin != value: 
        
                   if cls._kernel is not None: 
        
                       cls._kernel.daf.file.close() 
        
                       cls._kernel = None 
        
                   kernel = _get_kernel(value) 
        
                   if kernel is not None: 
        
                       kernel.origin = value 
        
                   cls._kernel = kernel 
        
               return cls._kernel

But it sounds like code does not always get there.

On the solution, both are somewhat hacky, but most important seems to be to trace down the remaining problems... The one in io/fits/tests/test_table.py seems particularly tricky.

mhvk · 2021-06-02T22:51:51Z

Ah, your pointer to _get_body_barycentric_posvel is very useful - it bypasses the solar_system_ephemeris system....

mhvk · 2021-06-03T00:03:07Z

@larrybradley - OK, I have a fix for solar_system which makes remote tests pass (plus a small fix in test_solar_system for skyfield). Shall I push it to your branch? Or open a PR to yours? Or just put them on my alternative, so you can cherry-pick what you want?

mhvk · 2021-06-03T14:11:23Z

p.s. One sad part of our tests taking so very long, is that I've taken to test only specific files, which often ends up missing bugs. Especially bad are the remote tests where for every invocation the full DE ephemeris has to be downloaded... I wish there was some better caching possible... But let me be smart and open a separate issue...

mhvk · 2021-06-03T14:57:13Z

Darn, I had forgotten to include my fix for test_table in reverting to your method of dealing with detaching. Sigh.

mhvk · 2021-06-03T15:39:15Z

Hmm, the windows/all-dependencies error still seems related. @larrybradley - do you want to take back control? If not, I'll try sometime in the coming days...

pllim · 2021-06-03T16:06:41Z

This is not looking very backportable. Can we re-milestone to 5.0?

pllim

I'll have another look once @larrybradley and @mhvk sort out the details. Thanks!

pllim · 2021-06-03T16:08:59Z

    Test guessing a file object.  Fixes #3013 and similar issue noted in #3019.
    """
-    t = ascii.read(open('data/ipac.dat.bz2', 'rb'))
+    fd = open('data/ipac.dat.bz2', 'rb')


Well, I guess we can always revisit in follow-up PR(s) if you don't want to address this here.

pllim · 2021-06-03T16:15:12Z

Re: Windows failure -- Maybe you missed one?

astropy/astropy/io/fits/tests/test_core.py

Lines 824 to 828 in 2004a30

    
           def test_detect_zipped(self): 
        
               """Test detection of a zip file when the extension is not .zip.""" 
        
               zf = self._make_zip_file(filename='test0.fz') 
        
               assert len(fits.open(zf)) == 5

mhvk · 2021-06-03T20:48:13Z

@pllim - thanks, that looks right. Though why did it pass on non-windows? Anyway, will try!

mhvk · 2021-06-03T22:33:50Z

Finally, tests pass!!

larrybradley · 2021-06-07T21:40:52Z

This is ready to be merged.

pllim · 2021-06-07T22:22:02Z

I'll review in a few days unless someone else gets to it first.

pllim

I don't grok the solar_system.py changes but the rest LGTM. As much as I itch to turn everything to use context manager, I understand it is out of scope here.

Thanks!

pllim · 2021-06-08T17:46:22Z

I moved the milestone as I don't think this will backport cleanly, especially the solar_system.py stuff. Merging now.

taldcroft · 2021-06-08T18:11:39Z

Thank you so much @larrybradley and @mhvk and @pllim for finally solving this thorny issue! I tried once and failed (and really didn't want to touch this again), so it is great to see others step up and make it happen. 🎉

mhvk · 2021-06-08T18:40:13Z

Indeed, thanks, @larrybradley, for starting this and for another nice collaborative effort! With as a bonus that the solar system stuff is a bit more logical and better too!

larrybradley · 2021-06-08T18:52:19Z

Many thanks everyone, especially @mhvk who fixed the solar system stuff!

larrybradley added 7 commits June 2, 2021 12:10

Detach underlying stream before TextIOWrapper closes its stream

4ce4f4d

Detach underlying stream before BufferedReader closes its stream

e398d96

No longer ignore close file ResourceWarnings

f52b02d

Close files opened in io.ascii tests

cd3a962

Close files in FITS test_image.py

a50053b

Close files opened in io.fits tests

53ae0ba

Close files opened in narrative doc examples

190da13

larrybradley added io.ascii Bug utils labels Jun 2, 2021

larrybradley added this to the v4.0.6 milestone Jun 2, 2021

larrybradley requested review from pllim, saimn and taldcroft June 2, 2021 16:45

github-actions Bot added coordinates installation io.fits wcs labels Jun 2, 2021

pllim reviewed Jun 2, 2021

View reviewed changes

pllim requested a review from embray June 2, 2021 16:52

mhvk mentioned this pull request Jun 2, 2021

Close files alternative #11810

Closed

mhvk reviewed Jun 2, 2021

View reviewed changes

Comment thread astropy/coordinates/tests/test_solar_system.py Resolved

larrybradley added 3 commits June 2, 2021 17:50

Ignore ResourceWarning for open file in solar_system wrong ephemeris …

ba8a0e9

…test

Add changelog entry

1c0ef75

Close the hdulist in the Kepler/TESS fits reader

3314301

larrybradley force-pushed the close-files branch from 8964e02 to f2c3a46 Compare June 2, 2021 21:50

mhvk force-pushed the close-files branch from 0e43c40 to 584b3ab Compare June 3, 2021 14:09

Fix another resource warning case.

619ce14

pllim reviewed Jun 3, 2021

View reviewed changes

Ensure test zip file is closed properly

e975cd4

saimn reviewed Jun 3, 2021

View reviewed changes

Comment thread docs/io/fits/index.rst Resolved

Use context manager in tests a bit more often

7ac9f9c

Remove extraneous hdu close

a7e3b9f

larrybradley requested review from mhvk, pllim and saimn June 7, 2021 21:40

larrybradley added the Ready-for-final-review label Jun 7, 2021

pllim approved these changes Jun 8, 2021

View reviewed changes

pllim modified the milestones: v4.0.6, v5.0 Jun 8, 2021

pllim added the Refactoring label Jun 8, 2021

pllim merged commit 6387507 into astropy:main Jun 8, 2021

larrybradley deleted the close-files branch June 8, 2021 17:48

Conversation

larrybradley commented Jun 2, 2021 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

larrybradley Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

mhvk Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larrybradley commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jun 2, 2021

Uh oh!

mhvk commented Jun 2, 2021

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

pllim commented Jun 3, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim commented Jun 3, 2021

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

Uh oh!

mhvk commented Jun 3, 2021

Uh oh!

larrybradley commented Jun 7, 2021

Uh oh!

pllim commented Jun 7, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Jun 8, 2021

Uh oh!

taldcroft commented Jun 8, 2021

Uh oh!

mhvk commented Jun 8, 2021

Uh oh!

larrybradley commented Jun 8, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

larrybradley commented Jun 2, 2021 •

edited by pllim

Loading

larrybradley commented Jun 2, 2021 •

edited

Loading