Changed QualityIO so that it uses iter explicitly on the handle, so t… #3031

xzy3 · 2020-07-07T03:44:43Z

…hat tempfile.NamedTemporaryFile can be used easily.

3.7.0 (default, Aug 15 2018, 09:03:42)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]
CPython
Linux-3.10.0-1127.e17.x86_64-x86_64-with-centos-7.8.2003-Core
Bio.version 1.77

This line:

biopython/Bio/SeqIO/QualityIO.py

Line 923 in 380932d

line = next(handle)

has broken reading from tempfile.NamedTemporaryFile unless you do thisSeqIO.parse(tmp_fd.file, 'fastq'). NamedTemporaryFile returns https://github.com/python/cpython/blob/1bf9cc509326bc42cd8cb1650eb9bf64550d817e/Lib/tempfile.py#L457
which is for some reason seems to not be its own iterator. I don't see any recent changes to the tempfile module to cause this.

I hereby agree to dual licence this and any previous contributions under both
the Biopython License Agreement AND the BSD 3-Clause License.
I have read the CONTRIBUTING.rst file, have run flake8 locally, and
understand that AppVeyor and TravisCI will be used to confirm the Biopython unit
tests and style checks pass with these changes.
I have added my name to the alphabetical contributors listings in the files
NEWS.rst and CONTRIB.rst as part of this pull request, am listed
already, or do not wish to be listed. (This acknowledgement is optional.)

codecov · 2020-07-07T03:58:20Z

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.30%. Comparing base (e838eb9) to head (1d18ede).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
Bio/SeqIO/PirIO.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3031      +/-   ##
==========================================
+ Coverage   85.37%   86.30%   +0.93%     
==========================================
  Files         282      282              
  Lines       59385    59467      +82     
==========================================
+ Hits        50697    51323     +626     
+ Misses       8688     8144     -544

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peterjc · 2020-07-07T12:59:47Z

Have you looked at the Python issue tracker to see if this is a known issue with tempfile.NamedTemporaryFile itself?

Can you add a test using a tempfile.NamedTemporaryFile - this could affect other parsers in SeqIO too?

xzy3 · 2020-07-07T23:00:05Z

There is some discussion on the Python mailing list from 2016, but nothing seemed to come of it: https://mail.python.org/pipermail/python-list/2016-July/862590.html

The git blame on tempfile.NamedTemporaryFile says it hasn't been touched in 5+ years. So it's at least a very old bug. There are no mentions of this specific quirk on the Python bugs list that I can find.

And sure i'll work on a unittest.

mdehoon · 2020-07-08T02:44:14Z

@xzy3 Can you provide a self-contained example showing the problem?

xzy3 · 2020-07-08T03:04:18Z

@mdehoon here you go

from tempfile import NamedTemporaryFile
from Bio import SeqIO

test_seq = """\
@HWI-EAS209_0006_FC706VJ:5:58:5894:21141#ATCACG/1
TTAATTGGTAAATAAATCTCCTAATAGCTTAGATNTTACCTTNNNNNNNNNNTAGTTTCTTGAGATTTGTTGGGGGAGACATTTTTGTGATTGCCTTGAT
+HWI-EAS209_0006_FC706VJ:5:58:5894:21141#ATCACG/1
efcfffffcfeefffcffffffddf`feed]`]_Ba_^__[YBBBBBBBBBBRTT\]][]dddd`ddd^dddadd^BBBBBBBBBBBBBBBBBBBBBBBB
"""

with NamedTemporaryFile(mode='w+t') as fd:
  fd.write(test_seq)
  fd.seek(0)
  next(SeqIO.parse(fd, 'fastq'))

(stantz5) xzy3@biolinux> python ~/test-seqio.py
Traceback (most recent call last):
  File "/scicomp/home/xzy3/test-seqio.py", line 14, in <module>
    next(SeqIO.parse(fd, 'fastq'))
  File "/scicomp/home/xzy3/.local/virtualenvs/stantz5/lib/python3.7/site-packages/Bio/SeqIO/Interfaces.py", line 68, in __next__
    return next(self.records)
  File "/scicomp/home/xzy3/.local/virtualenvs/stantz5/lib/python3.7/site-packages/Bio/SeqIO/QualityIO.py", line 1081, in iterate
    for title_line, seq_string, quality_string in FastqGeneralIterator(handle):
  File "/scicomp/home/xzy3/.local/virtualenvs/stantz5/lib/python3.7/site-packages/Bio/SeqIO/QualityIO.py", line 923, in FastqGeneralIterator
    line = next(handle)
TypeError: '_TemporaryFileWrapper' object is not an iterator

xzy3 · 2020-07-08T03:44:44Z

MafIO turned out to have the exact same problem. I did the same fix to that parser also.

mdehoon · 2020-07-08T06:09:53Z

@xzy3 Thank you.

It seems that this is a bug in Python. For example, we get the same problem here:

>>> fd = open("mytestfile", "w+t")
>>> fd.write('something')
9
>>> fd.seek(0)
0
>>> next(fd)
'something'
>>> fd.close()
>>> from tempfile import NamedTemporaryFile            
>>> fd = NamedTemporaryFile(mode='w+t')
>>> fd.write('something')
9
>>> fd.seek(0)
0
>>> next(fd)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '_TemporaryFileWrapper' object is not an iterator
>>>

if this is indeed a bug in Python, I don't think we should fix this in Biopython, in particular since there is a trivial workaround.

xzy3 · 2020-07-08T12:44:59Z

@mdehoon This exact thing was brought up on the Python mailing list in 2016 and it was informally dropped as "not a bug". The argument against was that NamedTemporaryFile does not claim to be an iterator in the documentation, and iter should be called on it rather than changing the wrapper object. I can agree, it definitely violates my assumptions of how it should work. I couldn't find a bug report in the Python bug tracker open or closed. However I can try formally pushing this upstream and see if we get a different result.
The result of NamedTemporaryFile is a wrapper object that uses dunder method magic to forward method calls to the underlying file. The most recent change to the object was to change __iter__ to be a generator in response to a bug. Apparently the iter builtin does not use __getattr__ to find methods and that may be true of __next__ also.

peterjc · 2020-07-09T12:23:44Z

I agree, it does seem to be a bug in Python itself, and would support reporting a bug to Python directly.

However, even if they do fix it, it won't be widely available for a while (and may never be backported as far at Python 3.6), so being pragmatic I am OK with adding the workaround (with tests) to Biopython.

xzy3 · 2020-07-09T12:38:09Z

@peterjc Ok I'll report it to the Python bugtracker and reference this discussion.

mdehoon · 2020-07-09T14:10:57Z

@peterjc Even then I don't think this needs to be fixed in Biopython. As @xzy3 wrote, one workaround is to use the file property of the named temporary file as in SeqIO.parse(tmp_fd.file, 'fastq'). Another solution is to call iter on tmp_fd, as in SeqIO.parse(iter(tmp_fd), 'fastq'). Both are simple workarounds that a user can use. If we do fix this in Biopython, we would have to fix it everywhere (not just Bio.SeqIO).

xzy3 · 2020-07-09T14:26:16Z

@mdehoon as best I can tell there are only two places in SeqIO that use next directly on a file. I've fixed them both in this patch.

xzy3 · 2020-07-10T14:16:11Z

@peterjc ok tests added, fixed a spot in MafIO that had the same problem. I made an issue over at the Python bug tracker and submitted a pull request python/cpython#21434

peterjc · 2020-07-14T20:41:04Z

Looks fairly positive so far on your Python pull request 👍

peterjc · 2020-07-22T12:52:26Z

Looks like the Python pull request is awaiting review by the core team.

Even if it is merged, I am inclined to merge this fix (although we have some git conflicts to resolve first). [fixed typo]

@mdehoon would you prefer to wait and see what the Python team decides?

mdehoon · 2020-07-22T12:57:18Z

@peterjc Yes we should wait and see the Python team decides.

xzy3 · 2020-10-19T12:36:05Z

Well I committed a brain-o and got the first pull request in a bad state. I had to restart it so it's now here python/cpython#22766

peterjc · 2020-10-19T12:41:20Z

Let's hope the CPython team accept that - thank you.

mdehoon · 2025-02-05T01:09:10Z

Bio/SeqIO/FastaIO.py

        super().__init__(source, fmt="Fasta")
        try:
-            line = next(self.stream)
+            line = next(iter(self.stream))


Is it possible to use line = self.stream.readline()?

mdehoon · 2025-02-05T01:11:30Z

Bio/SeqIO/PirIO.py

            raise StopIteration
-        stream = self.stream
+        # NamedTemporaryFiles don't support __next__ so create an iter as a workaround
+        stream = iter(self.stream)


Do you actually need this here? We are not calling next on stream, and for line in stream implicitly calls iter on stream.

line 155 is next(stream).step() which breaks in the unit tests when run with NamedTempFiles.

mdehoon · 2025-02-05T01:12:15Z

Bio/SeqIO/QualityIO.py

+        # handle as a work around.
+        handle_iter = iter(handle)
        try:
            line = next(handle)


can you use readline instead?

mdehoon · 2025-02-05T01:13:41Z

Bio/SeqIO/QualityIO.py

        if line is None:
            try:
-                line = next(self.stream)
+                line = next(stream)


Same here. The current fix required calling iter each time we enter __next__.

xzy3 · 2025-02-05T13:57:49Z

switched to fd.readline() everywhere. Doing so required some minor changes to parser logic.

peterjc · 2025-02-05T14:25:36Z

Bio/SeqIO/FastaIO.py

-            line = next(self.stream)
-        except StopIteration:
+        line = self.stream.readline()
+        if line == "":


I think if not line: is considered better stylistically.

mdehoon · 2025-02-06T07:45:36Z

Bio/SeqIO/PirIO.py

+
        identifier = line[4:].strip()
-        description = next(stream).strip()
+        description = self.stream.readline()


Do you need to call .strip() here?

It's called 3 lines down after checking if the empty string was returned to indicate EOF.

mdehoon · 2025-02-06T07:46:53Z

Bio/SeqIO/QualityIO.py

-                self.line = line
-        if line is None:
+            line = self.stream.readline()
+        if line is None or line == "":


I think line cannot be None here.

mdehoon · 2025-02-06T07:47:47Z

Bio/AlignIO/MafIO.py

        # allows parsing of the last bundle without duplicating code
        try:
-            line = next(handle)
+            line = next(handle_iter)


Can you use .readline() here instead of creating handle_iter?

tempfile.NamedTemporaryFiles do not support __next__.

xzy3 · 2025-02-21T13:50:41Z

Do I need to do anything else?

peterjc · 2025-02-24T14:16:30Z

Thank you both.

xzy3 requested a review from peterjc as a code owner July 7, 2020 03:44

xzy3 mentioned this pull request Feb 3, 2025

Biopython 1.85 FastaIO calling next on file descriptor #4926

Open

xzy3 force-pushed the named-tempfile-fix branch from d496faf to 50f2e60 Compare February 4, 2025 15:58

mdehoon self-requested a review February 5, 2025 01:05

mdehoon reviewed Feb 5, 2025

View reviewed changes

xzy3 force-pushed the named-tempfile-fix branch from 50f2e60 to 735c2e2 Compare February 5, 2025 13:33

peterjc reviewed Feb 5, 2025

View reviewed changes

xzy3 force-pushed the named-tempfile-fix branch from 735c2e2 to 32d5038 Compare February 5, 2025 15:15

mdehoon reviewed Feb 6, 2025

View reviewed changes

Changed several parsers avoid using next.

1d18ede

tempfile.NamedTemporaryFiles do not support __next__.

xzy3 force-pushed the named-tempfile-fix branch from 32d5038 to 1d18ede Compare February 6, 2025 14:00

mdehoon requested review from mdehoon and removed request for mdehoon February 22, 2025 01:29

mdehoon approved these changes Feb 22, 2025

View reviewed changes

peterjc merged commit d6fcdfa into biopython:master Feb 24, 2025
31 checks passed

xzy3 deleted the named-tempfile-fix branch February 24, 2025 17:13

Changed QualityIO so that it uses iter explicitly on the handle, so t… #3031

Changed QualityIO so that it uses iter explicitly on the handle, so t… #3031

Uh oh!

Conversation

xzy3 commented Jul 7, 2020

Uh oh!

codecov bot commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peterjc commented Jul 7, 2020

Uh oh!

xzy3 commented Jul 7, 2020

Uh oh!

mdehoon commented Jul 8, 2020

Uh oh!

xzy3 commented Jul 8, 2020

Uh oh!

xzy3 commented Jul 8, 2020

Uh oh!

mdehoon commented Jul 8, 2020

Uh oh!

xzy3 commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterjc commented Jul 9, 2020

Uh oh!

xzy3 commented Jul 9, 2020

Uh oh!

mdehoon commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xzy3 commented Jul 9, 2020

Uh oh!

xzy3 commented Jul 10, 2020

Uh oh!

peterjc commented Jul 14, 2020

Uh oh!

peterjc commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdehoon commented Jul 22, 2020

Uh oh!

xzy3 commented Oct 19, 2020

Uh oh!

peterjc commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xzy3 commented Feb 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xzy3 commented Feb 21, 2025

Uh oh!

Uh oh!

peterjc commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov bot commented Jul 7, 2020 •

edited

Loading

xzy3 commented Jul 8, 2020 •

edited

Loading

mdehoon commented Jul 9, 2020 •

edited

Loading

peterjc commented Jul 22, 2020 •

edited

Loading