eliminate seek()s #11

mr-c · 2014-10-10T00:31:25Z

mr-c · 2014-10-10T00:47:14Z

Alas this fix doesn't appear to be compatible with Python 2.6. How annoying. @ctb are you willing to ditch Python2.6 support for screed to enable streaming?

Otherwise one could build a workaround using the Python zlib library. Bleh.

ctb · 2014-10-10T19:01:32Z

What's not python2.6 compat? Is it handled by future? And/or can we try the 2.7 way and fall back to 2.6 dynamically, thus enabling only on 2.7+?

mr-c · 2014-10-10T19:25:36Z

The gzip module’s GzipFile now [...] implements the io.BufferedIOBase ABC, so you can wrap it with io.BufferedReader for faster processing (contributed by Nir Aides; issue 7471).

Not fixable via a __future__ import.

We could use http://bugs.python.org/file15619/gzip_7471_py27.diff to do a temporary MonkeyPatch of Python2.6.

I'll try out a versioned fall back first.

mr-c · 2014-10-10T20:09:46Z

Fallback is in place; could be a bit cleaner to allow for seek-free parsing for non-gziped files under Python2.6.

mr-c · 2014-10-31T17:07:02Z

@brtaylor92 & @ctb ready for review & merge

ctb · 2014-11-01T09:55:17Z

On Fri, Oct 31, 2014 at 10:07:03AM -0700, Michael R. Crusoe wrote:

@brtaylor92 & @ctb ready for review & merge

ok, I'll try to take a look, but might not get to it 'til middle of
week.

--t

C. Titus Brown, ctb@msu.edu

ctb · 2014-11-01T10:34:18Z

screed/openscreed.py

-
-    if not line:
-        return []
+    if sys.version_info[1] >= 7:


Maybe check that this is Python 2 here, or at least put a comment in about Python 2 just to make it clear for casual readers?

SensibleSalmon · 2014-11-03T02:56:16Z

While testing in khmer there's a change in behaviour as empty files now throw an exception.

Maybe we should have screed tests that test empty file behaviours.

I'll write that when I'm done.

Also, what's up with this test? https://github.com/ged-lab/khmer/blob/master/tests/test_scripts.py#L449

mr-c · 2014-11-03T03:40:56Z

Thanks @bocajnotnef. Yeah, it is a bit weird that we expect diginorm to silently accept an empty file without a --force or similar. Seems to be contrary to how the other tools work.

mr-c · 2014-11-03T04:32:28Z

write a test for unknown file format
port @bocajnotnef 's streaming tests to work directly with screed objects

mr-c · 2014-11-04T19:15:57Z

screed/openscreed.py

-    line = sequencefile.readline()
-
-    if not line:
-        return []


The current screed returns an empty list in the instance of an empty file. @ctb shall I retain this behaviour or throw an exception? (Like we do for an unknown file format)

On Tue, Nov 04, 2014 at 11:15:58AM -0800, Michael R. Crusoe wrote:

compression = None for magic, ftype in magic_dict.items(): if file_start.startswith(magic): compression = ftype break

sequencefile = {

'gz': lambda: gzip.open(filename),

'bz2': lambda: bz2.BZ2File(filename),

'zip': lambda: zipfile.ZipFile(filename),

- None: lambda: _open(filename)}compression

- line = sequencefile.readline()

if not line:

return []

The current screed returns an empty list in the instance of an empty file. @ctb shall I retain this behaviour or throw an exception? (Like we do for an unknown file format)

+0 for returning empty list. I don't have a strong reason for this but
there is nothing technically wrong with an empty file, is there? :)

cheers,

--titus

C. Titus Brown, ctb@msu.edu

It isn't wrong to return an empty list; it is a bit uninformative though

SensibleSalmon · 2014-11-12T19:13:35Z

Relevant to dib-lab/khmer#654

mr-c · 2014-12-02T16:30:55Z

Tests have been added.

SensibleSalmon · 2014-12-02T21:25:33Z

@ctb @camillescott @luizirber Status of CR on this....?

mr-c · 2014-12-05T16:26:15Z

I've retained the behavior of opening an empty file (returning an empty list)

Since we already have a buffered reader there is no reason to further wrap the compressed file readers in another buffered reader. This frees us to support streaming with Python 2.6 as well

mr-c · 2014-12-17T15:38:14Z

@ctb this is ready for your review

ctb · 2014-12-18T16:01:21Z

screed/openscreed.py

-    rawfile.close()
+    bufferedfile = io.open(file=filename, mode='rb', buffering=8192)
+    num_bytes_to_peek = max(len(x) for x in magic_dict)
+    file_start = bufferedfile.peek(8192)


shouldn't 8192 be replaced by num_bytes_to_peek?

ctb · 2014-12-18T16:03:45Z

Looks great apart from the few specific comments! Thanks!

mr-c · 2014-12-18T20:48:29Z

Ugh: Python 2.6 & Python 2.7 allow one to stream from a zipfile via /dev/stdin but not from a named pipe/FIFO. Excuse me while I find a new way to test this.

mr-c · 2014-12-18T20:49:59Z

I"m ripping out zipfile support for now and saving it in another branch for future development.

mr-c · 2014-12-19T18:26:57Z

Harsh Jenkins: why are you judging me for not having fixed all the PEP8 errors when #17 hasn't been merged yet?

SensibleSalmon · 2014-12-19T21:34:19Z

Good form? And I don't recall setting that. Jenkins should be very
tolerant. Currently.

On Fri, Dec 19, 2014, 13:26 Michael R. Crusoe notifications@github.com
wrote:

Harsh Jenkins: why are you judging me for not having fixed all the PEP8
errors when #17 #17 hasn't been
merged yet?

—
Reply to this email directly or view it on GitHub
#11 (comment).

SensibleSalmon · 2014-12-19T21:34:35Z

Which reminds me, what's #17 still need?

On Fri, Dec 19, 2014, 16:33 Jake Fenton bocajnotnef@gmail.com wrote:

Good form? And I don't recall setting that. Jenkins should be very
tolerant. Currently.

On Fri, Dec 19, 2014, 13:26 Michael R. Crusoe notifications@github.com
wrote:

Harsh Jenkins: why are you judging me for not having fixed all the PEP8
errors when #17 #17 hasn't been
merged yet?

—
Reply to this email directly or view it on GitHub
#11 (comment).

mr-c · 2014-12-19T22:18:28Z

I think I had prematurely set that.

mr-c · 2014-12-19T22:19:42Z

@ctb ready for review & merge

ctb · 2014-12-19T22:40:33Z

Nice work!

eliminate seek()s

mr-c force-pushed the fix/buffered branch from df06d9e to f37a3a8 Compare October 10, 2014 00:35

mr-c mentioned this pull request Oct 10, 2014

normalize-by-median.py does not accept reads on the stdin dib-lab/khmer#633

Closed

ctb reviewed Nov 1, 2014
View reviewed changes

mr-c reviewed Nov 4, 2014
View reviewed changes

mr-c force-pushed the fix/buffered branch from b7c9096 to 3c7186e Compare December 1, 2014 21:20

mr-c force-pushed the fix/buffered branch from d45819d to 3c7186e Compare December 2, 2014 21:58

mr-c added 8 commits December 17, 2014 10:37

eliminate seek()s

5560842

quick fix for 2.6

5f3be7c

test with zipfiles

9efebed

Fix zipfile handling; remove py ver test

c4bf58d

Since we already have a buffered reader there is no reason to further wrap the compressed file readers in another buffered reader. This frees us to support streaming with Python 2.6 as well

include test zip file

7d2b0ff

despite being buffered we aren't allowed to seek()

f041876

add tests

d20eeba

restore empty file behavior & test

1f2945d

mr-c force-pushed the fix/buffered branch from f4f8e3d to 1f2945d Compare December 17, 2014 15:38

mr-c mentioned this pull request Dec 17, 2014

state of streaming I/O in khmer, mark 2 dib-lab/khmer#700

Closed

ctb reviewed Dec 18, 2014
View reviewed changes

mr-c added 2 commits December 18, 2014 13:58

pre-fail streaming of zip archives in Python 2.6

106d1c3

pre-fail streaming of zip archives in Python 2.6 pt 2

22ada18

mr-c added 5 commits December 18, 2014 15:50

more and better tests

c5a0f57

ripout zip archive support

863443a

save the output of the .coverage target

54797f3

run develop setuptools command to install deps

f875dbb

don't run failing tests

00f78be

cover the gz fail

875e179

ctb added a commit that referenced this pull request Dec 19, 2014

Merge pull request #11 from ged-lab/fix/buffered

8238d98

eliminate seek()s

ctb merged commit 8238d98 into master Dec 19, 2014

ctb deleted the fix/buffered branch December 19, 2014 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eliminate seek()s #11

eliminate seek()s #11

mr-c commented Oct 10, 2014

mr-c commented Oct 10, 2014

ctb commented Oct 10, 2014

mr-c commented Oct 10, 2014

mr-c commented Oct 10, 2014

mr-c commented Oct 31, 2014

ctb commented Nov 1, 2014

ctb Nov 1, 2014

SensibleSalmon commented Nov 3, 2014

mr-c commented Nov 3, 2014

mr-c commented Nov 3, 2014

mr-c Nov 4, 2014

ctb Nov 4, 2014

mr-c Nov 4, 2014

SensibleSalmon commented Nov 12, 2014

mr-c commented Dec 2, 2014

SensibleSalmon commented Dec 2, 2014

mr-c commented Dec 5, 2014

mr-c commented Dec 17, 2014

ctb Dec 18, 2014

ctb commented Dec 18, 2014

mr-c commented Dec 18, 2014

mr-c commented Dec 18, 2014

mr-c commented Dec 19, 2014

SensibleSalmon commented Dec 19, 2014

SensibleSalmon commented Dec 19, 2014

mr-c commented Dec 19, 2014

mr-c commented Dec 19, 2014

ctb commented Dec 19, 2014

eliminate seek()s #11

eliminate seek()s #11

Conversation

mr-c commented Oct 10, 2014

mr-c commented Oct 10, 2014

ctb commented Oct 10, 2014

mr-c commented Oct 10, 2014

mr-c commented Oct 10, 2014

mr-c commented Oct 31, 2014

ctb commented Nov 1, 2014

--t

ctb Nov 1, 2014

Choose a reason for hiding this comment

SensibleSalmon commented Nov 3, 2014

mr-c commented Nov 3, 2014

mr-c commented Nov 3, 2014

mr-c Nov 4, 2014

Choose a reason for hiding this comment

ctb Nov 4, 2014

Choose a reason for hiding this comment

- None: lambda: _open(filename)}compression

- line = sequencefile.readline()

--titus

mr-c Nov 4, 2014

Choose a reason for hiding this comment

SensibleSalmon commented Nov 12, 2014

mr-c commented Dec 2, 2014

SensibleSalmon commented Dec 2, 2014

mr-c commented Dec 5, 2014

mr-c commented Dec 17, 2014

ctb Dec 18, 2014

Choose a reason for hiding this comment

ctb commented Dec 18, 2014

mr-c commented Dec 18, 2014

mr-c commented Dec 18, 2014

mr-c commented Dec 19, 2014

SensibleSalmon commented Dec 19, 2014

SensibleSalmon commented Dec 19, 2014

mr-c commented Dec 19, 2014

mr-c commented Dec 19, 2014

ctb commented Dec 19, 2014