Fix 2047 Ensure all files written by AiiDA and plugins are utf8 encoded

[ConradJohnston]

Fixes #2047 and fixes #2142

• Changes the folders.Folder.open function to use io.open and checks to see if the file should be opened in binary mode or formatted mode. Enforces UTF-8 by default.
• Replaces system "open" with "io.open" everywhere in the codebasem, except where sys open is preferable (e.g. HiddenFiles()). Explicitly sets the encoding as UTF-8, or as None for binary files.
• Sets a number of testing strings as unicode strings explicitly.
• Changes how writing JSON to zip files is done in Import/Export to handle uniicode.
• In some cases, change cStringIO to StringIO to ensure that unicode can bed used.

[dev-zero]

ignore my previous review: for some reason GitHub showed me the outdated stuff

[dev-zero]

this is not the same, StringIO gives you a file-like object while six.text_type converts a str to a unicode on Python 2 and is a no-op on Python 3

[ConradJohnston]

Hi, I disagree on this one.

I know that the code looks ugly, but it’s to satisfy compatibility with Python 2 and 3.
The underlying issue is really create_file_from_filelike() which uses shutil.copyfileobj internally.
Minimal code that shows the difficult behaviour is below.

# encoding: utf-8
import io
import json
import shutil
from six import StringIO as StringIO

d_ascii={'a':'hello'}
filelike = StringIO(json.dumps(d_ascii))

with io.open('/tmp/test.dat', 'wb')  as dest_file:
    shutil.copyfileobj(filelike, dest_file)

This will work in Python2 but fail in Python3.
The reason is that the filelike object returned by StringIO is of the string type (due to coming from json.dumps). In Python 3, this is a unicode string which can’t be buffered in ‘wb’ mode, which requires bytes.

One could change the io.open line to encode to UTF-8 (and this is what I now do):

with io.open('/tmp/test.dat', 'w’, encoding=’utf8’)  as dest_file:

but now Python 2 will fail as io.open demands a unicode object, but we are supplying a byte-string.

A solution which satisfies both Python2 and Python3, while ensuring that the written file is UTF-8 encoded, is to make sure that what comes out of json.dumps is converted to a unicode string.
Six.text_type() does this. At this point we haven’t encoded, but we eventually do this with io.open, and besides, json does encoding to UTF-8 internally.

In the end, the following examples satisfies both Python 2 and 3:

# encoding: utf-8
import io
import json
import shutil

import six
from six import StringIO as StringIO
 
d_ascii={'a':'hello'}
filelike = StringIO(six.text_type(json.dumps(d_ascii)))
with io.open('/tmp/test.dat', 'w', encoding='utf8')  as dest_file:
    shutil.copyfileobj(filelike, dest_file)

d_unicode={'b': u'γεια σας'}
filelike = StringIO(six.text_type(json.dumps(d_unicode)))
with io.open('/tmp/test.dat', 'w', encoding='utf8')  as dest_file:
    shutil.copyfileobj(filelike, dest_file)

[dev-zero]

The problem with using six.text_type to work around the inconsistent json API is that if you do not specify anything, it uses the default system encoding for decoding a bytes or a str into a unicode string, which is exactly the ambiguity we try to avoid in the first place.
But if you are adding the encoding="utf8", then six.text_type("", encoding="utf8") runs on Python 2 and fails on Python 3.

I would rather prefer an explicit if six.PY2: ... switch or a (maintained and consistent) library like simplejson.

The other drawback of these kind of solutions is that in a year or two nobody will have a clue anymore why this wrapping is there, whether it is still needed or not and it will have spread to other places because people are doing copy&paste, especially when it comes to solving problems they don't know about.

[ConradJohnston]

This is a fair point.
I think the tidy thing to do, if we want to use a switch, is to have a json utility function, although I don't know where best to put this. Then it would be just a case of changing the json dump function calls where they occur.

[coveralls]

Coverage increased (+0.01%) to 68.671% when pulling 369dd9f on ConradJohnston:fix_2047_ensure_all_files_are_utf8_encoded into 9bc4e36 on aiidateam:develop.

[dev-zero]

I would rather write directly the expected content after the json.dumps than do the six.text_type wrapping here.

[ConradJohnston]

Wrapping now removed.

[dev-zero]

the flush here and below are pointless since the file will be closed afterwards and therefore flushed.

[ConradJohnston]

flush() removed.

[dev-zero]

I am really not convinced that this is a portable and maintainable solution, see #2142

[ConradJohnston]

Now resolved.

[dev-zero]

My preference would be a separate codepath for Python 2:

if six.PY2:
  ...
else:
  ...

This way it would be clearer on what transformation is needed once we drop Python 2

[ConradJohnston]

Resolved by /utils/json.py replacement.

[sphuber]

We discussed this with @giovannipizzi and @ConradJohnston and we agreed that it would be possible to define the four used json functions load and dump (and the string variants) in aiida.utils.json, where we can do a try/catch importing simplejson and fall back on our own wrapper. We can change all the imports import json to from aiida.utils import json such that we have one single point of entry. In addition, the ujson and anyjson dependencies can be dropped in favor of simplejson as an optional dependency.

[sphuber]

Thanks a lot @ConradJohnston , to me it looks good and ready to merge. @dev-zero if you are also happy, let me know and I will merge this PR.

Just one final question that occurred to me. Since we are now requiring that all files written by AiiDA are utf-8 encoded, shouldn't we also ship a "migration" to properly write all currently written files in the repositories?

[dev-zero]

Looks very good, thanks @ConradJohnston

@sphuber I am not sure about that one yet. It might be easy to provide a script: use chardet to detect the encoding, if something is not pure ASCII or UTF8, load it using the detected encoding, write it again in UTF-8. But for that we would have to be able to know which ones are really text and which are binaries (or to be treated as such, like the UPF).

[ConradJohnston]

Thanks a lot @ConradJohnston , to me it looks good and ready to merge. @dev-zero if you are also happy, let me know and I will merge this PR.

Just one final question that occurred to me. Since we are now requiring that all files written by AiiDA are utf-8 encoded, shouldn't we also ship a "migration" to properly write all currently written files in the repositories?

I suppose the problem with a migration is that we can't know how the files in a user's repository were written and can't infer the encoding by inspection.

[sphuber]

But do we, after this PR, assume that all files in the repository are utf-8 encoded? And if so, and this is not the case, will everything fall over?

[ConradJohnston]

@sphuber - You could be right.
The encoder is set up to fail when it encounters characters that aren't in UTF-8, but the bigger problem is if it silently does the wrong thing. I think some IS0-8859-1 encoded files can be silently and wrongly interpreted by the UTF8 codec.
I'll see if there's an example of this.

[giovannipizzi]

I think we can merge this - this (hopefully) doesn't make the situation worse than now.
Anyway I think we must treat any file coming from a remote supercomputer as binary, and the parser has to be clever enough to figure it out - I think it would be bad to get the output of a code and try to change it when storing. Ideally, we should (in the future, see #2046) keep track of the default encoding on the supercomputer at the time of storing, as an additional metadata - even if I think getting this right is complicated (e.g. the user might have changed env vars in the submission script or other weird stuff).

[sphuber]

So does that mean that any file that is retrieved by the engine from the supercomputer is stored wholesale in the repository with the same encoding? Which means, that nowhere in the code we are (or should be) expecting the files to always be utf-8, but it could be anything?

[giovannipizzi]

that would be my expectation. instead for files aiida write itself and then has to read again (e.g. if it stores a file in the repo that it has to read again, say a json or other) then we should ensure always utf8

[sphuber]

OK, if we take the stance that we can never assume utf-8 unless we explicitly write it ourselves, we don't need a migration and we can merge this PR.

[sphuber]

Many thanks to @ConradJohnston and @dev-zero for the hard work!

@dev-zero already mentioned in the comments he approves after the made changes