initial work for issue 399: add --force flag #647

jessicalumian · 2014-11-04T21:52:31Z

ged-jenkins · 2014-11-04T21:52:31Z

Can one of the admins verify this patch?

ged-jenkins · 2014-11-04T21:52:31Z

Can one of the admins verify this patch?

mr-c · 2014-11-04T21:53:09Z

scripts/sample-reads-randomly.py

@@ -102,7 +104,8 @@ def main():
        if num_samples > 1:
            sys.stderr.write(
                "Error: cannot specify -o with more than one sample.")
-            sys.exit(-1)


Yep, this should be "1" not "-1"

mr-c · 2014-11-04T21:53:29Z

okay to test

ctb · 2014-11-04T21:54:05Z

Note, there seem to be some spacing/indentation issues - the indentation is not always consistent with your added lines.

jessicalumian · 2014-11-04T22:02:49Z

@ctb well aren't you fast. Fixed.

SensibleSalmon · 2014-11-04T22:03:23Z

there appear to be some idle time issues with the @ctb process....

mr-c · 2014-11-05T00:50:19Z

okay to test

mr-c · 2014-11-05T00:50:29Z

please test this

camillescott · 2014-11-05T00:52:49Z

scripts/do-partition.py

@@ -98,6 +98,8 @@ def get_parser():
    parser.add_argument('graphbase', help="base name for output files")
    parser.add_argument('input_filenames', metavar='input_sequence_filename',
                        nargs='+', help='input FAST[AQ] sequence filenames')
