Close files when CachingFileManager is garbage collected #2595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello @shoyer! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on December 23, 2018 at 19:51 Hours UTC |
Fixes GH2560 This frees users from needing to worry about this.
shoyer
force-pushed
the
file-manager-del
branch
from
b7b5cd5 to
be7cd46
Compare
|
Of course everything runs fine either on my laptop (Macbook) or work machine (Linux). I wonder what's going on with Travis-CI here. |
|
@pydata/xarray It would be great if someone else could take a look at this, though certainly thread-safety and garbage collection are slightly painful concepts to think deeply about. If nobody reviews this, I will probably self-merge in a few days. |
jhamman
reviewed
Dec 15, 2018
| # - threading.Lock on Python 2. | ||
| # - dask.SerializableLock with dask v1.0.0 or earlier. | ||
| # - multiprocessing.Lock calls the argument "block" instead. | ||
| return lock.acquire(blocking) |
There was a problem hiding this comment.
Given the date, do you think its useful here to consider this Python 2 work around? I guess its the same work-around as the dask/multiprocessing issue.
|
Yeah, it's not really any additional trouble. I plan to include this in a
v0.11.1 release, which will be the last release to support Python 2.
…On Fri, Dec 14, 2018 at 4:12 PM Joe Hamman ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xarray/backends/locks.py
<#2595 (comment)>:
> + Includes backwards compatibility hacks for old versions of Python, dask
+ and dask-distributed.
+ """
+ if blocking:
+ # no arguments needed
+ return lock.acquire()
+ elif DistributedLock is not None and isinstance(lock, DistributedLock):
+ # distributed.Lock doesn't support the blocking argument yet:
+ # dask/distributed#2412
+ return lock.acquire(timeout=0)
+ else:
+ # "blocking" keyword argument not supported for:
+ # - threading.Lock on Python 2.
+ # - dask.SerializableLock with dask v1.0.0 or earlier.
+ # - multiprocessing.Lock calls the argument "block" instead.
+ return lock.acquire(blocking)
Given the date, do you think its useful here to consider this Python 2
work around? I guess its the same work-around as the dask/multiprocessing
issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2595 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1iwt7OmehxX6coNie-LtSI3YJnhhks5u5D57gaJpZM4ZJ6GX>
.
|
|
OK, I'm actually going to merge soon (and issue the 0.11.1 release) unless I hear any objections... |
shoyer
force-pushed
the
file-manager-del
branch
from
4719ce6 to
b2bb60e
Compare
dcherian
pushed a commit
to yohai/xarray
that referenced
this pull request
Jan 2, 2019
* master: DEP: drop python 2 support and associated ci mods (pydata#2637) TST: silence warnings from bottleneck (pydata#2638) revert to dev version DOC: fix docstrings and doc build for 0.11.1 Source encoding always set when opening datasets (pydata#2626) Add flake check to travis (pydata#2632) Fix dayofweek and dayofyear attributes from dates generated by cftime_range (pydata#2633) silence import warning (pydata#2635) fill_value in shift (pydata#2470) Flake fixed (pydata#2629) Allow passing of positional arguments in `apply` for Groupby objects (pydata#2413) Fix failure in time encoding for pandas < 0.21.1 (pydata#2630) Fix multiindex selection (pydata#2621) Close files when CachingFileManager is garbage collected (pydata#2595) added some logic to deal with rasterio objects in addition to filepaths (pydata#2589) Get 0d slices of ndarrays directly from indexing (pydata#2625) FIX Don't raise a deprecation warning for xarray.ufuncs.{angle,iscomplex} (pydata#2615) CF: also decode time bounds when available (pydata#2571)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This frees users from needing to worry about this.
Using
__del__turned up to be easier than using weak references.whats-new.rstfor all changes andapi.rstfor new AP