Several improvements concerning dicts, passing open HDF5-files and more

[1313e]

This PR contains the following changes:

• Improved and simplified the imports;
• Made sure that all required future imports are done in hickle.py;
• Merged many checks for Python2/Python3 into the same check, decreasing the need to duplicate certain statements;
• Empty dicts can now be properly hickled and unhickled ( Hickling an empty dict does not return one when unhickled #91 );
• Dicts using tuple keys can now be properly hickled and unhickled ( Hickling an empty dict does not return one when unhickled #91 );
• Dicts using both integers/floats and integer/float strings as dict keys (e.g., 1 and '1') can now be properly hickled and unhickled;
• Passing an open h5py.File object to hickle.dump and hickle.load will no longer automatically close the file ( [FEATURE] Providing an open HDF5-file to dump/load does not close the file #92 );
• If an Exception is raised in hickle.dump or hickle.load, an opened HDF5-file is always closed, unless it was provided as open to these functions (that is the user's task obviously) ( Hickling NumPy arrays with dtypes that are not supported by h5py #90 );
• The working directory for doing tests is now automatically set to the system's temporary directory;
• Removed all os.remove uses in the tests, as pytest or your own machine automatically performs clean-ups in the temporary directory;
• Fixed a problem with the dtype of a dict not always being properly saved ( Hickling an empty dict does not return one when unhickled #91 ).

I have tried to remain as true as possible to the original style of coding (except I use parenthesis for return, since I hate using it as a statement).
All changes should be backwards compatible, except for hickled dicts that had their dtype saved incorrectly.

[1313e]

Actually, you may want to wait with accepting the PR, as I'm planning on doing one final upgrade to the keys of dicts, which will allow for both '1' and 1 to be used and saved as dict keys (which is currently not possible).

[telegraphic]

Thanks, looks like there's some great stuff here!

I am however going to push back against the use of eval, for security reasons. A nefarious soul could create a hdf5 file with a key 'os.system("rm -rf /"), with attribute "<class 'tuple'>", which would not be fun.

That said, loading pickles is in itself dangerous if you didn't make it yourself, e.g. in the pickle docs:

Warning: The pickle module is not intended to be secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source.

As hickle loads data mapped from HDF5, it should in theory be much safer*. There is however in hickle.load a safe=True flag so a user can explicitly decide to unpickle arbitrary objects.

I think in this case, the cons of using eval outweigh the pros, and existing users may not forsee arbitrary code execution when loading a HDF5 file. Perhaps there should be more discussion of safety in the documentation, as it is not currently stated?

* Of course, don't trust any code! Obligatory disclaimers in the LICENSE.

[1313e]

@telegraphic I was considering ACE as well, but it is not possible to create a dataset with a key that is a tuple but in reality is not.
The only way of doing that would be by manually adjusting the attribute yourself after hickle dumped the data.
In that case, it is not really hickle's fault.

But, I will have a look at it.
Maybe I can come up with a different solution (or make sure that using eval cannot be used for any ACE at all).

[telegraphic]

Thanks, hope you're having fun ;). It's was a learning experience for me dealing with bug reports due to weird & wonderful use cases I didn't think of (or even know you could do)! It's a bit of a balancing act to find sensible ways to map between Python and HDF5 data structures.

[1313e]

Thanks, hope you're having fun ;). It's was a learning experience for me dealing with bug reports due to weird & wonderful use cases I didn't think of (or even know you could do)! It's a bit of a balancing act to find sensible ways to map between Python and HDF5 data structures.

Oh, I am having fun with this.
How much focus do you want me to put on avoiding the opportunity for ACE?

[telegraphic]

Good question, difficult to answer! I think an estimate of likelihood of use case is required. As little as possible?

For example, I can certainly see use cases where you have tuple indexes all of the same type, e.g. a = {(0, 0, 0) : np.array(...)} for tying sparse data to an array index. So supporting tuples consisting of data all of the same type might be a reasonable compromise in terms of code simplicity and usefulness.

If you do find use cases that really really need eval, I would request it is tied to the safe= keyword in load, so a user gets a warning.

There is very basic support for registering a new class to hickle, which gives a path to allow users to add their own creations, but this is still limited and won't necessarily override default handling of e.g. dict.

[1313e]

I actually have a case where I am using tuples that have integers, floats and strings in them.
However, using the eval function simply makes saving and loading the tuples much, much easier.

[1313e]

Alright, I have currently removed using the eval function for everything besides tuples (it was also required for strings before, to make sure that 1 and '1' could be used).
Now looking into avoiding eval for tuples.

[1313e]

Alright, I just found out about the ast.literal_eval function (link) which does not allow for ACE to occur, but I can still convert tuple strings back to tuples.
I guess this satisfies the condition?

[telegraphic]

Thanks @1313e, ast.literal_eval is a nice find! Happy to accept all these changes, fantastic stuff :)

[1313e]

@telegraphic Would it be possible to generate a new release for PyPI, such that I can update the requirements in PRISM and make a new version there as well?

[telegraphic]

sure thing, just rerunning tests, the scipy sparse_matrix test is failing for some reason on Py 3.5 and 3.7 (but not Py 2.7 or 3.7), even though PR passed

[1313e]

I have been having some issues with Travis lately as well, where it was simply giving me errors about timing out or something.

[1313e]

Ugh, it is astropy, again...
Seems that astropy 3.1.2 is the problem, as the PR used 3.1.1, but the current builds are using 3.1.2.

[1313e]

You may want to set a minimum required version for NumPy, as I get the feeling that that is the problem (it raises errors of NumPy 1.7, which was released a very long time ago).

[telegraphic]

@1313e -- I think this is an upstream bug, so pushed to PyPi

[1313e]

@telegraphic I saw it, thanks.

[1313e]

@telegraphic Also, if you need some help with Python2/Python3 compatibility (especially strings) or anything else, let me know.