ENH: switch to 'with' statement for open/close file in lib.npyio._savez #6545
Conversation
268637a to
a53671c
Compare
| fd, tmpfile = tempfile.mkstemp(suffix='-numpy.npy') | ||
| os.close(fd) | ||
| try: | ||
| with zipfile_factory(file, mode="w", compression=compression) as zipf: |
There was a problem hiding this comment.
I don't think your context handler is exception safe, so either needs a fix or this should still be in a try..except clause.
There was a problem hiding this comment.
OK, exception handler will reraise.
There was a problem hiding this comment.
As the exception handler was only here to remove/close files in the original version, no need to add try/except mechanism to the with statement.
This this exactly the case given in the "Motivation and summary" section of pep-0343 .
7aabdd6 to
7397544
Compare
|
I've got a problem with test_load_refcount that randomly fails even on the master branch So sometimes Travis check fails or not with the same exact code. |
7397544 to
74d9552
Compare
| @@ -599,13 +611,41 @@ def savez_compressed(file, *args, **kwds): | |||
| """ | |||
| _savez(file, args, kwds, True) | |||
|
|
|||
| @contextlib.contextmanager | |||
There was a problem hiding this comment.
Maybe something simpler?
@contextmanager
def TempFileName(suffix='', prefix='tmp', dir=None):
fid, name = mkstemp(suffix, prefix, dir)
os.close(fid)
yield name
os.remove(name)
This is a common pattern in a lot of tests, e.g, in test_io.py
There was a problem hiding this comment.
Nb that that's not the right way to write a context manager -- if the with
block raises an exception then the yield will raise that exception and
os.remove won't be called, you still need a try/finally.
On Nov 5, 2015 2:03 PM, "Charles Harris" notifications@github.com wrote:
In numpy/lib/npyio.py
#6545 (comment):@@ -599,13 +611,41 @@ def savez_compressed(file, _args, *_kwds):
"""
_savez(file, args, kwds, True)Maybe something simpler?
@contextmanager
def TempFileName(suffix='', prefix='tmp', dir=None):
fid, name = mkstemp(suffix, prefix, dir)
os.close(fid)
yield name
os.remove(name)This is a common pattern in a lot of tests, e.g, in test_io.py
—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/6545/files#r44076176.
There was a problem hiding this comment.
OK, same thing rewritten as an official context manager ;)
There was a problem hiding this comment.
@njsmith I believe that for real exception handlers __exit__ is called, then the exception is reraised.
There was a problem hiding this comment.
we also often just use a temporary directory for this situation, a context manager for that is pretty simple (and also already exists in python3.x) we really should have it in our test utils
There was a problem hiding this comment.
oh we already have one but its not correct ...
There was a problem hiding this comment.
@juliantaylor See also the discussion at #6660.
There was a problem hiding this comment.
Isn't this also a reasonable place to use contextlib.closing, instead of creating your own class? This seems like exactly the situation that function / decorator exists...
(PS sorry for the double comment, the last one was in the wrong place)
There was a problem hiding this comment.
@wackywendell not here but for the zipfile, yes
There was a problem hiding this comment.
Right, sorry, I don't know why I'm having so much trouble putting my comments on the right line.
| with tempfile.NamedTemporaryFile(*args, **local_kwds) as fid: | ||
| filename = fid.name | ||
| yield fid | ||
| try: |
There was a problem hiding this comment.
hm where is the os.close?
There was a problem hiding this comment.
The os.close is automatically performed by the context manager of NamedTemporaryFile at the exit of the with statement (here before line 633).
The whole pain came from the fact that NamedTemporaryFile is a factory function not a class, so you can not subclass it.
There was a problem hiding this comment.
What happens when an error is raised from yield?
There was a problem hiding this comment.
If you mean by the caller of DeleteOnContextExitNamedTemporaryFile before the return of "yield fid" then it calls the exit method of fid. That is the purpose of using the context manager NamedTemporaryFile. See
https://www.python.org/dev/peps/pep-0343
The exit method will close but not delete the file.
After that our context manager takes the relay and deletes the file.
There was a problem hiding this comment.
Exit method of NamedTemporaryFile, which closes the file but doesn't delete it. The __exit__ method will then reraise the error and you will not catch it.
There was a problem hiding this comment.
OK, I've described the good way of doing it by rewritting class _TemporaryFileWrapper from tempfile.py on my last comment on #6866
There was a problem hiding this comment.
I think you'd want ExitStack here to implement this properly
f9a31c2 to
0583fd8
Compare
|
I've committed a |
|
Thank you, but as I have replied on #6866, the next step is to replace in-disk tempfile by possibly in-mem tempfile (at user choice) for npz saving; returning a file path prevents that. |
Note that we are dropping Python 2.6 after 1.11 is branched, so it might make sense to postpone this until then so that the 2.6 code can be removed. |
|
Yes of course less compat code better it is |
0583fd8 to
4a18bf7
Compare
|
☔ The latest upstream changes (presumably #7133) made this pull request unmergeable. Please resolve the merge conflicts. |
4a18bf7 to
c1aa2d8
Compare
The function _savez stages data during archiving through temporary files. Before that commit these temp and zip files were handled through old open/close style. This commits switches to 'with' statement. which is the recommendation since python 2.5. It's simplifying A LOT exception handling and garbage collection. Due to uncompatibility between zipfile in python2.6 and with statement, ZipFile class has been subclassed to add needed stub methods. BUG: WinNT prevents a file to be opened twice Using contextlib.closing rather than own class
c1aa2d8 to
7d0bca1
Compare
|
See also #9510. |
|
OK, going to close this. Python 3.6 allows 8A15 writing directly to the zipfile with no temporary file needed, so I think that problem will fix itself. For lesser versions of Python we can live with it. The @rherault-insa Thanks for the PR, even if it didn't get in it led to some useful discussion. |
The function _savez stages data during archiving through temporary
files. Before that commit these temp and zip files were handled through
old open/close style. This commits switches to 'with' statement.
which is the recommendation since python 2.5. It's simplifying A LOT
exception handling and garbage collection.
Due to uncompatibility between zipfile in python2.6 and with statement,
ZipFile class has been subclassed to add needed stub methods.