initial stab at exception handling around file operations #333

ctb · 2014-03-19T04:52:15Z

No description provided.

ged-jenkins · 2014-03-19T04:54:52Z

Test PASSed.
Refer to this link for build results: http://ci.ged.msu.edu/job/khmer-multi-pullrequest/295/

RamRS · 2014-03-19T13:23:43Z

Are there other file IO operations in the C++ code that need wrapping?

ctb · 2014-03-19T13:25:27Z

On Wed, Mar 19, 2014 at 06:23:44AM -0700, Ram RS wrote:

Are there other file IO operations that need wrapping?

Yes, of course, but I want to get the approach worked out first.

RamRS · 2014-03-19T14:01:32Z

Hmmm, so you have given a demo on how to handle file input on wholesale
consumption as well as streaming input. I'll take a look and see if I can
find spots that we can apply these constructs to.

Would you be willing to go over a list I prepare of candidate spots? I'd
rather not mess up the C++ code without adult supervision.

Ram

On Wed, Mar 19, 2014 at 9:25 AM, C. Titus Brown notifications@github.comwrote:

On Wed, Mar 19, 2014 at 06:23:44AM -0700, Ram RS wrote:

Are there other file IO operations that need wrapping?

Yes, of course, but I want to get the approach worked out first.

Reply to this email directly or view it on GitHubhttps://github.com//pull/333#issuecomment-38049681
.

ctb · 2014-03-19T14:02:59Z

On Wed, Mar 19, 2014 at 07:01:32AM -0700, Ram RS wrote:

Hmmm, so you have given a demo on how to handle file input on wholesale
consumption as well as streaming input. I'll take a look and see if I can
find spots that we can apply these constructs to.

Would you be willing to go over a list I prepare of candidate spots? I'd
rather not mess up the C++ code without adult supervision.

Please feel free to prepare such a list, but I might ignore it :)

RamRS · 2014-03-19T18:12:29Z

I'm gonna go out on a limb and wrap most (if not all) c_str(file,ios::X) operations.

Quick question: would it not be easier to wrap the open() lines in the underlying read_parsers.cc and other source files?

ctb · 2014-03-19T19:05:42Z

On Wed, Mar 19, 2014 at 11:12:28AM -0700, Ram RS wrote:

I'm gonna go out on a limb and wrap most (if not all) c_str(file,ios::X) operations.

Quick question: would it not be easier to wrap the open() lines in the underlying read_parsers.cc and other source files?

Ram, please don't step on my work; it's preliminary. Thanks.

RamRS · 2014-03-19T19:10:50Z

I wasn't planning to actually start working on this, just figured we could discuss. I apologize.

I think if we could assign issues (at least those opened by repo collaborators for themselves), that would avoid this confusion.

mr-c · 2014-03-19T19:58:12Z

@RamRS We use pull requests as a way of sharing on-going development, mostly between @ctb and those people he supervises.

I change the assignment of an issue when someone has claimed it. One could even use the Assignee field of a pull request to signal their readiness for code review.

RamRS · 2014-03-19T20:02:38Z

Ah, I understand now. I apologize for this oversight. It was foolish of me to assume that this PR was a pilot/demo to a branch of work and not an ongoing piece in and of itself.

ged-jenkins · 2014-04-01T22:41:04Z

Test FAILed.
Refer to this link for build results: http://ci.ged.msu.edu/job/khmer-multi-pullrequest/442/

ged-jenkins · 2014-04-05T02:08:24Z

Test FAILed.
Refer to this link for build results: http://ci.ged.msu.edu/job/khmer-multi-pullrequest/458/

ctb · 2014-05-18T09:38:50Z

Also see #411.

…g_file_exceptions Conflicts: lib/counting.cc

…ions

ctb · 2014-05-18T22:27:57Z

Here is a brief description of the strategy, implemented (for now) in counting.cc, CountingHashFileReader constructor:

use ifstream::exceptions() to configure ifstream to throw an exception for eof or failure;
catch the resulting exception in C++ code and raise a custom C++ exception with our own message;
catch this custom exception in _khmermodule and pass it along to Python as an IOError with the appropriate message;
write tests that involve reading in truncated hashtables;

