issue_comments

Thanks a bunch @shoyer!

@shoyer, all tests (including coveralls) passed. Please let me know if you have additional concerns and if we could merge fairly soon, e.g., because of https://github.com/MPAS-Dev/MPAS-Analysis/issues/151 I would really appreciate it.

@shoyer, this is ready for the final review now. Coveralls appears to have hung but other tests pass.

@shoyer, I had a minor bug that is now removed. The last caveat no longer applicable:

The scipy backend can handle objects like BytesIO that really aren't file handles and there doesn't appear to be a clean way to close these types of objects. So, at present I'm explicitly setting _autoclose=False if they are encountered in the datastore. If this needs to be changed, particularly since it doesn't affect existing behavior, I'd prefer this be resolved in a separate issue / PR if possible.

I'll let you know when tests pass and this is ready for your final review.

@shoyer, that subclass-based approach you outlined worked (fixture parameters really don't work with classes as far as I could tell). We now have more comprehensive, named testing. Note, there was one minor point that required more explicitly specification that arose from the more rigorous testing:

The scipy backend can handle objects like BytesIO that really aren't file handles and there doesn't appear to be a clean way to close these types of objects. So, at present I'm explicitly setting _autoclose=False if they are encountered in the datastore. If this needs to be changed, particularly since it doesn't affect existing behavior, I'd prefer this be resolved in a separate issue / PR if possible.

@shoyer, if we generally cover test_backends for autoclose=True, then we should get the pickle testing for free: xarray/tests/test_backends.py:181: def test_pickle(self): xarray/tests/test_backends.py:191: def test_pickle_dataarray(self): xarray/tests/test_backends.py:792: def test_bytesio_pickle(self): or was there some other test that is needed?

Thanks @shoyer. Ok, so we pass checks following merge of #1311. Is there anything else that we need to do on this PR prior to merging?

@shoyer, we can roll back the squash if you want because for whatever reason my interactive rebase removed your name from tagged to the commit too, which is not what I expected. This is obviously suboptimal and the choice of having two commits versus one is up to you. I think the commit prior to the squash was 9c274eb.

The tests passed so I will squash the commits...

@shoyer, all tests are passing now following your recommended edits.

@shoyer- can you please have another look following edits as you requested?

Note: tests pass before submitting changes.

They did- thank you! I'll take a quick look at your comments now so we can hopefully get this merged soon.

@shoyer, do you mind restarting the Travis-CI?

Awesome, thanks @vnoel for testing this for us all on your dataset!

with that keyword the open_mfdataset goes through ! my code crashes later because the data is too big to fit in memory, but that's another problem ;-)

Is something wrong with travis-ci right now? It doesn't look like it is running the tests...

Note, the force push was just to get travis-ci to "reboot". There was no real change to the code.

Note, if we include 482ef54 for resource-limited testing we will need to do something special for windows on appveyor. We could skip this in the short-term to make this merge and deal with these types of issues in #1309, which is what probably makes the most sense to help keep the scope of this PR more limited.

@vnoel, did you use autoclose=True? Can you please share the code snippet you used to instantiate this?

Hey @pwolfram, I installed your branch and tried to open_mfdataset on 1474 hdf5 files.

I got the following: File "/users/noel/.conda/envs/python3/lib/python3.6/site-packages/xarray/backends/api.py", line 524, in open_mfdataset File "/users/noel/.conda/envs/python3/lib/python3.6/site-packages/xarray/backends/api.py", line 524, in <listcomp> File "/users/noel/.conda/envs/python3/lib/python3.6/site-packages/xarray/backends/api.py", line 299, in open_dataset File "/users/noel/.conda/envs/python3/lib/python3.6/site-packages/xarray/backends/netCDF4_.py", line 203, in __init__ File "/users/noel/.conda/envs/python3/lib/python3.6/site-packages/xarray/backends/netCDF4_.py", line 178, in _open_netcdf4_group File "netCDF4/_netCDF4.pyx", line 1848, in netCDF4._netCDF4.Dataset.__init__ (netCDF4/_netCDF4.c:13992) OSError: Too many open files

@rabernat, I suspect you have some datasets that could be used to stress test this PR too... thanks for the help and advice yesterday.

@PeterDSteinberg and @vnoel, do you mind stress-testing this PR to make sure all works as expected? You can install easily via pip with pip -v install git+ssh://git@github.com/pwolfram/xarray@fix_too_many_open_files

@shoyer, I've collapsed all the commits to a single commit. At this point I would say this meets the scope of this PR and it should be ready to merge once CI finishes its check and it passes. Please feel free to take another pass on the code for changes that you would like to see made.

I obviously need to squash prior to the merge, however...

@shoyer, this should essentially be ready to go now with the exception that I didn't come to a clean resolution on testing the too many open files issue via resource. Thoughts on this issue?

@shoyer, I'd like to see this code integrated if possible. Should we just disable autoclose for h5netcdf and merge?

@shoyer, I'll leave the ball in your court for now but am happy to take another look before we go the disabling autoclose route for h5netcdf. Please let me know when/if you'd like me to take another look.

@shoyer, it is quite possible the error is on my end because the error message implies that modification of the data prior to closing it may be the issue. However, I'm not getting that type of an error following a similar philosophy for the other backends, which is confusing.

Do you have advice on hunting this bug further? You obviously have more experience with h5py and h5netcdf than I do.

@shoyer, the pushed code represents my progress. The initial PR had a bug-- essentially a calculation couldn't be performed following the load. This fixes that bug and provides a test to ensure that this doesn't happen. However, I'm having trouble with h5netcdf, which I'm not very familiar with compared to netcdf. This represents my current progress, I just need some more time (or even inspiration from you) to sort out this last key issue...

I'm getting the following error:

```bash ================================================================================================================== FAILURES ================================================================================================================== _____________ OpenMFDatasetTest.test_4_open_large_num_files_h5netcdf _____________

self = <xarray.tests.test_backends.OpenMFDatasetTest testMethod=test_4_open_large_num_files_h5netcdf>

@requires_dask
@requires_h5netcdf
def test_4_open_large_num_files_h5netcdf(self):

  self.validate_open_mfdataset_large_num_files(engine=['h5netcdf'])

xarray/tests/test_backends.py:1040:

xarray/tests/test_backends.py:1018: in validate_open_mfdataset_large_num_files self.assertClose(ds.foo.sum().values, np.sum(randdata)) xarray/core/dataarray.py:400: in values return self.variable.values xarray/core/variable.py:306: in values return as_array_or_item(self._data) xarray/core/variable.py:182: in _as_array_or_item data = np.asarray(data) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/numpy/core/numeric.py:482: in asarray return array(a, dtype, copy=False, order=order) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/array/core.py:1025: in __array__ x = self.compute() ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/base.py:79: in compute return compute(self, kwargs)[0] ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/base.py:179: in compute results = get(dsk, keys, kwargs) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:537: in get_sync raise_on_exception=True, kwargs) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:500: in get_async fire_task() ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:476: in fire_task callback=queue.put) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:525: in apply_sync res = func(*args, kwds) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:268: in execute_task result = _execute_task(task, data) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:248: in _execute_task args2 = [_execute_task(a, cache) for a in args] ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:248: in <listcomp> args2 = [_execute_task(a, cache) for a in args] ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:245: in _execute_task return [_execute_task(a, cache) for a in arg] ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:245: in <listcomp> return [_execute_task(a, cache) for a in arg] ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/async.py:249: in _execute_task return func(*args2) ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/dask/array/core.py:52: in getarray c = a[b] xarray/core/indexing.py:401: in getitem return type(self)(self.array[key]) xarray/core/indexing.py:376: in getitem return type(self)(self.array, self._updated_key(key)) xarray/core/indexing.py:354: in _updated_key for size, k in zip(self.array.shape, self.key): xarray/core/indexing.py:364: in shape for size, k in zip(self.array.shape, self.key): xarray/core/utils.py:414: in shape return self.array.shape xarray/backends/netCDF4.py:37: in getattr return getattr(self.datastore.ds.variables[self.var], attr) ../../anaconda/envs/test_env_xarray35/lib/python3.5/contextlib.py:66: in exit next(self.gen) xarray/backends/h5netcdf_.py:105: in ensure_open self.close() xarray/backends/h5netcdf_.py:190: in close close_ds(self.ds) xarray/backends/h5netcdf.py:70: in _close_ds find_root(ds).close() ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/h5netcdf/core.py:458: in close self._h5file.close() ../../anaconda/envs/test_env_xarray35/lib/python3.5/site-packages/h5py/_hl/files.py:302: in close self.id.close() h5py/_objects.pyx:54: in h5py._objects.with_phil.wrapper (/Users/travis/miniconda3/conda-bld/work/h5py-2.6.0/h5py/_objects.c:2840) ??? h5py/_objects.pyx:55: in h5py._objects.with_phil.wrapper (/Users/travis/miniconda3/conda-bld/work/h5py-2.6.0/h5py/_objects.c:2798) ??? h5py/h5f.pyx:282: in h5py.h5f.FileID.close (/Users/travis/miniconda3/conda-bld/work/h5py-2.6.0/h5py/h5f.c:3905) ??? h5py/_objects.pyx:54: in h5py._objects.with_phil.wrapper (/Users/travis/miniconda3/conda-bld/work/h5py-2.6.0/h5py/_objects.c:2840) ??? h5py/_objects.pyx:55: in h5py._objects.with_phil.wrapper (/Users/travis/miniconda3/conda-bld/work/h5py-2.6.0/h5py/_objects.c:2798) ???

??? E RuntimeError: dictionary changed size during iteration

h5py/_objects.pyx:119: RuntimeError ============================================================================================ 1 failed, 1415 passed, 95 skipped in 116.54 seconds ============================================================================================= Exception ignored in: <function WeakValueDictionary.__init__.\<locals>.remove at 0x10f16e598> Traceback (most recent call last): File "/Users/pwolfram/anaconda/envs/test_env_xarray35/lib/python3.5/weakref.py", line 117, in remove TypeError: 'NoneType' object is not callable ```

I'm just chiming in to signify my interest in seeing this issue solved. I have just hit "OSError: Too many open files". The data itself is not even huge, but it's scattered across many files and it's a PITA to revert to manual concatenation -- I've grown used to dask doing the work for me ;-)

There are still a few more issues that need ironed out. I'll let you know when I've resolved them.

@shoyer and @PeterDSteinberg I've updated this PR to reflect requested changes.

@PeterDSteinberg, did this PR fix the issue for you? I obviously need to update it but just wanted to confirm that the current branch resolved the too-many-open files error issue. Also, do you have any idea of the performance impact of these changes I'm proposing?

Thanks @shoyer. This makes sense. I think the path forward on the next round of edits should include making sure existing tests using open_mfdataset use both options to autoclose. If we do this we could future-proof ourselves against loss due to accidental breaking of this new functionality and avoid potentially contaminating existing workflows via performance concerns.

Documentation is also obviously required.

FYI as a heads up, I probably won't be able to get to this mid-week at the earliest but it appears we are close to a viable solution.

@shoyer, I just realized this might conflict with #1087. Do you foresee this causing problems and what order do you plan to merge this PR and #1087 (which obviously predates this one...)? We are running into the snag with #463 in our analysis and my personal preference would be to get some type of solution into place sooner than later. Thanks for considering this request.

Also, I'm not sure exactly the best way to test performance either. Could we potentially use something like the "toy" test cases for this purpose? Ideally we would have a test case with O(100) files to gain a clearer picture of the performance cost of this PR.

Please let me know what you want me to do with this PR-- should I clean it up in anticipation of a merge or just wait for now to see if there are extra things that need fixed via additional testing? Note I have the full scipy, h5netcdf and pynio implementations that can also be reviewed because they weren't available when you did your review yesterday.

Thanks @shoyer, Does that mean if the checks pass the code is at least minimally correct in terms of not breaking previous design choices? E.g., does this imply that we are ok except for cleanup / implementation details on this PR?

@shoyer, all the checks "pass" but there are still errors in the "allowed" list. If you get a change could you please provide me some perspective on whether these are errors on my end or not? I'm not exactly sure how to interpret them.

Once I know I have correctness in this code I plan to fix the inlines you graciously highlighted above. I think we are getting close here, assuming that I have enough testing to demonstrate we have accurately fixed the too many open file issue. Any additional ideas you have for tests would be really helpful too.

The intent of this PR is to address (or at least partially address) the following issues:

• https://github.com/pydata/xarray/issues/463
• https://github.com/pydata/xarray/issues/798
• https://github.com/CCI-Tools/cate-core/issues/102
• https://github.com/MPAS-Dev/MPAS-Analysis/issues/49