Skip to content

Conversation

filmor
Copy link
Contributor

@filmor filmor commented Feb 25, 2015

Currently the HDFStore in read_hdf is opened in rw mode by default, leading to permission
errors when reading a non-writeable file.

This patch changes the default mode to r.

@bashtage
Copy link
Contributor

This seems like a big change since any code like

store = pd.HDFStore('my_store.h5')
store.put(df,'key')
store.close()

will now not work. The default is a I believe (append).

@filmor
Copy link
Contributor Author

filmor commented Feb 25, 2015

I updated the description, this doesn't actually change the behaviour on HDFStore but just on read_hdf.

@bashtage
Copy link
Contributor

I see - seems reasonable to read in 'r'.

@shoyer
Copy link
Member

shoyer commented Feb 25, 2015

Is it possible to test this change?

Either way, this needs a release note for "what's new".

@bashtage
Copy link
Contributor

Create a hdf file and change it to read only using os.chmod. The use read_hdf...it should pass now and would have failed before.

On Feb 25, 2015 3:06 PM, Stephan Hoyer [email protected] wrote:

Is it possible to test this change?

Either way, this needs a release note for "what's new".


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

@jreback
Copy link
Contributor

jreback commented Feb 25, 2015

the default mode is 'a', which is the PyTables default, and IMHO the most correct thing to do. It doesn't actually write the file if you don't do any write operations so its safe for read-only systems as well.

You can write a test to validate, but since the mode is determined by .open() I believe it is already correct.

this was a bug, but fixed in 0.13: #4513

@jreback jreback added the IO HDF5 read_hdf, HDFStore label Feb 25, 2015
@filmor
Copy link
Contributor Author

filmor commented Feb 26, 2015

Hmm, indeed it works locally, but if the h5 file to be opened is on a network share it fails with a permission error (that's where this appeared). I'll try to verify it, but it's going to be difficult to test this ...

@filmor
Copy link
Contributor Author

filmor commented Feb 26, 2015

Indeed, when I deny the Write permission in Windows' security settings I can not read anymore with mode="a" but it works with mode="r".

@bashtage
Copy link
Contributor

It sounds like it is a platform-specific bug, but it also looks innocuous to open 'r' in read_hdf since the store is immediately closed.

Currently the `HDFStore` is opened in `rw` mode by default, leading to permission
errors when reading a non-writeable file.
@jreback jreback modified the milestone: Next Major Release Mar 3, 2015
@jreback
Copy link
Contributor

jreback commented Mar 5, 2015

@filmor I don't think this is going to be possible as the default is append (and makes more sense)

@jreback jreback closed this Mar 5, 2015
@filmor
Copy link
Contributor Author

filmor commented Mar 6, 2015

What? For *read*_hdf append is more sensible than read? I disagree.

Just to make this clear, this PR does not change the default on HDFStore, nor does it change how read_hdf works if you pass in a store directly.

@filmor
Copy link
Contributor Author

filmor commented Mar 23, 2015

Please reconsider, the patch is trivial, doesn't break anything because behaviour doesn't change and this issue is causing real problems in our systems, as people are not regularly allowed to change the shared HDF5 files. I hate to sprinkle mode='r' all over the place, in particular as read_hdf(f, mode='r') just sounds broken ...

@jreback
Copy link
Contributor

jreback commented Mar 23, 2015

you can simply use a function internal to what you do. Even if I agreed that this should be changed, this would take a month or more to make it to the next release, then you would have to update, etc.

def read_hdf(fname, key, mode=None, **kwargs):
    if mode is None:
         mode = 'r'
    return pd.read_hdf(fname, key, mode=mode, **kwargs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IO HDF5 read_hdf, HDFStore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants