Skip to content
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

TST: Use tempfiles in all tests. #5419

Closed

Conversation

@jtratner
Copy link
Contributor

commented Nov 3, 2013

If you really need to, possible to opt-out with make_tempfile=False.
Something weird with HDF where, even though it's supposed to be
removing the file every time, it causes issues to use a real temp
file... Not sure why (maybe I missed something there).

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

why r u changing the hdf stuff? this is specifically designed to guarantee the file removal

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

Problem is that it should be creating the files in the temp directory rather than the current directory (like it is doing on master) - tried to add that in to hdf tests but somehow it wasn't working.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

isn't that what make temp file is supposed to do?

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

Yes, but if you passed an actual filename, it didn't create a temp file.

On Sat, Nov 2, 2013 at 9:13 PM, jreback notifications@github.com wrote:

isn't that what make temp file is supposed to do?


Reply to this email directly or view it on GitHubhttps://github.com//pull/5419#issuecomment-27636563
.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

maybe I messed something up with hdf.

On Sat, Nov 2, 2013 at 9:13 PM, Jeffrey Tratner
jeffrey.tratner@gmail.comwrote:

Yes, but if you passed an actual filename, it didn't create a temp file.

On Sat, Nov 2, 2013 at 9:13 PM, jreback notifications@github.com wrote:

isn't that what make temp file is supposed to do?


Reply to this email directly or view it on GitHubhttps://github.com//pull/5419#issuecomment-27636563
.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

ok let me have a look tom

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

I think you just need to check in ensure_clean if the path is a filename, if so then put it in a temp dir, but use that filename (otherwise generate both)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

@jreback that's what I did (just by passing in as suffix). I'll check again - maybe there's a part that uses ensure_clean twice or something:

import tempfile
In [4]: a = tempfile.mkstemp()[1]

In [5]: b = tempfile.mkstemp(suffix='apples!')[1]

In [6]: a
Out[6]: '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/tmp7oEhoT'

In [7]: b
Out[7]: '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/tmphkXUEKapples!'

In [8]: b = tempfile.mkstemp(suffix='apples!', prefix='red')[1]

In [9]: b
Out[9]: '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/redqbTnB_apples!'

In [10]:
@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

seems odd that u have to pass the make_tempfile=False in test_pytables just check if the passed filename is a a full pathname

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

@jreback the point isn't to maintain the file path, it's that all the files we create for testing need to go into the temp directory. I added that make_tempfile=False part to work around pytables tests (otherwise they get littered everywhere). For example, I get this error when I try to use it with tempfile:

ERROR: pandas.io.tests.test_pytables:TestHDFStore.test_mode
  /usr/local/bin/vim +412 pandas/io/tests/test_pytables.py  # test_mode
    check('r')
  /usr/local/bin/vim +377 pandas/io/tests/test_pytables.py  # check
    self.assertRaises(IOError, HDFStore, path, mode=mode)
  /usr/local/bin/vim +475 ../python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py  # assertRaises
    callableObj(*args, **kwargs)
  /usr/local/bin/vim +384 pandas/io/pytables.py  # __init__
    self.open(mode=mode)
  /usr/local/bin/vim +504 pandas/io/pytables.py  # open
    self._handle = h5_open(self._path, self._mode)
  /usr/local/bin/vim +230 pandas/io/pytables.py  # h5_open
    return tables.openFile(path, mode)
  /usr/local/bin/vim +35  ../python2.7/site-packages/tables/_past.py  # oldfunc
    return obj(*args, **kwargs)
  /usr/local/bin/vim +251 ../python2.7/site-packages/tables/file.py  # open_file
    return File(filename, mode, title, root_uep, filters, **kwargs)
  /usr/local/bin/vim +527 ../python2.7/site-packages/tables/file.py  # __init__
    self._g_new(filename, mode, **params)
  /usr/local/bin/vim +463 hdf5extension.pyx  # tables.hdf5extension.File._g_new (tables/hdf5extension.c:4284)
    None
HDF5ExtError: HDF5 error back trace

  File "H5F.c", line 1582, in H5Fopen
    unable to open file
  File "H5F.c", line 1373, in H5F_open
    unable to read superblock
  File "H5Fsuper.c", line 334, in H5F_super_read
    unable to find file signature
  File "H5Fsuper.c", line 155, in H5F_locate_signature
    unable to find a valid file signature

End of HDF5 error back trace

Unable to open/create file '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/tmpcoEZWF__cJHxrUtdtb__.h5'
@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

