8000 ENH: switch to 'with' statement for open/close file in lib.npyio._savez by rherault-pro · Pull Request #6545 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: switch to 'with' statement for open/close file in lib.npyio._savez #6545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

ENH: switch to 'with' statement for open/close file in lib.npyio._savez #6545

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 40 additions & 25 deletions numpy/lib/npyio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import sys
import os
import os.path
import contextlib
import re
import itertools
import warnings
Expand Down Expand Up @@ -599,13 +601,41 @@ def savez_compressed(file, *args, **kwds):
"""
_savez(file, args, kwds, True)

@contextlib.contextmanager
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

+@contextlib.contextmanager

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, same thing rewritten as an official context manager ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njsmith I believe that for real exception handlers __exit__ is called, then the exception is reraised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh we already have one but its not correct ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliantaylor See also the discussion at #6660.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wackywendell not here but for the zipfile, yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry, I don't know why I'm having so much trouble putting my comments on the right line.

def DeleteOnContextExitNamedTemporaryFile(*args, **kwds):
""" Factory for a NamedTemporaryFile that is
deleted on context exit but not on file close.

Usefull replacement for NamedTemporaryFile,
as WinNT prevents a file to be opened twice.

Typical case:
with DeleteOnExitNamedTemporaryFile() as fid:
foo(fid)
fid.close()
# File not deleted here
bar(fid.name)
# File deleted here
"""
# Import deferred for startup time improvement
import tempfile
local_kwds = kwds.copy()
local_kwds['delete'] = False
filename = None
with tempfile.NamedTemporaryFile(*args, **local_kwds) as fid:
filename = fid.name
yield fid
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm where is the os.close?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when an error is raised from yield?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 conte 8000 xt manager takes the relay and deletes the file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've described the good way of doing it by rewritting class _TemporaryFileWrapper from tempfile.py on my last comment on #6866

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd want ExitStack here to implement this properly

if os.path.exists(filename):
os.unlink(filename)
except OSError:
pass


def _savez(file, args, kwds, compress, allow_pickle=True, pickle_kwargs=None):
# Import is postponed to here since zipfile depends on gzip, an optional
# component of the so-called standard library.
import zipfile
# Import deferred for startup time improvement
import tempfile

if isinstance(file, basestring):
if not file.endswith('.npz'):
Expand All @@ -624,35 +654,20 @@ def _savez(file, args, kwds, compress, allow_pickle=True, pickle_kwargs=None):
else:
compression = zipfile.ZIP_STORED

zipf = zipfile_factory(file, mode="w", compression=compression)

# Stage arrays in a temporary file on disk, before writing to zip.

# Since target file might be big enough to exceed capacity of a global
# temporary directory, create temp file side-by-side with the target file.
file_dir, file_prefix = os.path.split(file) if _is_string_like(file) else (None, 'tmp')
fd, tmpfile = tempfile.mkstemp(prefix=file_prefix, dir=file_dir, suffix='-numpy.npy')
os.close(fd)
try:
# Context manager compatible with the 'with' statement
# In python => 2.7 ZipFile class has been corrected
# No more need of contextlib.closing
with contextlib.closing(
zipfile_factory(file, mode="w", compression=compression)) as zipf:
for key, val in namedict.items():
fname = key + '.npy'
fid = open(tmpfile, 'wb')
try:
with DeleteOnContextExitNamedTemporaryFile() as fid:
fname = key + '.npy'
format.write_array(fid, np.asanyarray(val),
allow_pickle=allow_pickle,
pickle_kwargs=pickle_kwargs)
fid.close()
fid = None
zipf.write(tmpfile, arcname=fname)
except IOError as exc:
raise IOError("Failed to write to %s: %s" % (tmpfile, exc))
finally:
if fid:
fid.close()
finally:
os.remove(tmpfile)
zipf.write(fid.name, arcname=fname)

zipf.close()


def _getconv(dtype):
Expand Down
0