Expose `memory` argument for "netcdf4" engine #6956

ianliu · 2022-08-26T14:04:25Z

This commit exposes the memory argument for the "netcdf4" engine,
allowing one to create a netcdf dataset from a memory buffer, like so:

buffer: bytes = ...
ds = xr.open_dataset("", engine="netcdf4", memory=buffer)

Closes Expose "memory" argument to the "netcdf4" engine #6955
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

dcherian · 2022-08-26T15:04:53Z

Thanks @ianliu you should be able to do this with xr.open_dataset(..., backend_kwargs={"memory": buffer})

ianliu · 2022-08-26T15:54:43Z

@dcherian backend_kwargs doesn't work either, because the NetCDF4BackendEntrypoint.open_dataset function doesn't accept a memory argument:

xarray/xarray/backends/netCDF4_.py

Lines 534 to 552 in 434f9e8

    
           def open_dataset( 
        
               self, 
        
               filename_or_obj, 
        
               mask_and_scale=True, 
        
               decode_times=True, 
        
               concat_characters=True, 
        
               decode_coords=True, 
        
               drop_variables=None, 
        
               use_cftime=None, 
        
               decode_timedelta=None, 
        
               group=None, 
        
               mode="r", 
        
               format="NETCDF4", 
        
               clobber=True, 
        
               diskless=False, 
        
               persist=False, 
        
               lock=None, 
        
               autoclose=False, 
        
           ):

dcherian · 2022-08-26T16:03:45Z

Ah thanks. I always forget that.

Can you add a test please? Somewhere in this class would be the right place I think

xarray/xarray/tests/test_backends.py

Line 1225 in 434f9e8

class NetCDF4Base(CFEncodedBase):

ianliu · 2022-08-26T17:11:05Z

@dcherian I've added a unit test, but outside the class. Is this a problem? I see in the Xarray's contributing page that Xarray is transitioning to more functional tests.

ianliu · 2022-08-26T17:23:14Z

Hmm, the test doesn't pass like this, it is complaining that nc4 is not defined. I will put the test inside the class.

ianliu · 2022-08-26T18:58:24Z

@dcherian can you review my changes? I think the PR is done now.

xarray/backends/netCDF4_.py

ianliu · 2022-08-30T19:19:46Z

I've just committed a change that also allows one to save a netcdf file to memory, like so:

ds = xr.Dataset({ "v": xr.DataArray(data=range(10)) })
buf = ds.to_netcdf(engine="netcdf4")
assert xr.open_dataset("", engine="netcdf4", memory=buf).equals(ds)

dcherian · 2022-10-03T20:55:29Z

xarray/backends/file_manager.py

@@ -221,7 +221,7 @@ def close(self, needs_lock=True):
            default = None
            file = self._cache.pop(self._key, default)
            if file is not None:
-                file.close()
+                return file.close()


Is this needed?

According to netCDF4's API, an in-memory dataset binary data is returned upon closing it. If you look at the diff-chunk over this one, I use the return value of close() to get the bytes.

reupping this question.

jhamman · 2023-01-31T15:46:11Z

@ianliu - are you interested in finishing up this PR? From my perspective, this seems like a useful feature that would be nice to get in to xarray.

ianliu · 2023-01-31T16:05:05Z

Hi @jhamman ! I'm willing to finish it, but don't know whats really missing. I guess I'll rebase with main and fix the failing tests.

But I would like a review on the second commit, since it adds more profound changes to the internal API's, such as returning a value from CachingFileManager.close function. This was needed because NetCDF4 returns the bytearray upon close.

Also, the logic on the to_netcdf function is starting to get a little bit convoluted, would be nice to get a review there as well.

jhamman

Just a few comments, including a suggestion for how to fix the failing test.

jhamman · 2023-01-31T20:55:09Z

xarray/backends/file_manager.py

@@ -221,7 +221,7 @@ def close(self, needs_lock=True):
            default = None
            file = self._cache.pop(self._key, default)
            if file is not None:
-                file.close()
+                return file.close()


reupping this question.

xarray/backends/api.py

xarray/backends/netCDF4_.py

jhamman · 2023-01-31T21:01:01Z

xarray/tests/test_backends.py

+        buf = ds.close()
+        with open_dataset("dummy.nc", engine="netcdf4", memory=buf) as ds:
+            assert all(ds.x == x)
+


Re the test failures below in test_engine, I think you can remove the pytest.raises for the case without a filename now.

ianliu · 2023-02-12T00:43:09Z

@jhamman sorry for the delay. So, the return that you commented "reupping this question", I did respond the first code review but forgot to submit my answer... I will respond here as well, because I find all these code reviews confusing.

The return statement is there because the netCDF4 API returns the byte array upon closing the dataset. See the API docs here: https://unidata.github.io/netcdf4-python/#in-memory-diskless-datasets

ianliu · 2023-02-27T16:27:44Z

@jhamman I think this PR is OK now, what do you think?

headtr1ck

LGTM.

Some type hints would be nice to have :)

