Improve Error Handling in TreeTime #206

anna-parker · 2022-09-02T14:00:59Z

Related Issue and PR:

nextstrain/augur#1032
nextstrain/augur#1033

Building on @victorlin's suggestions in : 44e7c56...2632d96 I have made a couple minor changes to TreeTime's error handling process:

more consistent labeling of error classes: rename errors due to incorrect input data or incorrect use of treetime as one of the three predefined TreeTime error classes: MissingDataError, UnknownMethodError and NotReadyError
add new TreeTimeError class for errors that might be due to input data not matching with assumptions or unknown behavior (potential bug): TreeTimeOtherError.
call treetime.run in a wrapper to catch all errors with notification on how to report issues. i.e. ERROR in TreeTime.run: An error has occurred which is not properly handled in TreeTime. If this error persists, please let us know " "by filing a new issue including the original command and the error above at: https://github.com/neherlab/treetime/issues
when run from another application, such as augur return all errors as TreeTimeErrors but with distinction between input/usage errors and potential treetime bugs (these are output as TreeTimeOtherErrors). On standard run print traceback for unknown errors / potential bugs, print warning and then exit treetime (avoid double printing of traceback or traceback after final warning).

Example for unknown ValueError:

Example for known MissingDataError:

I used the same example as in the PR above to create an error. I made two test cases, one where the error is called a ValueError which should be handled as a TreeTimeOtherError and another where the error is called a MissingDataError.

Testing

install augur and treetime in local environment
cause an Exception in treetime (either call this a ValueError or a TreeTimeError), I added a line in _exp_lt

        """
        Parameters
        ----------

         t : float
            time to propagate

        Returns
        --------

         exp_lt : numpy.array
            Array of values exp(lambda(i) * t),
            where (i) - alphabet index (the eigenvalue number).
        """
        log_val = self.mu * t * self.eigenvals
        log_val = np.ones(len(log_val))*750 ##cause an exception
        if any(i > 10 for i in log_val):
            raise ValueError("Error in computing exp(Q * t): Q has positive eigenvalues or the branch length t \n"
                    "is too large. This is most likely caused by incorrect input data.")

        return np.exp(log_val)

install modified treetime in local environment (pip install .)
call treetime.run() in a test script where it is not in a try - catch statement, for example: run python test/treetime_algorithms.py

corneliusroemer

Still a bit confused what the error hierarchy is supposed to be :)

treetime/__init__.py

corneliusroemer · 2022-09-02T14:30:10Z

treetime/__init__.py

@@ -9,13 +12,17 @@ class MissingDataError(TreeTimeError):
    pass

 class UnknownMethodError(TreeTimeError):
-    """MissingDataError class raised when tree or alignment are missing"""
+    """MissingDataError class raised when an unknown method is called"""


Are methods added dynamically? In that case it may be helpful to explain this.

"alignment are missing" sounds like this error would be raised if you haven't loaded an alignment but call a method that gets added only during alignment

treetime/__init__.py

corneliusroemer · 2022-09-02T14:33:18Z

treetime/__init__.py

@@ -9,13 +12,17 @@ class MissingDataError(TreeTimeError):
    pass


Or parameters are missing? If I understand usage below correctly?

corneliusroemer · 2022-09-02T14:33:59Z

treetime/clock_tree.py

@@ -77,7 +77,7 @@ def __init__(self, *args, dates=None, debug=False, real_dates=True, precision_ff
        """
        super(ClockTree, self).__init__(*args, **kwargs)
        if dates is None:
-            raise ValueError("ClockTree requires date constraints!")
+            raise MissingDataError("ClockTree requires date constraints!")


If this is how you want to use MissingDataError one should probably change the description of the MissingDataError to encompass missing parameters

corneliusroemer · 2022-09-02T14:35:03Z

treetime/clock_tree.py

@@ -203,7 +203,7 @@ def _set_precision_fft(self, precision_fft, precision_branch='auto'):
                        " fft_grid_points=%.3e"%(precision_fft), 2)
                self.fft_grid_size = precision_fft
            elif precision_fft!='auto':
-                raise ValueError(f"ClockTree: precision_fft needs to be either 'auto' or an integer, got '{precision_fft}'.")
+                raise UnknownMethodError(f"ClockTree: precision_fft needs to be either 'auto' or an integer, got '{precision_fft}'.")


This should be a new type of error? Something like: Wrong parameter passed - not unknown method. This has nothing to do with a method?

corneliusroemer · 2022-09-02T14:36:27Z

treetime/gtr.py

-            raise ValueError("Error in computing exp(Q * t): Q has positive eigenvalues or the branch length t \n"
-                    "is too large. This is most likely caused by incorrect input data. If this error persists \n"
-                    "please let us know by filing an issue at: https://github.com/neherlab/treetime/issues")
+            raise ValueError("Error in computing exp(Q * t): Q has positive eigenvalues or the branch length t is too large. "


Isn't this the type of error that should be wrapped by Augur? If so, it should maybe be "InputDataError"? Or is this what you use ValueError for now? Though I'd think ValueError would need to be TreetimeError - or is this supposed to be a "TreetimeOtherError"?

Co-authored-by: Cornelius Roemer <[email protected]>

anna-parker · 2022-09-07T09:23:54Z

treetime/treetime.py

@@ -51,7 +51,29 @@ def __init__(self, *args,**kwargs):
        super(TreeTime, self).__init__(*args, **kwargs)


-    def run(self, root=None, infer_gtr=True, relaxed_clock=None, n_iqd = None,
+    def run(self, augur=False, **kwargs):


@victorlin Hi, I modified how the run function is called to make sure that the Errors get printed at the correct location when running in TreeTime and in augur - however @corneliusroemer mentioned that using the argument augur=true is not the best coding practice - is there a better way to do this? Perhaps we could use @rneher raise_uncaught_exception = true instead as an argument to make it less specific and then add some commentary? Thanks for any input :-)

For example, for "unknown" errors (which will get thrown as TreeTimeOtherErrors ), the error traceback in augur (PR: nextstrain/augur#1033) is as follows, where a warning message with information how to report issues comes after the full traceback:

For "known" TreeTimeErrors they will get thrown as follows (with information after the traceback):

I agree, naming this parameter as augur seems too specific – it's possible that other Python scripts would want to use this too. raise_uncaught_exceptions (plural) is nice self-describing name.

victorlin

Post-merge requested changes

victorlin · 2022-09-07T23:30:29Z

treetime/__init__.py

    pass

 class NotReadyError(TreeTimeError):
    """NotReadyError class raised when results are requested before inference"""
    pass

+class TreeTimeOtherError(Exception):


"Other" seems a bit ambiguous... "other" than what?

Suggested change

class TreeTimeOtherError(Exception):

class TreeTimeUnknownError(Exception):

I like unknown

victorlin · 2022-09-07T23:36:00Z

treetime/treetime.py

@@ -51,7 +51,29 @@ def __init__(self, *args,**kwargs):
        super(TreeTime, self).__init__(*args, **kwargs)


-    def run(self, root=None, infer_gtr=True, relaxed_clock=None, n_iqd = None,
+    def run(self, augur=False, **kwargs):


I agree, naming this parameter as augur seems too specific – it's possible that other Python scripts would want to use this too. raise_uncaught_exceptions (plural) is nice self-describing name.

victorlin · 2022-09-07T23:43:00Z

treetime/treetime.py

+            print("ERROR in TreeTime.run: An error has occurred which is not properly handled in TreeTime. If this error persists, please let us know "
+                    "by filing a new issue including the original command and the error above at: https://github.com/neherlab/treetime/issues \n", file=sys.stderr)


Minor grammar:

Suggested change

print("ERROR in TreeTime.run: An error has occurred which is not properly handled in TreeTime. If this error persists, please let us know "

"by filing a new issue including the original command and the error above at: https://github.com/neherlab/treetime/issues \n", file=sys.stderr)

print("ERROR in TreeTime.run: An error occurred which was not properly handled in TreeTime. If this error persists, please let us know "

"by filing a new issue including the original command and the error above at: https://github.com/neherlab/treetime/issues \n", file=sys.stderr)

victorlin · 2022-09-07T23:45:15Z

treetime/treetime.py

+        except TreeTimeError as err:
+            print(f"ERROR: {err} \n", file=sys.stderr)
+            if augur:
+                raise err
+            else:
+                sys.exit(2)


No need to print the error if it is being raised:

Suggested change

except TreeTimeError as err:

print(f"ERROR: {err} \n", file=sys.stderr)

if augur:

raise err

else:

sys.exit(2)

except TreeTimeError as err:

if augur:

raise err

else:

print(f"ERROR: {err} \n", file=sys.stderr)

sys.exit(2)

victorlin · 2022-09-07T23:56:14Z

treetime/treetime.py

+                sys.exit(2)
+        except BaseException as err:
+            import traceback
+            print(traceback.format_exc())


Traceback should be printed to stderr to as well:

Suggested change

print(traceback.format_exc())

print(traceback.format_exc(), file=sys.stderr)

anna-parker · 2022-09-08T06:29:54Z

@victorlin thanks so much for your suggestions - I have added them to another branch https://github.com/neherlab/treetime/tree/feat/victor-error-handling - will do a PR after checking they integrate with your suggestions in augur :-)

push traceback before error message

6b9043e

anna-parker requested review from rneher, victorlin and corneliusroemer September 2, 2022 14:00

add back return statement

5bc4ef4

corneliusroemer requested changes Sep 2, 2022

View reviewed changes

rneher and others added 2 commits September 7, 2022 09:48

Update treetime/__init__.py

769195c

Co-authored-by: Cornelius Roemer <[email protected]>

Update treetime/__init__.py

f797f5c

Co-authored-by: Cornelius Roemer <[email protected]>

rneher merged commit 0848fdf into master Sep 7, 2022

anna-parker commented Sep 7, 2022

View reviewed changes

victorlin reviewed Sep 8, 2022

View reviewed changes

victorlin mentioned this pull request Sep 8, 2022

Specify --verbosity for TreeTime, improve error handling nextstrain/augur#1033

Merged

1 task

anna-parker mentioned this pull request Sep 8, 2022

TreeTime Error Log - add Victor's suggestions #208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Error Handling in TreeTime #206

Improve Error Handling in TreeTime #206

anna-parker commented Sep 2, 2022

corneliusroemer left a comment

corneliusroemer Sep 2, 2022

corneliusroemer Sep 2, 2022

corneliusroemer Sep 2, 2022

corneliusroemer Sep 2, 2022

corneliusroemer Sep 2, 2022

anna-parker Sep 7, 2022 •

edited

Loading

victorlin Sep 7, 2022

victorlin left a comment

victorlin Sep 7, 2022

corneliusroemer Sep 8, 2022

victorlin Sep 7, 2022

victorlin Sep 7, 2022

victorlin Sep 7, 2022

victorlin Sep 7, 2022

anna-parker commented Sep 8, 2022

		@@ -9,13 +12,17 @@ class MissingDataError(TreeTimeError):
		pass

	class TreeTimeOtherError(Exception):
	class TreeTimeUnknownError(Exception):

		print("ERROR in TreeTime.run: An error has occurred which is not properly handled in TreeTime. If this error persists, please let us know "
		"by filing a new issue including the original command and the error above at: https://github.com/neherlab/treetime/issues \n", file=sys.stderr)

	print(traceback.format_exc())
	print(traceback.format_exc(), file=sys.stderr)

Improve Error Handling in TreeTime #206

Improve Error Handling in TreeTime #206

Conversation

anna-parker commented Sep 2, 2022

Related Issue and PR:

Testing

corneliusroemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anna-parker Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anna-parker commented Sep 8, 2022

anna-parker Sep 7, 2022 •

edited

Loading