Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Python exception handling in user provided functions #2194

Merged
merged 1 commit into from
May 14, 2018

Conversation

JONBRWN
Copy link
Contributor

@JONBRWN JONBRWN commented May 10, 2018

Adds null checks around the returned object of user provided functions
and fails if the returned object is NULL. This stops us from
segfaulting and provides an immediate error as opposed to the error
propagating up further down the pipeline when we attempt to use the
NULL object.

Addresses: #2180 #2184
I'm pretty sure I got all cases, but if I missed something, let me know.

To test, apply this diff and comment in the exception you want to test:

index 796de72..029813c 100644
--- a/examples/python/market_spread/market_spread.py
+++ b/examples/python/market_spread/market_spread.py
@@ -94,6 +94,7 @@ class SymbolData(object):
 
 @wallaroo.partition
 def symbol_partition_function(data):
+    # raise Exception("Exception on partition U64!\n")
     return str_to_partition(data.symbol)
 
 
diff --git a/examples/python/word_count/word_count.py b/examples/python/word_count/word_count.py
index 140d71e..42d7534 100644
--- a/examples/python/word_count/word_count.py
+++ b/examples/python/word_count/word_count.py
@@ -41,6 +41,7 @@ def application_setup(args):
 
 @wallaroo.computation_multi(name="split into words")
 def split(data):
+    # raise Exception("Exception in Split!\n")
     punctuation = " !\"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~"
 
     words = []
@@ -56,12 +57,14 @@ def split(data):
 
 @wallaroo.state_computation(name="Count Word")
 def count_word(word, word_totals):
+    # raise Exception("Exception in state computation!\n")
     word_totals.update(word)
     return (word_totals.get_count(word), True)
 
 
 class WordTotals(object):
     def __init__(self):
+        # raise Exception("Exception in State init!\n")
         self.word_totals = {}
 
     def update(self, word):
@@ -76,12 +79,14 @@ class WordTotals(object):
 
 class WordCount(object):
     def __init__(self, word, count):
+        # raise Exception("Exception in State init!\n")
         self.word = word
         self.count = count
 
 
 @wallaroo.partition
 def partition(data):
+    # raise Exception("Exception in partition!\n")
     if data[0] >= "a" and data[0] <= "z":
         return data[0]
     else:
@@ -90,9 +95,11 @@ def partition(data):
 
 @wallaroo.decoder(header_length=4, length_fmt=">I")
 def decoder(bs):
+    # raise Exception("Exception in decoder!\n")
     return bs.decode("utf-8")
 
 
 @wallaroo.encoder
 def encoder(data):
+    # raise Exception("Exception in encoder!\n")
     return data.word + " => " + str(data.count) + "\n"

@JONBRWN JONBRWN requested a review from aturley May 10, 2018 16:43
@aturley
Copy link
Contributor

aturley commented May 11, 2018

We should also do these checks in PyStateBuilder.apply(). That would let us address #1732, which is basically part of this and also if I recall correctly is specifically a problem @SeanTAllen ran into when he was recently trying to build an application.

@aturley
Copy link
Contributor

aturley commented May 11, 2018

The current work looks fine. A few if statements in the Pony code shouldn't have a major impact on performance, so I don't think we need to worry about testing specifically for that, but if others disagree then I'll defer to them.

@SeanTAllen
Copy link
Contributor

I think we should do a single worker test to make sure there is no large performance change. This is all hotpath.

@JONBRWN
Copy link
Contributor Author

JONBRWN commented May 11, 2018

@aturley addressed in the last commit, can be confirmed by including the exception in WordTotals. IF this is good with that, I'll do a single worker test

@@ -154,7 +154,9 @@ class PyStateBuilder
_state_builder = state_builder

fun apply(): PyState =>
PyState(@state_builder_build_state(_state_builder))
let py_state = @state_builder_build_state(_state_builder)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to call print_errors().

@aturley
Copy link
Contributor

aturley commented May 11, 2018

Other than the comment about printing errors this looks good. Once you fix that you should do a single worker test.

@JONBRWN JONBRWN force-pushed the improve-py-exception-handling branch 2 times, most recently from c553e0d to d25e467 Compare May 11, 2018 21:16
@JONBRWN
Copy link
Contributor Author

JONBRWN commented May 11, 2018

@aturley print_errors() added and perf tested with no difference in performance against current master

@JONBRWN
Copy link
Contributor Author

JONBRWN commented May 14, 2018

@aturley I've also added a null check in the PyTCPEncoder to address the segfault created by #2182

@aturley
Copy link
Contributor

aturley commented May 14, 2018

@JONBRWN looks good to me.

@JONBRWN
Copy link
Contributor Author

JONBRWN commented May 14, 2018

thanks @aturley, I'll squash and merge

Adds null checks around the returned object of user provided functions
and fails if the returned object is NULL. This stops us from
segfaulting and provides an immediate error as opposed to the error
propagating up further down the pipeline when we attempt to use the
NULL object.

Add null check for encoded string in Machida's TCPSinkEncoder

Verifies that the string we're outputting is infact a string so
we do not segfault if the user provides an encoder function that
does not return a string.
@JONBRWN JONBRWN force-pushed the improve-py-exception-handling branch from c73b586 to c90a9bb Compare May 14, 2018 14:40
@JONBRWN JONBRWN merged commit 4085cd5 into master May 14, 2018
@JONBRWN JONBRWN deleted the improve-py-exception-handling branch May 31, 2018 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants