issue_comments

Thanks @jhamman! @xylar, I'm thinking file_cache_maxsize=1200 would be a good default (100 years of monthly files) that could be modified via the config files we use.

@jhamman, LRU is much better. The api-change on our size is to replace autoclose=True with file_cache_maxsize=A_REASONABLE_BIG_NUMBER, correct?

I agree this could be helpful... is there any interest in reviving this stale issue?

Thanks @shoyer, I find this feature extremely useful as I keep running into use cases where I can use it. Thanks for the update, given changes to xarray it sounds like the prudent course of action is as you outline. Thanks again for the quick reply!

@shoyer, it looks like your list above is the place to start from your branch, correct?

Just to refresh here-- what needs done to finish this off?

Thanks @fmaussion and @shoyer, sorry about the duplicated issue...

Note, we don't use decode_cf=False. Does it crash without making this specification, e.g., using the default?

@rabernat, I would say that this is a bug. Is this with the scipy backend or netCDF4? Presumably if you have this problem we could run into too. For the record, we are using netCDF4.

Both cases are fixed by #1361.

@shoyer, thanks for the fix here-- sorry this fell between the cracks.

Thanks @shoyer and @jhamman! I've used this new citations for my papers in review. Awesome-- congratulations!!!

@shoyer, thanks for fixing this bug that I missed.

@mathause can you please comment on the status of this issue? Is there an associated PR somewhere? Thanks!

As it turns out this was found this hunting a more complicated bug, but I couldn't easily reproduce it via DataArrays from scratch. My guess is that solving this will most likely solve that issue:

```python ds = xr.open_dataset('mpaso.hist.0100-06-01_00000_potDensityOnly.nc') ds['latCell'] = ds.latCell*180.0/np.pi ds.set_coords('latCell', inplace=True)

potDen = ds.potentialDensity.where(ds.potentialDensity != 0, drop=True)

yieldingpython Traceback (most recent call last): File "./plot_potential_density.py", line 52, in <module> main() File "./plot_potential_density.py", line 16, in main potDen = ds.potentialDensity.where(ds.potentialDensity != 0, drop=True) File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/common.py", line 627, in where outobj = self.sel(indexers) File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/dataarray.py", line 672, in sel result = self.isel(drop=drop, pos_indexers) File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/dataarray.py", line 657, in isel ds = self._to_temp_dataset().isel(drop=drop, indexers) File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/dataset.py", line 1119, in isel new_var = var.isel(var_indexers) File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/variable.py", line 548, in isel return self[tuple(key)] File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/variable.py", line 378, in getitem values = self._indexable_data[key] File "/Users/pwolfram/anaconda/lib/python2.7/site-packages/xarray/core/indexing.py", line 423, in getitem return type(self)(self.array[key]) IndexError: shape mismatch: indexing arrays could not be broadcast together with shapes (1,) (235446,) (100,) ```

It is possible that ds.set_coords('latCell', inplace=True) is part of the problem for this bug, so it may actually be a few problems, not just one.

Thanks @shoyer!

@shoyer, all checks pass and this is ready for a review / merge when you have time.

@shoyer, changes have been made as requested. Thanks!

@shoyer, the bug fix note has now been added and commits merged! Thank you!

@shoyer, tests should be restarted following merge of #1336 and this PR should be ready to merge.

@shoyer the changes have been made and this should be ready to merge now.

This also fixes the issue noted in https://github.com/pydata/xarray/pull/1038 where flakey tests cause travis CI failure.

@shoyer, as we discussed, here is a robustness-ing of the testing as needed for 0.9.2

@shoyer, note that we probably need this fix to be included in the 0.9.2 release.

Thanks @shoyer, the test stub is at https://github.com/pydata/xarray/pull/1342 and hopefully CI will give some more useful information.

@shoyer, is there any easy way to initialize python <xarray.DataArray (nCells: 0, nVertLevels: 10)> array([], shape=(0, 10), dtype=float64) Dimensions without coordinates: nCells, nVertLevels so that I can write a clean test for this error?

The script (e.g., on 3.5) should return something like ```python In [1]: import xarray as xr

In [2]: import numpy as np

In [3]: da = xr.DataArray(np.random.rand(100,10), dims=['nCells','nVertLevels'])

In [4]: mask = xr.DataArray(np.zeros((100,), dtype='bool'), dims='nCells')

In [5]: da.where(mask, drop=True)

Out[6]: <xarray.DataArray (nCells: 0, nVertLevels: 10)> array([], shape=(0, 10), dtype=float64) Dimensions without coordinates: nCells, nVertLevels ```

Thanks @shoyer. This is what I was thinking but I just wanted to double check. Feel free to close the issue if you would like- I'll move this to dask issues.

@shoyer, is there anything else you would like done on this before merging?

@shoyer, all tests pass and I've spot checked that this works as expected, e.g., ```python xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_1_autoclose_netcdf4 PASSED

xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_1_open_large_num_files_netcdf4 SKIPPED

xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_2_autoclose_scipy PASSED

xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_2_open_large_num_files_scipy SKIPPED

xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_3_autoclose_pynio PASSED

xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_3_open_large_num_files_pynio SKIPPED ```

@shoyer et al, please feel free to mark additional tests as slow in this PR. I only marked the ones from #1198 that were too slow for this round.

See #1336 for a fix that disables these tests that have been acting up because of resource issues.

cc @MaximilianR

@shoyer, this PR should also fix the issue with travis CI failing due to testing of too many open files. Note, when a user runs py.test locally, the slow tests will run. They can be skipped via running py.test --skip-slow.

@rabernat, do you think that the proposed keyword additions should be included in xarray or not? I personally would like to see them in xarray but don't know if it is just me or not. If you think they should be in xarray, are you ok with the api above?

@shoyer, I'm not sure we want to wrap np.gradient. It seems like other approaches like @rabernat 's xgcm would be more appropriate as a superset of xarray.

Fundamentally, I want something that is like an inverse of cumsum and the proposed change could be used in that context. It is just super inconvenient to do array resizing following the diff of a time vector to get timesteps, but maybe this use case is too niche to be useful for the community.

Thanks @fmaussion, it just seems strange that if the data is one-dimensional we return an error. I would agree that we probably want an error for dimensionality larger than one. I think the thing to change here is to make da.cumsum() work for one-dimensional da. But, it may not be worth the effort to fix this issue relative to other issues.

Crash in the same place... but when I restarted it via a force push earlier it passed, which would imply we are running out of resources on travis.

Maybe the thing to do is just to do a reset on the open file limit as @rabernat suggested, this way it provides a factor of safety on travis.

Thoughts on this idea @shoyer and @fmaussion?

Is it possible that the test fails if more than one is simultaneously run on the same node? Could you restart the other tests to verify (restart at the same time if possible).

Still passing locally... xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_1_autoclose_netcdf4 PASSED xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_1_open_large_num_files_netcdf4 PASSED xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_2_autoclose_scipy PASSED xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_2_open_large_num_files_scipy PASSED xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_3_autoclose_pynio PASSED xarray/tests/test_backends.py::OpenMFDatasetManyFilesTest::test_3_open_large_num_files_pynio PASSED Test passes even if I run it multiple times too.

I'm continuing to take a look-- my tests were not 100% set up locally on this branch and I'll see if I can reproduce the sporadic error on macOS.

@shoyer, should I do a quick "hot fix" and then try to sort out the problem?

It happened here too... I just tried it out on my local machine via conda env create -f ci/requirements-py27-cdat+pynio.yml and wasn't able to get an error... are any of the crashes better then a "seg fault"?

@shoyer, added a test as requested.

I think we can close this because 0.9.0 has been released.

@ajoros should correct me if I'm wrong but it sounds like everything is working for his use case.

@ajoros, #1198 was just merged so the bleeding-edge version of xarray is the one to try!

Thanks a bunch @shoyer!

@ajoros, can you try something like pip -v install --force git+ssh://git@github.com/pwolfram/xarray@fix_too_many_open_files to see if #1198 fixes your problem with your dataset, noting that you need open_mfdataset(..., autoclose=True)?

@shoyer should correct me if I'm wrong but we are almost ready to merge the code in this PR and this would be a great "in the field" check if you could try it out soon.

As long as we can explicitly obtain the attrs data if necessary truncating the data for the repr makes sense. Note that appending a set of characters like ... to the end of it would be useful to indicate that the string continues but is only partially displayed.

@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?

Provided checks pass this should be ready to merge @fmaussion unless @shoyer has any additional recommended changes.

Note, I would say that open_mfdataset is no longer experimental because of its widespread use.

Note, this issue should be resolvable via #924.

@fmaussion and @shoyer, I'd like to close this PR out if possible. I'm not 100% sure this PR is worthwhile to complete in a general fashion because of the ambiguity in how to best handle this issue. My current take on this would be to go with whatever is simplest / cleanest, at least in the short term, which is @fmaussion's suggestion above. Does this work for you both?

We are very close on #1198 and will be merging soon. This would be a great time for everyone to ensure that #1198 resolves this issue before we merge.

@mrocklin and @shoyer, we now have dask.distributed and xarray support. Should this issue be closed?

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?

Thanks!

@shoyer, this seems pretty straight forward from http://doc.pytest.org/en/latest/example/simple.html#control-skipping-of-tests-according-to-command-line-option-- all we need is to define the @slow decorator and mark the slow tests, correct? This seems like a fast PR write-up / fulfill following merge of #1198 unless I'm missing something.

@shoyer, should this PR be merged before #1198, just in case to make sure that there aren't any "gotchas" that I've missed in #1198?

@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!

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?

@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?

@RafalSkolasinski and @shoyer, can I please get an update on this PR? This is something we need sometime soon too (cc @milenaveneziani).

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

We would also benefit from this specifically for #1198 :+1:

@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 ```

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

@JoyMonteiro and @shoyer, as I've been thinking about this more and especially regarding #463, I was planning on building on opener from #1128 to essentially open, read, and then close a file each time a read get operation was needed on a newCDF file. My initial view was that output fundamentally would be serial but as @JoyMonteiro points out, there may be a benefit to making a provision for parallel output. However, we will probably run into the same netCDF limitation on the number of open files. Would we want similar functionality on opener for set as well as the get methods? I'm not sure how something like sync would work in this context and suspect this could lead to problems. Presumably we would be requiring writing each dimension, attribute, variable, etc at each call with its own associated open, write, and close. I obviously need to find the time to dig into this further...

@shoyer, I think this is really great and appreciate you doing this awesome work. I obviously have a vested interest in the drop=True capability because we have found great use for it via where. My vote is go for it!

I just realized I didn't say thank you to @shoyer et al for the advice and help. Please forgive my rudeness.

@shoyer, if I understand correctly the best approach as you see it to build on opener via #1128, recognizing this will be essentially "upgraded" sometime in the future, right?