@mr-c and I talked about why we shouldn't just catch the ifstream exceptions in _khmermodule. The reason I did it this way is so that we can add in more specific error messages in the function that is actually doing the loading: _khmermodule will have no real context for why an exception is being thrown, while the code in counting.cc may be able to figure it out and provide a more useful error message to the Python level. (See last link in extra reading -- there are no special format conventions for figuring out what an exception means here.)

Extra reading:

@mr-c, @camillescott, @luizirber - any comments on this strategy? I can apply it to all input streams easily enough.

ctb · 2014-05-18T22:32:47Z

Probably addresses #178, #247 and #95. Also see #246 and #411.

mr-c · 2014-05-20T20:57:44Z

The approach and the code looks good to me.

mr-c · 2014-05-30T00:05:02Z

On Thu, May 29, 2014 at 7:39 PM, C. Titus Brown notifications@github.com
wrote:

I am still confused about the coverage report.

The cobertura report doesn't show me individual lines that are or aren't
covered. How can I generate that? Is there a good reason not to generate
that for each commit?

If you log in to Jenkins the cobertura report will show the source for the
most recent build**.

** Yes, this causes problems when multiple pull requests are being updated
in a short time

Am I supposed to run the 'make' commands, above, locally on my laptop, as
#251 #251 implies?

Developers can also use git-coverage or diff-cover to show the same
information.

I've added a Makefile target in this branch that automates the diff-cover
approach; use either diff-cover or diff-cover.html

mr-c · 2014-05-30T00:05:22Z

The HTML diff-cover report is available at http://athyra.ged.msu.edu/~mcrusoe/diff-cover.html if you have problems running it locally.

ctb · 2014-05-30T07:32:54Z

OK, I see, so yes, I need run the coverage on my own. We'll have to document all of this for the hackathon, I think.

ctb · 2014-05-30T07:34:16Z

(I've forgotten how to log into Jenkins, if I ever knew; but it seems like the coverage report should be publicly available, no?)

ctb · 2014-05-30T10:48:08Z

Hallelujah!! Quick, @mr-c, merge it before it breaks again :)

mr-c · 2014-05-30T14:06:05Z

tests/test_hashbits.py

+    except IOError, e:
+        print str(e)
+
+def test_hashbits_file_type_check():


tests/test_hashbits.py:682:1: E302 expected 2 blank lines, found 1

mr-c · 2014-05-30T15:33:51Z

On Fri, May 30, 2014 at 3:32 AM, C. Titus Brown notifications@github.com
wrote:

OK, I see, so yes, I need run the coverage on my own. We'll have to
document all of this for the hackathon, I think.

Yep! I've make a checklist of issues to target for a mid-July release:
#440

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

mr-c · 2014-05-30T15:34:41Z

On Fri, May 30, 2014 at 3:34 AM, C. Titus Brown notifications@github.com
wrote:

(I've forgotten how to log into Jenkins, if I ever knew; but it seems like
the coverage report should be publicly available, no?)

There's a link in the upper right corner that uses your GitHub credentials.
I agree it should be publicly available. I'll file a ticket.

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

mr-c · 2014-05-30T15:36:38Z

I'm going to fix the spacing error & squash this down to a handful of commits instead of 55

initial stab at exception handling around file operations

mr-c · 2014-05-30T16:47:35Z

I switched to a matrix-based authorization scheme and now non-logged-in
users can see the coverage reports with code highlighting. Tested by
loading
http://ci.ged.msu.edu/job/khmer-pullrequest/107/label=linux/cobertura/khmer/_khmermodule_cc/
into a incognito browser window.

Jenkins issue is at https://issues.jenkins-ci.org/browse/JENKINS-23254

On Fri, May 30, 2014 at 11:34 AM, Michael Crusoe michael.crusoe@gmail.com
wrote:

On Fri, May 30, 2014 at 3:34 AM, C. Titus Brown notifications@github.com
wrote:

(I've forgotten how to log into Jenkins, if I ever knew; but it seems
like the coverage report should be publicly available, no?)

There's a link in the upper right corner that uses your GitHub credentials.
I agree it should be publicly available. I'll file a ticket.

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

ctb · 2014-05-31T11:01:48Z

Thanks, @mr-c, for all your work on this! Question -- should the squashing be part of our standard dev workflow? I'm -0 on it as a general thing (because I'm scared of git rebase) but +0 on it being your call. Either way it should be mentioned as an option in our dev docs (perhaps added as part of #440).

mr-c · 2014-05-31T23:08:07Z

Personally I git fetch && git rebase origin/master instead of git merging. That makes a later commit squash via git rebase -i origin/master easy.

For this PR it was too messy for me to clean up. shrug It is a strong preference but I haven't come up with a sure-fire way to use it. I'm going to punt on this for now.

mr-c · 2015-02-26T22:33:03Z

tests/test_hashbits.py

+    ht.add_tag('A' * 32)
+    ht.add_tag('G' * 32)
+    ht.save_tagset(outfile)
+    ht.save_tagset('/tmp/goodversion-k32.tagset')


Hmm.. I don't think this line should be in the test.

initial stab at exception handling around file operations

86ded16

mr-c added this to the 1.0 release milestone Mar 19, 2014

mr-c modified the milestones: 1.1+ Release, 1.0 release Apr 2, 2014

ctb added 7 commits May 18, 2014 10:41

Merge branch 'master' of github.com:ged-lab/khmer into feature/missin…

758b5e8

…g_file_exceptions Conflicts: lib/counting.cc

added in (untested) file error testing in hashtable load

e62c3a7

added test for loading truncated counting hash; results in infinite loop

1e61681

Merge branch 'tests/file_exceptions' into feature/missing_file_except…

dd0ba97

…ions

test for IOError on attempt to load truncated k-mer table

a38ae7e

marked source files that need exception handling for reads

cce7c8b

shifted eof into general catch block

41352f1

ctb added 3 commits May 22, 2014 11:37

Merge branch 'master' into tests/file_exceptions

4feefa8

Merge branch 'master' into feature/missing_file_exceptions

a239174

removed highmem attr from truncated file load test

96c42e0

ctb added 7 commits May 30, 2014 09:44

remove merge2* functions: unused, untested

4ce97ce

added test for counting hash gzload of wrong file type

1bfef99

added test for counting hash load of wrong file type

b891ec2

removed unnecessary hashbits creation

3c50997

added test for wrong file format type to test_hashbits

2af259c

added tests for file type check on tagset and stop_tags load

06f7bb0

added tests for pmap loading/file type and version

a1d3388

mr-c reviewed May 30, 2014
View reviewed changes

autopep8

3b06508

mr-c added a commit that referenced this pull request May 30, 2014

Merge pull request #333 from ged-lab/feature/missing_file_exceptions

34d6b06

initial stab at exception handling around file operations

mr-c merged commit 34d6b06 into master May 30, 2014

mr-c deleted the feature/missing_file_exceptions branch May 30, 2014 17:03

ctb mentioned this pull request May 31, 2014

Update C++ writing code to handle write exceptions properly. #443

Closed

mr-c reviewed Feb 26, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial stab at exception handling around file operations #333

initial stab at exception handling around file operations #333

ctb commented Mar 19, 2014

ged-jenkins commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

mr-c commented Mar 19, 2014

RamRS commented Mar 19, 2014

ged-jenkins commented Apr 1, 2014

ged-jenkins commented Apr 5, 2014

ctb commented May 18, 2014

ctb commented May 18, 2014

ctb commented May 18, 2014

mr-c commented May 20, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

ctb commented May 30, 2014

ctb commented May 30, 2014

ctb commented May 30, 2014

mr-c May 30, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

ctb commented May 31, 2014

mr-c commented May 31, 2014

mr-c Feb 26, 2015

initial stab at exception handling around file operations #333

initial stab at exception handling around file operations #333

Conversation

ctb commented Mar 19, 2014

ged-jenkins commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

ctb commented Mar 19, 2014

RamRS commented Mar 19, 2014

mr-c commented Mar 19, 2014

RamRS commented Mar 19, 2014

ged-jenkins commented Apr 1, 2014

ged-jenkins commented Apr 5, 2014

ctb commented May 18, 2014

ctb commented May 18, 2014

ctb commented May 18, 2014

mr-c commented May 20, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

ctb commented May 30, 2014

ctb commented May 30, 2014

ctb commented May 30, 2014

mr-c May 30, 2014

Choose a reason for hiding this comment

mr-c commented May 30, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

mr-c commented May 30, 2014

ctb commented May 31, 2014

mr-c commented May 31, 2014

mr-c Feb 26, 2015

Choose a reason for hiding this comment