+    parser.add_argument('-f', '-force', default=False, action='store_true',


Long form arguments should use --, ie --force.

jessicalumian · 2014-11-06T15:40:48Z

Fixed more errors, including what @camillescott pointed out, but I want to look at it one more time to make sure everything looks good. Did I successfully push the new changes through?

ctb · 2014-11-06T16:07:36Z

On Thu, Nov 06, 2014 at 07:40:48AM -0800, jessicamizzi wrote:

Fixed more errors, including what @camillescott pointed out, but I want to look at it one more time to make sure everything looks good. Did I successfully push the new changes through?

Can you see them in the diff view?

--titus

C. Titus Brown, ctb@msu.edu

mr-c · 2014-11-08T17:35:53Z

scripts/load-into-counting.py

@@ -130,7 +132,8 @@ def main():
        print >> sys.stderr, ("** ERROR: the k-mer counting table is too small"
                              " this data set.  Increase tablesize/# tables.")
        print >> sys.stderr, "**"
-        sys.exit(1)


This particular use of force won't have any effect on script execution as it is at the last stage. Lines 72-75 need addressing. We probably need to add a force parameter to the check_file_status(), check_space(), and check_space_for_hashtable() functions.

mr-c · 2014-11-14T18:09:04Z

Tagging the source issue: #399

mr-c · 2014-11-14T18:13:24Z

Testing may be a bit tricky. To trigger code paths that a --force command line flag would enable may require slight refactoring of the scripts. Monkeypatching may be required to avoid system crashes by replacing functionality with empty functions. @ctb, @luizirber is there an easier way to Mock, or a better testing approach?

ctb · 2014-11-15T15:01:36Z

khmer/load_pe.py

@@ -23,7 +23,7 @@ def load_pe(screed_handle):

    screed_iter = iter(screed_handle)

-    while 1:
+    while True:


Why this change? 'while 1' is perfectly idiomatic in Python...?

This may be an 'autopep8'-ism

ctb · 2014-11-15T15:06:06Z

@mr-c what specifically are you thinking of re crashes? On a quick skim I didn't see anything that would be tough to test.

mr-c · 2014-11-16T01:34:48Z

@ctb The force argument needs to be passed to check_file_status(), check_space(), and check_space_for_hashtable() (see #647 (comment))

As far as I can see these will require Mocking to test.

ctb · 2014-11-16T11:55:23Z

Ahh, OK. There are several ways to go about this --

we can make a special 'testing' flag that lets us test the behavior. Ugly.
we can test it by importing the script, monkeypatching check_file_status, setting up sys.args, etc. - eschewing the execfile approach to script testing in this case

...and I think those are the best of the options. The latter is the way to do it, I think, and it fits with our long term plans (of moving most script code out of scripts/ into an importable place).

We don't have any example code in khmer that already does this, but the basic routine is

module = __import__('script-name')
module.main()

and then place the monkeypatching in a try/finally, e.g.

 module = __import__(...)
 try:
   saved_fn = khmer.check_args.check_file_status
   khmer.check_args.check_file_status = new_fn

   # run test code
 finally:
     khmer.check_args.check_file_status = saved_fn

mr-c · 2014-11-16T19:07:36Z

@jessicamizzi Once again a "simple" issue has turned out to be more complex. Sorry! I am happy to write the tests for you if you want.

jessicalumian · 2014-11-17T01:35:34Z

@mr-c if there are any simple ones, I can do them. Otherwise, it might be best if you do them. I will make the other changes you talked about earlier on this page, but I will tomorrow.

jessicalumian · 2014-12-05T18:58:03Z

Can two people review these changes?

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?

SensibleSalmon · 2014-12-05T19:54:49Z

ChangeLog

-
-    * Doxyfile.in: add links to the stdc++ docs
+2014-12-05  Jessica Mizzi  <mizzijes@msu.edu>
+    * khmer/file.py,sandbox/sweep-reads.py,scripts/{abundance-dist-single,abundance-dist,annotate-partitions,count-median,count-overlap,do-partition,extract-paired-reads,extract-partitions,filter-abund-single,filter-abund,filter-stoptags,interleave-reads,load-graph,load-into-counting,make-initial-stoptags,merge-partitions,normalize-by-median,partition-graph,sample-reads-randomly,split-paired-reads}.py,setup.cfg,tests/{test_script_arguments,test_scripts}.py: Added force option to all scripts to script IO sanity checks and updated tests to match. 


This should be broken up so it doesn't run over the 80 char line.

Also, don't remove @mr-c entry.

SensibleSalmon · 2014-12-05T20:00:45Z

Jenkins, test this please

SensibleSalmon · 2014-12-05T20:05:13Z

Jenkins is failing (unstable) but it wasn't test related. I'm guessing pep8?

luizirber · 2014-12-05T20:06:13Z

Yup: http://ci.ged.msu.edu/job/khmer-pullrequest/879/label=linux/violations/

…s.force

…ted tests to match

initial work for issue 399: add --force flag

mr-c · 2014-12-19T19:36:57Z

@jessicamizzi, Congratulations on your first commit to the khmer project! Your name will be included in the release notes for the next version and you'll be listed amongst our other contributors in the next software release paper.

mr-c reviewed Nov 4, 2014
View reviewed changes

camillescott reviewed Nov 5, 2014
View reviewed changes

mr-c reviewed Nov 8, 2014
View reviewed changes

ctb reviewed Nov 15, 2014
View reviewed changes

ctb changed the title ~~initial work for issue 399~~ initial work for issue 399: add --force flag Nov 22, 2014

mr-c added this to the 1.2+ milestone Dec 1, 2014

SensibleSalmon reviewed Dec 5, 2014
View reviewed changes

Jessica Mizzi added 7 commits December 19, 2014 13:50

initial work for issue 399

d56d5db

autopep8

20ee270

fixed syntax errors in force parser and fixed 2 other errors with arg…

1655683

…s.force

fixed some formatting and combined 2 if statements into 1

7b24ad2

Fixed typo

12b196d

Fixed typos

ab123c0

Added force option to all scripts to script IO sanity checks and upda…

eea8efa

…ted tests to match

jessicalumian force-pushed the fix/check-override branch from 47e8890 to eea8efa Compare December 19, 2014 19:14

pep8

4bab69a

mr-c added a commit that referenced this pull request Dec 19, 2014

Merge pull request #647 from jessicamizzi/fix/check-override

ffd0691

initial work for issue 399: add --force flag

mr-c merged commit ffd0691 into dib-lab:master Dec 19, 2014

mr-c modified the milestones: 1.3+, 1.3 Dec 19, 2014

mr-c mentioned this pull request Dec 19, 2014

Add --force or --expert or --ignore flag to all khmer scripts that do sanity checking #399

Closed

ctb mentioned this pull request Feb 25, 2015

Regarding abundance-dist.py Documentation #848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial work for issue 399: add --force flag #647

initial work for issue 399: add --force flag #647

jessicalumian commented Nov 4, 2014

ged-jenkins commented Nov 4, 2014

ged-jenkins commented Nov 4, 2014

mr-c Nov 4, 2014

mr-c commented Nov 4, 2014

ctb commented Nov 4, 2014

jessicalumian commented Nov 4, 2014

SensibleSalmon commented Nov 4, 2014

mr-c commented Nov 5, 2014

mr-c commented Nov 5, 2014

camillescott Nov 5, 2014

jessicalumian commented Nov 6, 2014

ctb commented Nov 6, 2014

mr-c Nov 8, 2014

mr-c commented Nov 14, 2014

mr-c commented Nov 14, 2014

ctb Nov 15, 2014

mr-c Nov 16, 2014

ctb commented Nov 15, 2014

mr-c commented Nov 16, 2014

ctb commented Nov 16, 2014

mr-c commented Nov 16, 2014

jessicalumian commented Nov 17, 2014

jessicalumian commented Dec 5, 2014

SensibleSalmon Dec 5, 2014

luizirber Dec 5, 2014

SensibleSalmon commented Dec 5, 2014

SensibleSalmon commented Dec 5, 2014

luizirber commented Dec 5, 2014

mr-c commented Dec 19, 2014

initial work for issue 399: add --force flag #647

initial work for issue 399: add --force flag #647

Conversation

jessicalumian commented Nov 4, 2014

ged-jenkins commented Nov 4, 2014

ged-jenkins commented Nov 4, 2014

mr-c Nov 4, 2014

Choose a reason for hiding this comment

mr-c commented Nov 4, 2014

ctb commented Nov 4, 2014

jessicalumian commented Nov 4, 2014

SensibleSalmon commented Nov 4, 2014

mr-c commented Nov 5, 2014

mr-c commented Nov 5, 2014

camillescott Nov 5, 2014

Choose a reason for hiding this comment

jessicalumian commented Nov 6, 2014

ctb commented Nov 6, 2014

--titus

mr-c Nov 8, 2014

Choose a reason for hiding this comment

mr-c commented Nov 14, 2014

mr-c commented Nov 14, 2014

ctb Nov 15, 2014

Choose a reason for hiding this comment

mr-c Nov 16, 2014

Choose a reason for hiding this comment

ctb commented Nov 15, 2014

mr-c commented Nov 16, 2014

ctb commented Nov 16, 2014

mr-c commented Nov 16, 2014

jessicalumian commented Nov 17, 2014

jessicalumian commented Dec 5, 2014

SensibleSalmon Dec 5, 2014

Choose a reason for hiding this comment

luizirber Dec 5, 2014

Choose a reason for hiding this comment

SensibleSalmon commented Dec 5, 2014

SensibleSalmon commented Dec 5, 2014

luizirber commented Dec 5, 2014

mr-c commented Dec 19, 2014