Cache files for different CachingFileManager objects separately

[shoyer]

This means that explicitly opening a file multiple times with
open_dataset (e.g., after modifying it on disk) now reopens the file
from scratch, rather than reusing a cached version.

If users want to reuse the cached file, they can reuse the same xarray
object. We don't need this for handling many files in Dask (the original
motivation for caching), because in those cases only a single
CachingFileManager is created.

I think this should some long-standing usability issues: #4240, #4862

Conveniently, this also obviates the need for some messy reference
counting logic.

• Closes jupyter repr caching deleted netcdf file #4240, Obtaining fresh data from the disk when reopening a NetCDF file a second time #4862
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst

[cjauvin]

Thank you for the feedback! I quickly tested your suggested fix against the script I refered to in my original issue, and it's still behaving the same if I'm not mistaken. I looked very quickly so perhaps I'm wrong, but what I seem to understand is that your fix is similar to an idea my colleague @huard had, which was to make the cached item more granular by adding a call to Path(..).stat() in the cache key tuple (the idea being that if the file has changed on disk between the two open calls, this will detect it). It doesn't work because (I think) it doesn't change the fact that the underlying netcdf file is never explicitly close, that is, this line is never called:

xarray/xarray/backends/file_manager.py

Line 222 in a5f53e2

file.close()

Sorry in advance if something in my analysis is wrong, which is very likely!

[cjauvin]

As my colleague @huard suggested, I have written an additional test which demonstrates the problem (essentially the same idea I proposed in my initial issue):

master...cjauvin:add-netcdf-refresh-test

As I explained in the issue I have a potential fix for the problem:

master...cjauvin:netcdf-caching-bug

but the problem is that it feels a bit weird to have to that, so I suspect that there's a better way to solve it.

[shoyer]

Thanks for sharing the test case, which I'll add to this PR. I ran it locally and it seems to pass with this PR for the netCDF4 and SciPy netCDF backends (but not h5netcdf). So I'm not entirely sure what to make of that.

…

On Mon, Feb 8, 2021 at 1:56 PM Christian Jauvin ***@***.***> wrote: As my colleague @huard <https://github.com/huard> suggested, I have written an additional test which demonstrates the problem (essentially the same idea I proposed in my initial issue <#4862>): master...cjauvin:add-netcdf-refresh-test <master...cjauvin:add-netcdf-refresh-test> As I explained in the issue I have a potential fix for the problem: master...cjauvin:netcdf-caching-bug <master...cjauvin:netcdf-caching-bug> but the problem is that it feels a bit weird to have to that, so I suspect that there's a better way to solve it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4879 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJFVTISNAZV4YYOUMURLLS6BMZHANCNFSM4XH4MWIA> .

[shoyer]

I added @cjauvin's integration test, and verified that the fix works for the scipy and h5netcdf backends.

Unfortunately, it doesn't work yet for the netCDF4 backend. I don't think we can solve this in Xarray without fixes netCDF4-Python or the netCDF-C library: Unidata/netcdf4-python#1195

[dcherian]

I don't think we can solve this in Xarray without fixes netCDF4-Python or the netCDF-C library:

I think we should document this and merge. Though the test failures are real (having trouble cleaning up on windows when deleting the temp file), and the diff includes some zarr files right now.

[shoyer]

OK, after a bit more futzing tests are passing and I think this is actually ready to go in. I ended up leaving in the reference counting after all -- I couldn't figure out another way to keep files open after a pickle round-trip.

[shoyer]

Actually maybe we don't need to keep files open after pickling... let me give this one more try.

Nevermind, this didn't work -- it still results in failing tests with dask-distributed on Windows.

[shoyer]

Anyone want to review here? I think this should be ready to go in.

[Illviljan]

Some typing suggestions while we're at it.

[Illviljan]

Suggested change

	def __getstate__(self):
	def __getstate__(self) -> tuple[Any, Any, Any, Any, Any, Any]:

The Any's can be replaced with narrower versions, I couldn't figure them out on a quick glance.

[dcherian]

mypy error:

xarray/backends/file_manager.py:277: error: Accessing "init" on an instance is unsound, since instance.init could be from an incompatible subclass [misc]

for

    def __setstate__(self, state):
        """Restore from a pickle."""
        opener, args, mode, kwargs, lock = state
        self.__init__(opener, *args, mode=mode, kwargs=kwargs, lock=lock)

Seems like we can just ignore?

[shoyer]

I think we could fix this by marking CachingFileManager with typing.final

[headtr1ck]

LGTM

[headtr1ck]

I don't know enough what exactly this is used for, but make sure that you don't need to do a similar thing as for lock (replace with None in case it is default).
But ignore this comment if this is totally intentional :)