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

Add a debug mode to print stack trace for exceptions caught by top-level handler #1308

Closed
corneliusroemer opened this issue Sep 6, 2023 · 6 comments · Fixed by #1577
Closed
Assignees
Labels
enhancement New feature or request

Comments

@corneliusroemer
Copy link
Member

I got this error from refine that I struggled to debug:

$ augur refine --tree builds/tree_raw.nwk --alignment test.fasta  --output-tree builds/tree.nwk
...
ERROR: SequenceData: loading alignment failed... builds/masked.fasta

ERROR from TreeTime: This error is most likely due to a problem with your input data.
Please check your input data and try again. If you continue to have problems, please open a new issue including
the original command and the error above:  <https://github.com/nextstrain/augur/issues/new/choose> 
I didn't know why the alignment couldn't be loaded.

Turns out, my sequences weren't aligned, they had different lengths. I'll try to improve treetime's error message, see neherlab/treetime#258.

But while debugging and trying to find out what went wrong, I got quite frustrated by how hard it was to get a stacktrace of how the error originated. (Maybe @tsibley has some clues on how one can get a stacktrace here in this case without having to change any code, maybe some ENV variable trick?)

I think one problem is that we just blanket wrap all treetime errors and discard the stacktrace. Maybe we print the stack trace behind a verbosity flag here, i.e. add traceback.print_exc(file=sys.stderr) but only when -v is passed.

augur/augur/__init__.py

Lines 76 to 92 in 854dc7d

except TreeTimeUnknownError as e:
print_err(dedent("""\
ERROR from TreeTime: An error occurred in TreeTime (see above). This may be due to an issue with TreeTime or Augur.
Please report you are calling TreeTime via Augur.
"""))
sys.exit(2)
except TreeTimeError as e:
print_err(f"ERROR: {e}")
print_err("\n")
print_err(dedent("""\
ERROR from TreeTime: This error is most likely due to a problem with your input data.
Please check your input data and try again. If you continue to have problems, please open a new issue including
the original command and the error above: <https://github.com/nextstrain/augur/issues/new/choose>
"""))
except Exception:
traceback.print_exc(file=sys.stderr)
print_err("\n")

@corneliusroemer corneliusroemer added the enhancement New feature or request label Sep 6, 2023
@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Sep 6, 2023

One nice way is to use exception chaining.

try:
  some_code()
except ConcreteException as e:
  raise FinalError(f"There was a concrete error and here is some recommendations: ...") from e

(notice raise ... from ...)

I think this is the recommended way to express causality of errors in Python nowadays. The default handler then prints all errors in the chain, with stacks, so there's no need for handcrafted solutions.

Perhaps the default handler printout not as nice, but then the custom handler can also be augmented with understanding chains.

I am not sure when it was introduced though and so what's the minimum Python version requirement.

@tsibley
Copy link
Member

tsibley commented Sep 6, 2023

Yes, exceptions should be chained (except in rare cases), and the default handler should know how to emit the full chain with all the gory details when a debug or verbosity flag is set. This is what Nextstrain CLI does, for example.

In a case where you couldn't (or didn't want to) just modify the source, you could use the debugger.

@victorlin
Copy link
Member

victorlin commented Sep 6, 2023

It sounds like the missing piece here is a debug/verbosity flag. Right now the stack trace is only printed on unhandled exceptions because we definitely want that in an issue report. With errors such as AugurError which are raised internally, the message is clear and doesn't need a stack trace to go with it, at least without a --debug flag.

With TreeTimeError it seems like both unhandled exceptions and expected errors can be represented. The default behavior can be no stack trace, which is printed with --debug. The "report an issue" text can be modified to something like:

please open a new issue including the original command and the full error when run with --debug.

@tsibley
Copy link
Member

tsibley commented Sep 6, 2023

@victorlin unfinished comment?

I'd recommend the debug flag live in the environment rather than a command-line option, as it makes it much easier to set in a wide variety of contexts (e.g. places you can't easily control the command-line invocation) and doesn't have issues of not applying before option parsing.

@victorlin
Copy link
Member

@tsibley oops, fixed!

That sounds reasonable. I'd think to basically mirror what you did in nextstrain/cli#208.

@victorlin victorlin changed the title Unhelpful error message when refine input alignment is incorrect due to TreeTimeError catchall Add a debug mode to print stack trace for exceptions caught by top-level handler Sep 6, 2023
@victorlin victorlin self-assigned this Sep 6, 2023
@corneliusroemer
Copy link
Member Author

@victorlin There are two types of tree time error: TreeTimeError and TreeTimeUnknownError - we might want to show stack trace in one by default, depends on the semantics. Exposing stack trace with env variable sounds good, would be great to report this in the error message (for more info, rerun with AUGURDEBUG=true augur refine ...) as you've suggested

@tsibley tsibley mentioned this issue Aug 2, 2024
7 tasks
@tsibley tsibley assigned tsibley and unassigned victorlin Aug 14, 2024
tsibley added a commit that referenced this issue Aug 14, 2024
At the moment, this only enables printing of stack traces and the full
exception chain for handled (i.e. anticipated) errors, which otherwise
were not printed.  In the future, this mode can also control the output
of verbose debugging/troubleshooting logging for more commands.

Commit contents and message based on similar commits I made to
Nextstrain CLI.¹

Resolves <#1308>.

¹ <nextstrain/cli@229f2e8>
  <nextstrain/cli@b6d9290>
  <nextstrain/cli#208>
@tsibley tsibley linked a pull request Aug 14, 2024 that will close this issue
7 tasks
tsibley added a commit that referenced this issue Aug 14, 2024
At the moment, this only enables printing of stack traces and the full
exception chain for handled (i.e. anticipated) errors, which otherwise
were not printed.  In the future, this mode can also control the output
of verbose debugging/troubleshooting logging for more commands.

Commit contents and message based on similar commits I made to
Nextstrain CLI.¹

Resolves <#1308>.

¹ <nextstrain/cli@229f2e8>
  <nextstrain/cli@b6d9290>
  <nextstrain/cli#208>
tsibley added a commit that referenced this issue Aug 14, 2024
At the moment, this only enables printing of stack traces and the full
exception chain for handled (i.e. anticipated) errors, which otherwise
were not printed.  In the future, this mode can also control the output
of verbose debugging/troubleshooting logging for more commands.

Commit contents and message based on similar commits I made to
Nextstrain CLI.¹

Resolves <#1308>.

¹ <nextstrain/cli@229f2e8>
  <nextstrain/cli@b6d9290>
  <nextstrain/cli#208>
@tsibley tsibley removed a link to a pull request Aug 14, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants