added screed and read_parser streaming testing #644

bocajnotnef · 2014-10-30T20:48:52Z

Marked as known failing since they currently fail.
Will investigate.

Relevant to #393, #654

mr-c · 2014-10-30T21:00:04Z

tests/test_scripts.py

+    # create the subprocess of the script
+    scriptp = \
+        subprocess.Popen(['normalize-by-median.py -C 2 -k 17 /dev/stdin'],
+                shell=True, stdin=subprocess.PIPE, stderr=subprocess.PIPE)


When I run this on the command line with screed 0.7 and 0.7.1 it works. Maybe using a FIFO named pipe (os.mkfifo) like we did when we were debugging with GDB would do the trick. That way you can still invoke the script using utils.runscript like the rest of the tests.

Ah, I was using redirection instead of a pipe which evidently matters (??).

bocajnotnef · 2014-10-31T17:24:59Z

Augment testing for empty files in khmer/file.py to not test for empty on block devices

mr-c · 2014-11-03T00:10:45Z

Retest this please

bocajnotnef · 2014-11-03T16:11:51Z

Tests redesigned to use fifos within python to hold to the current testing structure (i.e. runscript())

Next up:

Fasta test
Fastq test
gzip test (fasta, fastq)
bzip test (fasta, fastq)

bocajnotnef · 2014-11-04T20:05:31Z

Breakdown of which script uses what reading library:

screed:

count-median
extract-long
extract-paired
extract-partitions
fastq-to-fasta
interleave-reads
normalize-by-median
sample-reads-randomly
split-paired-reads

readparser:

abundance-dist-single
filter-abund-single
load-graph
load-into-counting

mr-c · 2014-11-05T00:49:51Z

retest this please

bocajnotnef · 2014-11-11T21:05:23Z

retest this please

bocajnotnef · 2014-11-11T21:16:10Z

retest this please

bocajnotnef · 2014-11-11T21:24:29Z

Should be noted that these tests are all marked known-failing because we need lower level seqan for streaming to work and (at least) screed 0.7.1 for non-gzip streaming to work in screed.

In order to verify that these work you'd have to edit the setup.cfg to run known_failing tests and to not stop on a test failure and then run the test_scripts.py stuff manually as documented in the docs.

Is it mergable
Did it pass the tests?
If it introduces new functionality in scripts/ is it tested?
Check for code coverage.
Is it well formatted? Look at pep8/pylint, cppcheck, and
make doc output. Use autopep8 and astyle -A10 --max-code-length=80
if needed.
Is it documented in the Changelog?
Was spellcheck run on the source code and documentation after changes
were made?

@brtaylor92, @b-wyss, @camillescott, CR please?

ctb · 2014-11-13T15:07:33Z

khmer/file.py



 def check_file_status(file_path):
    """
    Check status of file - return if file exists; warn and exit
    if empty, or does not exist
+    This check will return if the file being checked is a block device
+    This check will return if the file being checked is a fifo


Pls combine sentences.

bocajnotnef · 2014-11-14T17:39:22Z

@brtaylor92 @b-wyss @wrightmhw Requesting CR

ctb · 2014-11-14T18:30:55Z

khmer/file.py

+
+    mode = os.stat(file_path).st_mode
+    # block devices will be nonzero
+    if S_ISBLK(mode):


why not

if S_ISBLK(mode) or S_ISFIFO(mode):

?

Mm. Good point.

ctb · 2014-11-14T18:32:25Z

A few more comments, but otherwise LGTM.

bocajnotnef · 2014-11-14T18:42:42Z

Comments as in add more comments in the code, or...?

ctb · 2014-11-14T18:50:41Z

On Fri, Nov 14, 2014 at 10:42:42AM -0800, bocajnotnef wrote:

Comments as in add more comments in the code, or...?

I made more comments that you should look at!

bocajnotnef · 2014-11-14T18:54:46Z

Ah. Sorry. git inline formatting confusing. Resolving comments.

bocajnotnef · 2014-11-15T22:10:52Z

Really not a fan of merge conflicts.

@ctb Should be good for final pass. Hopefully.

ctb · 2014-11-15T22:12:56Z

khmer/file.py

@@ -11,13 +11,21 @@

 import os
 import sys
+from stat import *


Hmm, just import S_ISBLK and S_ISFIFO here.

ctb · 2014-11-15T22:13:50Z

Apart from that one comment, LGTM. Suggest waiting for @mr-c to take a look as he is more familiar with issues.

mr-c · 2014-11-16T01:25:43Z

tests/test_scripts.py

@@ -446,6 +451,7 @@ def test_normalize_by_median_dumpfrequency():
    assert 'Nothing' in out


+@attr('known_failing')


Why is an existing test now marked as a known failure?

bocajnotnef · 2014-11-16T02:40:38Z

@mr-c: Resolved, methinks. All good?

mr-c · 2014-11-16T19:08:53Z

khmer/file.py

-    Check status of file - return if file exists; warn and exit
-    if empty, or does not exist
-    """
+    return


This return disable the entire method

…n_failing since they fail with existing systems. non-gzip streaming works in screed 0.7.1

Cleaned up formatting in streaming tests. Renamed streaming test helper functions to something more explicit. Cleaned up fifo/blk testing logic structure to be cleaner Added docstrings to helper functions to better explain functionality Cleaned up some comments

Cleared up logical structure

removed extraneous known_failing attr

bocajnotnef · 2014-11-16T20:13:51Z

Commit discontinuity due to forcing with a merge conflict fix.

@mr-c Asserts/spacing resolved.

added screed and read_parser streaming testing

mr-c · 2014-11-16T20:17:17Z

Good job, @bocajnotnef !

bocajnotnef · 2014-11-16T20:17:32Z

Yaaaay!

mr-c · 2014-11-16T21:22:15Z

retest this please

mr-c · 2014-11-16T21:22:35Z

please test this

mr-c · 2014-11-25T02:56:29Z

tests/test_scripts.py

+def execute_abund_dist_single_streaming(ifilename, somedir=None):
+    '''Helper function for the matrix of streaming tests using screed via
+    filter-abund-single, i.e. uncompressed fasta, gzip fasta, bz2 fasta,
+    uncompressed fastq, etc.


Also this comment is wrong (not using screed, wrong script name)

mr-c reviewed Oct 30, 2014
View reviewed changes

bocajnotnef added Python theme:best-practices labels Nov 4, 2014

bocajnotnef self-assigned this Nov 4, 2014

mr-c mentioned this pull request Nov 5, 2014

first pass seqan impl #642

Merged

bocajnotnef force-pushed the testing/streaming branch from 625bf2e to bc2b82f Compare November 6, 2014 15:54

bocajnotnef force-pushed the testing/streaming branch from 15ab059 to e8f6c30 Compare November 11, 2014 21:14

ctb reviewed Nov 13, 2014
View reviewed changes

ctb reviewed Nov 14, 2014
View reviewed changes

ctb reviewed Nov 15, 2014
View reviewed changes

khmer/file.py

@@ -11,13 +11,21 @@

import os

import sys

from stat import *

Copy link

Member

ctb Nov 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, just import S_ISBLK and S_ISFIFO here.

mr-c reviewed Nov 16, 2014
View reviewed changes

bocajnotnef added 6 commits November 16, 2014 14:17

Added screed and read_parser stream testing. Currently marked as know…

52ffdcc

…n_failing since they fail with existing systems. non-gzip streaming works in screed 0.7.1

Updated docstring formatting

2c1ea64

Cleared up logical structure

updated docstring to be clearer.

de6ae25

Reducing import bloat

8eb88f4

updated comments

7803f9b

removed extraneous known_failing attr

bocajnotnef force-pushed the testing/streaming branch from 042de1e to 7803f9b Compare November 16, 2014 19:22

made read_parser stream test asserts more intensive

2486e26

mr-c added a commit that referenced this pull request Nov 16, 2014

Merge pull request #644 from ged-lab/testing/streaming

91a0308

added screed and read_parser streaming testing

mr-c merged commit 91a0308 into master Nov 16, 2014

mr-c deleted the testing/streaming branch November 16, 2014 20:17

mr-c restored the testing/streaming branch November 16, 2014 21:22

mr-c reviewed Nov 25, 2014
View reviewed changes

mr-c deleted the testing/streaming branch April 15, 2015 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added screed and read_parser streaming testing #644

added screed and read_parser streaming testing #644

bocajnotnef commented Oct 30, 2014

mr-c Oct 30, 2014

mr-c Oct 31, 2014

bocajnotnef commented Oct 31, 2014

mr-c commented Nov 3, 2014

bocajnotnef commented Nov 3, 2014

bocajnotnef commented Nov 4, 2014

mr-c commented Nov 5, 2014

bocajnotnef commented Nov 11, 2014

bocajnotnef commented Nov 11, 2014

bocajnotnef commented Nov 11, 2014

ctb Nov 13, 2014

bocajnotnef commented Nov 14, 2014

ctb Nov 14, 2014

bocajnotnef Nov 14, 2014

ctb commented Nov 14, 2014

bocajnotnef commented Nov 14, 2014

ctb commented Nov 14, 2014

bocajnotnef commented Nov 14, 2014

bocajnotnef commented Nov 15, 2014

ctb Nov 15, 2014

ctb commented Nov 15, 2014

mr-c Nov 16, 2014

bocajnotnef commented Nov 16, 2014

mr-c Nov 16, 2014

bocajnotnef commented Nov 16, 2014

mr-c commented Nov 16, 2014

bocajnotnef commented Nov 16, 2014

mr-c commented Nov 16, 2014

mr-c commented Nov 16, 2014

mr-c Nov 25, 2014

		@@ -446,6 +451,7 @@ def test_normalize_by_median_dumpfrequency():
		assert 'Nothing' in out


		@attr('known_failing')

added screed and read_parser streaming testing #644

added screed and read_parser streaming testing #644

Conversation

bocajnotnef commented Oct 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocajnotnef commented Oct 31, 2014

mr-c commented Nov 3, 2014

bocajnotnef commented Nov 3, 2014

bocajnotnef commented Nov 4, 2014

mr-c commented Nov 5, 2014

bocajnotnef commented Nov 11, 2014

bocajnotnef commented Nov 11, 2014

bocajnotnef commented Nov 11, 2014

Choose a reason for hiding this comment

bocajnotnef commented Nov 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctb commented Nov 14, 2014

bocajnotnef commented Nov 14, 2014

ctb commented Nov 14, 2014

bocajnotnef commented Nov 14, 2014

bocajnotnef commented Nov 15, 2014

Choose a reason for hiding this comment

ctb commented Nov 15, 2014

Choose a reason for hiding this comment

bocajnotnef commented Nov 16, 2014

Choose a reason for hiding this comment

bocajnotnef commented Nov 16, 2014

mr-c commented Nov 16, 2014

bocajnotnef commented Nov 16, 2014

mr-c commented Nov 16, 2014

mr-c commented Nov 16, 2014

Choose a reason for hiding this comment