xarray/backends/netCDF4_.py

xarray/tests/test_backends.py

This commit exposes the `memory` argument for the "netcdf4" engine, allowing one to create a netcdf dataset from a memory buffer, like so: ```python buffer: bytes = ... ds = xr.open_dataset("", engine="netcdf4", memory=buffer) ``` This commit also adds a unit test for the added feature.

headtr1ck · 2023-03-22T19:31:00Z

The error of mypy is real, but unfortunately this is a design error on our side, which is out of scope for this PR.
see the mypy message:

xarray/backends/netCDF4_.py:563: error: Signature of "open_dataset" incompatible with supertype "BackendEntrypoint" [override]

so simply add a # type: ignore[override] at the end of this line.

Adds support to save a netcdf to memory using the "netcdf4" engine: ```python ds = xr.Dataset({ "v": xr.DataArray(data=range(10)) }) buf = ds.to_netcdf(engine="netcdf4") assert xr.open_dataset("", engine="netcdf4", memory=buf).equals(ds) ```

github-actions bot added io topic-backends topic-documentation labels Aug 26, 2022

dcherian removed the topic-documentation label Aug 26, 2022

ianliu force-pushed the main branch from b9080de to 38c09dd Compare August 26, 2022 17:08

ianliu force-pushed the main branch 2 times, most recently from 2033902 to a107fb6 Compare August 26, 2022 17:37

Illviljan reviewed Aug 27, 2022

View reviewed changes

xarray/backends/netCDF4_.py Outdated Show resolved Hide resolved

ianliu force-pushed the main branch from 1ef0904 to 02a8eba Compare August 30, 2022 19:16

dcherian reviewed Oct 3, 2022

View reviewed changes

jhamman reviewed Jan 31, 2023

View reviewed changes

ianliu force-pushed the main branch 2 times, most recently from 9ee6abf to 8a4b3be Compare February 12, 2023 00:32

headtr1ck requested a review from jhamman March 10, 2023 20:45

headtr1ck approved these changes Mar 16, 2023

View reviewed changes

xarray/backends/netCDF4_.py Outdated Show resolved Hide resolved

xarray/backends/netCDF4_.py Show resolved Hide resolved

xarray/tests/test_backends.py Outdated Show resolved Hide resolved

xarray/tests/test_backends.py Outdated Show resolved Hide resolved

ianliu force-pushed the main branch from 8a4b3be to d1de6ea Compare March 21, 2023 19:08

ianliu force-pushed the main branch from d1de6ea to 082f9e7 Compare March 22, 2023 13:32

Supports saving netcdf4 to memory

fa2b97b

Adds support to save a netcdf to memory using the "netcdf4" engine: ```python ds = xr.Dataset({ "v": xr.DataArray(data=range(10)) }) buf = ds.to_netcdf(engine="netcdf4") assert xr.open_dataset("", engine="netcdf4", memory=buf).equals(ds) ```

ianliu force-pushed the main branch from 082f9e7 to fa2b97b Compare March 23, 2023 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `memory` argument for "netcdf4" engine #6956

Expose `memory` argument for "netcdf4" engine #6956

ianliu commented Aug 26, 2022 •

edited by jhamman

dcherian commented Aug 26, 2022

ianliu commented Aug 26, 2022 •

edited

dcherian commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 30, 2022

dcherian Oct 3, 2022

ianliu Oct 4, 2022

jhamman Jan 31, 2023

jhamman commented Jan 31, 2023

ianliu commented Jan 31, 2023

jhamman left a comment

jhamman Jan 31, 2023

jhamman Jan 31, 2023

ianliu commented Feb 12, 2023

ianliu commented Feb 27, 2023

headtr1ck left a comment

headtr1ck commented Mar 22, 2023

Expose memory argument for "netcdf4" engine #6956

Are you sure you want to change the base?

Expose memory argument for "netcdf4" engine #6956

Conversation

ianliu commented Aug 26, 2022 • edited by jhamman

dcherian commented Aug 26, 2022

ianliu commented Aug 26, 2022 • edited

dcherian commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 26, 2022

ianliu commented Aug 30, 2022

dcherian Oct 3, 2022

Choose a reason for hiding this comment

ianliu Oct 4, 2022

Choose a reason for hiding this comment

jhamman Jan 31, 2023

Choose a reason for hiding this comment

jhamman commented Jan 31, 2023

ianliu commented Jan 31, 2023

jhamman left a comment

Choose a reason for hiding this comment

jhamman Jan 31, 2023

Choose a reason for hiding this comment

jhamman Jan 31, 2023

Choose a reason for hiding this comment

ianliu commented Feb 12, 2023

ianliu commented Feb 27, 2023

headtr1ck left a comment

Choose a reason for hiding this comment

headtr1ck commented Mar 22, 2023

Expose `memory` argument for "netcdf4" engine #6956

Expose `memory` argument for "netcdf4" engine #6956

ianliu commented Aug 26, 2022 •

edited by jhamman

ianliu commented Aug 26, 2022 •

edited