this particular test IS supposed to raise (as it's testing using read mode on a non-existent file)

why don't u take the hdf stuff out of this PR and I'll fix up -

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

yep - if I revert the last PR that's where we end up. My goal was to make it so that new people automatically opt in to using temp files (instead of allowing a filename to determine it) + allow you to specify the file extension you want (i.e. ensure_clean('.xls') as path makes path end with '.xls'

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

okay, I pushed back to the HDF-opt-out. if you use make_tempfile=False you get the original behavior of ensure_clean.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

@jtratner ok, see #5422, fixes some actual testing bugs....however I think ensure_clean is prob in conflict for our PR's. don't need the make_tempfile kw any longer, so go ahead and merge yours w/o the hdf stuff and I will rebase

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

How about I cherry-pick #5422 on to this and then put everything together? Might be simpler...

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

yep...go for it....makes more sense

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

@jreback right now 'r+' is causing a file not existing error - https://travis-ci.org/jtratner/pandas/jobs/13440462

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

why did you take all of the changes out from utils/testing.py???? you don't need this make_tempfile bizness

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

just revert the last commit on THIS pr (the hdf stuff you changed), add in my stuff and it will work

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

Ah okay...I just tried to cherry-pick the HEAD of your branch, that's why
it messed up.

On Sun, Nov 3, 2013 at 2:11 PM, jreback notifications@github.com wrote:

just revert the last commit on THIS pr (the hdf stuff you changed), add in
my stuff and it will work


Reply to this email directly or view it on GitHubhttps://github.com//pull/5419#issuecomment-27651492
.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

Btw - the way you set up the filename thing, if you pass a path like
"/a/b/c.txt", it will just completely ignore it:

In [1]: import pandas.util.testing as tm

In [2]: s = tm.ensure_clean('/a/b/c.txt')

In [4]: s.__enter__()
Out[4]: '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/tmpQL_8ra'

In [7]: s = tm.ensure_clean('a.txt')

In [8]: s.__enter__()
Out[8]: '/var/folders/rz/5czmf6hj3kdbhbqwxz4klf8h0000gn/T/tmpkucAwIa.txt'
@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

@jtratner that was on purpose, in theory if you wanted to pass a fully qualified name, that means you actually want to use it that way (which is silly but, but not meant to be bullet proof)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

you still have make_tempfile=False all over the place which defeats the purpose of using the ensure_clean, take that out

@jreback

View changes

pandas/util/testing.py Outdated
if True, returns a file-like which is *always* cleaned. Necessary for
savefig and other functions which want to append extensions. Ignores
filename if True.
make_tempfile : bool (default True)

This comment has been minimized.

Copy link
@jreback

jreback Nov 3, 2013

Contributor

you need to take this out, it serves no purpose

@jtratner

View changes

pandas/util/testing.py Outdated
if filename is None:
filename = tempfile.mkstemp()[1]
# don't generate tempfile if using a path with directory specified
if not len(os.path.dirname(filename)):

This comment has been minimized.

Copy link
@jtratner

jtratner Nov 3, 2013

Author Contributor

@jreback this is what I meant before - need to not replace if it has a dirname.

This comment has been minimized.

Copy link
@jtratner

jtratner Nov 3, 2013

Author Contributor

were you thinking that this would generate a tempfile in that directory? If so, that would mean doing tempfile.mkstemp(dir=filename)[1]

This comment has been minimized.

Copy link
@jreback

jreback Nov 3, 2013

Contributor

not at all...this is just to guard against one actually passing a fully qualified name in (I don't do it in pytables...but maybe its done else where), though it might be an error if it is...as we ALWAYS want tmp files

This comment has been minimized.

Copy link
@jtratner

jtratner Nov 3, 2013

Author Contributor

Got it - I'll change it to raise, just for clarity.

TST: Use tempfiles in all tests.
Includes @jreback's commits from #5422 and hdf_temp:
* TST: make pytables tests go thru a temporary dir and file
* TST/BUG: incorrect way of testing for r+ modes
@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

This is now at the same point as your PR, except that it raises a ValueError. It looks like it's failing with the same set of errors as the hdf_temp PR. Do you want to pick this up and iron out the problems? (and I can close this)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

sure

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

closing in favor of #5425

@jreback jreback closed this Nov 3, 2013

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2013

unbelievable...the whole problem was that needed to use mktemp and not mkstemp; the later opens the file and even you actually close it, hdf bombs (on py3 only).....now good to go

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

No no can't use mktemp - not safe and deprecated
http://docs.python.org/2/library/tempfile.html

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2013

I don't understand why it opens the file though, that's totally silly,
given that you could just use the temporary file class for that. We could
just hack together a temp directory that's a global initialized in each
file or something...

@jtratner jtratner deleted the jtratner:make-tests-use-tmp-files branch Nov 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.