BUG: close hdf store on any error

[adimascio]

NOTE: I'm opening the PR mainly to open a discussion about the current behaviour. If this is intended, please tell me. If not and you need me to write some tests, I'll do it :)

In read_hdf, if the store.select() call throws either a
ValueError, a TypeError or a KeyError then the store is closed.

However, if any other exception is thrown (e.g. an AssertionError if
there are gaps in blocks ref_loc) , the store is not closed and some
client code could end up trying to reopen the store and hit an
error like: the file XXX is already opened. Please close it before reopening in write mode. Furthermore, the exception is re-raised in all
cases.

This commit just catches any exception.

Closes #28430

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[WillAyd]

Can you add a test and a whatsnew for v1.0.0 when you get the chance?

[adimascio]

Can you add a test and a whatsnew for v1.0.0 when you get the chance?

Sure, I will do that in the next days

[adimascio]

Can you add a test and a whatsnew for v1.0.0 when you get the chance?

Sure, I will do that in the next days

I've just re-requested your review for a first version of the PR.

[adimascio]

CI is failing… It looks like some exceptions are exceptions are expected not to close the HDF store... Should we explicitly whitelist these exceptions or should we do the opposite, i.e. list all exceptions we should close the store on? In my specific case, I expected the store to be closed on AssertionErrors.

[TomAugspurger]

Which tests are failing because of that? Something like test_read_hdf_open_store?

I also would have expected an exception to close an auto-opened store.

[adimascio]

Which tests are failing because of that? Something like test_read_hdf_open_store?

The failing tests are:

• test_select_iterator
• test_read_hdf_iterator
• test_read_hdf_open_store

I also would have expected an exception to close an auto-opened store.

I didn't have the time to check for now, probably something going on with the iteration protocol. I will try to do that soon.

[jreback]

In read_hdf, if the store.select() call throws either a
ValueError, a TypeError or a KeyError then the store is closed.

However, if any other exception is thrown (e.g. an AssertionError if
there are gaps in blocks ref_loc) , the store is not closed and some
client code could end up trying to reopen the store and hit an
error like: the file XXX is already opened. Please close it before reopening in write mode. Furthermore, the exception is re-raised in all
cases.

we would want to close the store on all exceptions, likely this is an oversite.

[jreback]

you don't need to catch something that you are expecting to pass

[adimascio]

Thanks @jreback for the feedback. I'm not exactly sure about what you meant but if you suggest to remove the except ValueError, that was done on purpose in order to fail (F) rather than to output an error (E).

[adimascio]

we would want to close the store on all exceptions, likely this is an oversite.

@jreback ok, thanks! Should I go and fix the other tests accordingly then?

[TomAugspurger]

I think the tests can be updated where needed.

One question: if a user does something like

store = pd.HDFStore(...)
store.<something that causes an exception>

then will the store be closed? Or do we only close stores that we open? Ideally we would only ensure we close stores that we open.

[jreback]

this PR is slightly orthogonal to #28700 so if you want to rebase after we merge that would be great.

[WillAyd]

@adimascio can you merge master on this?

[adimascio]

@adimascio can you merge master on this?

@WillAyd @jreback I've just rebased on master. I will try to fix the tests as discussed earlier this week-end!

[WillAyd]

@adimascio can you fix the merge conflict on the whatsnew and see if you can get this green?

[jbrockmendel]

@adimascio can you fix the merge conflict on the whatsnew and see if you can get this green?

[jbrockmendel]

@adimascio can you fix the merge conflict and see if you can get the CI passing

[adimascio]

@adimascio can you fix the merge conflict and see if you can get the CI passing

I've just push-forced to fix the conflicts. I'll try to fix the CI soon.

[jreback]

@adimascio pls merge master and see if you can get passing

[WillAyd]

Closing as CI is red and I think this has gone stale, but ping if you’d like to pick this back up and can reopen