io.fits tests fail on Windows as memmap behaviour has changed

I'm not completely sure when this became a problem, and suspect it is related to a numpy update, but recent appveyor builds (e.g., https://ci.appveyor.com/project/Astropy/astropy/build/1.0.6535/job/y82p08ealenwojtl) at a spot for which there is a specific comment that it is a horrible hack:

            if sys.platform.startswith('win'):
                # On Windows, all the original data mmaps were closed above.
                # However, it's possible that the user still has references to
                # the old data which would no longer work (possibly even cause
                # a segfault if they try to access it).  This replaces the
                # buffers used by the original arrays with the buffers of mmap
                # arrays created from the new file.  This seems to work, but
                # it's a flaming hack and carries no guarantees that it won't
                # lead to odd behavior in practice.  Better to just not keep
                # references to data from files that had to be resized upon
                # flushing (on Windows--again, this is no problem on Linux).
                for idx, mmap, arr in mmaps:
                    if mmap is not None:
>                       arr.data = self[idx].data.data
E                       DeprecationWarning: Assigning the 'data' attribute is an inherently unsafe operation and will be removed in the future.
astropy\io\fits\hdu\hdulist.py:1342: DeprecationWarning
 2 failed, 11580 passed, 200 skipped, 79 xfailed, 1 xpassed in 417.46 seconds =

@embray

git blame gives code created in May 2012 by @embray.

OK, it is a numpy update: 1.12 gives these deprecation warnings (see release notes)

So, what would be the new behavior? Just throw an exception or something?

I don't think we can throw an exception -- that would just break things on windows even if users do not have a reference to these files still around. I hope someone with any experience with windows can help here...

Sorry about this inconsistency. In ci-helpers I was waiting for numpy 1.12 to come out on the defaults channel before making it the default stable, but apparently we only have that control built in our travis builds, so appveyor picked up np 1.12 straight after it became available.

@MSeifert04

Pinging @MSeifert04 and @mwcraig as I think they have the most experience with windows among us.

xfailing tests for np1.12 and windows that are affected by astropy#5797

I've added a commit in #5642 to temporarily xfail these two tests for numpy 1.12 and windows, so we can still run appveyor while this issue is solved.

xfailing tests for np1.12 and windows that are affected by astropy#5797

ah, that's a nice hack. I always wondered why I had some permission errors when I used memmap after closing a file.

What behaviour is actually useful here? It seems that io.fits on purpose allows memmaps to extend beyond their "expected" lifetime. I can't think of an not-hacky way to deal with that but I'm actually not too familiar with memory maps and file handles.

I changed the title -- I fear this will need a real fits/memory mapping expert...

I've also raised an issue with numpy -- numpy/numpy#8628 -- just in case somebody there has ideas (and to ensure we don't get caught if writing to data is removed altogether).

I don't know why the numpy folks decided this shouldn't be allowed anymore. I mean yes, it is unsafe in general, but it was done here for a reason. May be necessary to write something in Cython to do it.

Changed title to reflect that this is a general problem on windows with all recent versions of numpy.

A possible fix to this would be to recreate hdu.data, so change

mmaps = [(idx, _get_array_mmap(hdu.data), hdu.data)
                             for idx, hdu in enumerate(self) if hdu._has_data]

to s/hdu.data/hdu/

mmaps = [(idx, _get_array_mmap(hdu.data), hdu)
                             for idx, hdu in enumerate(self) if hdu._has_data]

then the lines that are emitting the warning become

              for idx, mmap, hdu in mmaps:
                    if mmap is not None and hdu.data is not None:
                        # recreate hdu.data from self[idx].data.data
                        hdu.data = np.frombuffer(...)

@mattip

@mattip - thanks very much for the suggestion! I tried looking into it in detail again, and think the worry is not about the user having a reference still to hdu, but rather that they have a reference to hdu.data, i.e., the actual array contained in the file. The latter can be a mmap and it is bad form to have that closed under one's feet. In the original code, therefore, the pointer of the array was just changed. It is not clear at all this is better if there are views, etc.

I think a user having a reference to hdu is not as much of a problem, as then getting data might well do the right thing.

Anyway, since we have had no reports at all of problems from this stanza not having worked roughly forever, I'm doing some tests to see if the whole work-around is in fact needed; see #9228.

#9228 failed, i.e., we still need the work-around for windows: any memory maps need to be closed before resizing the file.

I think a user having a reference to hdu is not as much of a problem, as then getting data might well do the right thing.

That was my intention

Navigation Menu

Repository navigation

io.fits tests fail on Windows as memmap behaviour has changed #5797

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

io.fits tests fail on Windows as memmap behaviour has changed #5797

Description

Activity

mhvk commented on Feb 10, 2017

mhvk commented on Feb 10, 2017

pllim commented on Feb 10, 2017

mhvk commented on Feb 10, 2017

bsipocz commented on Feb 11, 2017

bsipocz commented on Feb 11, 2017

bsipocz commented on Feb 11, 2017

MSeifert04 commented on Feb 11, 2017

mhvk commented on Feb 17, 2017

embray commented on Mar 30, 2017

mhvk commented on Apr 25, 2018

mattip commented on Aug 28, 2019

mhvk commented on Sep 14, 2019

mhvk commented on Sep 14, 2019

mattip commented on Sep 14, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions