8000 ENH: Tempfile context manager by charris · Pull Request #6866 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

ENH: Tempfile context manager #6866

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.

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

Merged
merged 2 commits into from
Dec 20, 2015
Merged

Conversation

charris
Copy link
Member
@charris charris commented Dec 19, 2015

Context manager intended for use when the same temporary file needs to be opened and closed more than once. The context manager creates the file, closes it, and returns the path to the file. On exit from the context block the file is removed. The file should be closed before
exiting the context as an error will be raised on windows if not.

Also fix up the tempdir context manager to deal with exceptions.

Tests are added for both temppath and tempdir and test_not_closing_opened_fid is refactored as an example.

Context manager intended for use when the same temporary file needs to
be opened and closed more than once. The context manager creates the
file, closes it, and returns the path to the file. On exit from the
context block the file is removed. The file should be closed before
exiting the context as an error will be raised on windows if not.

Also fix up the `tempdir` context manager to deal with exceptions.

Tests are added for both `temppath` and `tempdir`.
The test is in numpy/lib/tests/test_io.py. This commit is intended
as a demonstration of using temppath.
@charris charris force-pushed the tempfile-context-manager branch from ae085ad to c4156cf Compare December 20, 2015 14:57
@charris
Copy link
Member Author
charris commented Dec 20, 2015

This is needed by several other PRs, so merging.

charris added a commit that referenced this pull request Dec 20, 2015
@charris charris merged commit 765422c into numpy:master Dec 20, 2015
@rherault-pro
Copy link

That context manager yields a file path but not a file descriptor.
I think, relying on file path is not a good idea as you can not transparently replace an in-disk file by an in-mem file.
When one's needs an explicit path, It should be the role of the caller to get the file path by fid.name

@charris
Copy link
Member Author
charris commented Dec 20, 2015

But that file can only be opened once, and is already open. It is probably best to leave the location of the temp files up to the system administrator. It is not unusual for it to be in memory, but then, it may run out of space...

@rherault-pro
Copy link

I think we should use the same interface for all tempfile, whether in-mem or in-disk (see #6540) whether deleted on close or not.

A lot of utils can directly work on fid (thus memfile, like zip), and it can be much much much faster this way than writing tempfile on the disk. At least for np.savez, my purpose is to make it user choice even if in-disk is the default choice.

@rherault-pro
Copy link

The problem come from the fact that the base class _TemporaryFileWrapper from python tempfile module
https://github.com/python-git/python/blob/master/Lib/tempfile.py deletes the tempfile on both close or exit methods. It should only delete the file on exit to be worthy on WinNT (for multiple close/open on the named tempfile) then we have our desired context manager.

Please note the choice of python to only return fid not path even for NamedTemporaryFile context manager. So all tempfile context manager have the same interface and can be transparently replace by io.BytesIO.

Moreover, If i read the comments of tempfile python module, there can be an automatic deletion by the winnt system on closing tempfile, that will be a pity in our case. As I do not have WinNT I can not check it;

@charris charris deleted the tempfile-context-manager branch January 6, 2016 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0