Biopython: MafIO should index files in bytes mode

[benfulton]

Does not seem to occur using Python 3.6.

Test failure as follows:

======================================================================

ERROR: test_correct_retrieval_1 (test_MafIO_index.TestSpliceGoodMAF)

Traceback (most recent call last):
File "C:\GIT\biopython\Tests\test_MafIO_index.py", line 398, in test_correct_retrieval_1
3227479), 1)
File "C:\GIT\biopython\build\lib.win32-3.5\Bio\AlignIO\MafIO.py", line 555, in get_spliced
fetched = [multiseq for multiseq in self.search(starts, ends)]
File "C:\GIT\biopython\build\lib.win32-3.5\Bio\AlignIO\MafIO.py", line 555, in
fetched = [multiseq for multiseq in self.search(starts, ends)]
File "C:\GIT\biopython\build\lib.win32-3.5\Bio\AlignIO\MafIO.py", line 530, in search
(rec_start, rec_end, offset, start, end))
ValueError: Expected 3185728-3185798 @ offset 1974167, found 3185634-3185641

[peterjc]

This is with Biopython 1.69, I assume?

@polyatail @adamnovak @blaiseli Do any of you have access to a Windows machine to try reproducing this?

It puzzles me that this works on Python 3.6 but fails on Python 3.5, otherwise I would immediately suspect something like text mode versus binary mode...

[peterjc]

Actually the file mode might be the problem, does changing this line fix it?

https://github.com/biopython/biopython/blob/biopython-169/Bio/AlignIO/MafIO.py#L250

self._maf_fp = open(self._maf_file, "r")

proposed:

self._maf_fp = open(self._maf_file, "rb")

[peterjc]

Separately it might be worth trying using from Bio.File import _open_for_random_access instead which would do this and hopefully make it work on BGZF compressed MAF files?

[benfulton]

Changing the "r" to "rb" fixes the issue, and causes half the remaining MafIO_Index tests to fail :)

File "C:\GIT\biopython\Bio\AlignIO\MafIO.py", line 220, in MafIterator
elif line.startswith("a"):
TypeError: startswith first arg must be bytes or a tuple of bytes, not str

I believe PEP 529 -- Change Windows filesystem encoding to UTF-8 -- probably fixed the issue for 3.6.

[peterjc]

OK - this is an oversight from the MAF indexing not previously having proper testing on Windows.

I think I know what to look for having been though similar issues with the SeqIO indexing, although I'm not sure if I'll be able to dig into it immediately. The key design point was deliberately opening the files in binary mode, so that the indexes are always raw bytes - and not offsets corrected with behind the scenes new line conversions.

[peterjc]

I've had an initial look at this, and unfortunately retrospectively fixing the indexing to use bytes mode will be more work than I hoped - work in progress at https://github.com/peterjc/biopython/tree/maf_index

The SeqIO and SearchIO indexer all implement a get_raw method returning bytes, and the default for pulling out a single entry is essentially to call next(iterator_based_text_parser(StringIO(_bytes_to_string(self.get_raw(offset))))). The MAF indexing does not attempt to offer this functionality, and does not record the raw record size in its database either.

[chebizarro]

I've run the Biopython 1.78dev0 test suite (built from source) on a Windows 10 machine running Python 3.8 and the MafIO index tests pass without any problems. This is a "native" installation, not using WSL. If the issue is related to versions of Python < 3.6 , can this issue be considered resolved by obsolescence?

[peterjc]

My expectation is that for Linux style new lines (CR only) the both indexes will record the true offset in bytes, but with Windows style double-byte CR LF new line files, the indices will use the character offset treating the two-byte line breaks as one character (and perform less well than using raw bytes mode because Python is adding an offset translation via unicode mode).

The design in SeqIO and SearchIO assumed raw bytes mode and indices, but using unicode mode ought to work at a slight performance hit - but it seems entirely possible that doing it in unicode mode introduces the additional factor of the system's unicode encoding. Perhaps there was a bug in Python 3.5?

If we had spotted it at the time this code was added, I would have encouraged indexing the MAF files in binary mode. At this point perhaps not worth worrying about? Making the change would also invalidate pre-existing indexes of files with Windows new lines, which would be annoying.

So maybe close this, or re-label it from bug to enhancement?

[MarkusPiotrowski]

I've run the Biopython 1.78dev0 test suite (built from source) on a Windows 10 machine running Python 3.8 and the MafIO index tests pass without any problems.

It depends on the environment in which you are testing. If you download the Biopython master directly from the Github page (e.g. as zip file), build and test, then the test will pass.
If you are working in your Git environment, Git on Windows will usually convert line endings on checkout from LF to CR/LF. The indices in the test are derived from files with LF line endings and therefore the test still fails.

Just tested.

[JoaoRodrigues]

Debugging this here locally and on the Github Actions CI platform. I set the file to binary on .gitattributes and locally it works: all tests pass. I can't seem to get it to work on the cloud though. I checked the git settings for the Windows machines and they set git.core.autocrlf to true. I am not caching any code/test files so I am not sure where the problem comes from.

In any case, to solve this particular issue (#1149), I think we should simply add the rule to .gitattributes. New clones will automatically have this problem fixed and if people run into the test failure again it's a matter of simply asking them to re-clone.

[JoaoRodrigues]

Argh, what an annoying issue. We have to set both cnksr3.fa and ucsc_mm9_chr10_big.maf as binary in the .gitattributes file for the test to pass. Took me a while to figure that one out. Let me check if this has an effect on the GHA branch CI, that would be a good independent test.

EDIT: Fixed!