Convert all errors into try/catch/except to not have to deal with `sys.excepthook`

[lwjohnst86]

Do you know if this actually impacts things when used e.g. via the command line? Like if we use check-datapackage internally in Flower, does check-datapackage's exception changes also change exceptions in Flower?

Currently yes, and that's why I did this more elaborate rewrite so so that they would play nicely with each other. But it doesn't have to work this way. There are alternatives to modifying sys.excepthook, such as using individual try/except statements where we expect our custom errors to be raised. The downside would be that it is less automatic/implicit and we would need try/except logic at every place where our custom error might occur (which aren't that many, maybe even just one for check-datapackage). One could argue that this would be cleaner and I see it more commonly elsewhere, but since we already had modified sys.excepthook earlier in check-datapackage I continued on that approach here. Maybe that was not the best choice though now that I reflect on it... It does get messy to modify a global state like this. I'm down to switch it around and use try/except instead (we already use it in flower to customize the error message so it wouldn't be that much additional logic there either I think).

I can explore that in a separate PR. We would still need to move the exception pretty printing to soil, but maybe we could actually get rid of most of the complicated traceback chaining. I'm not 100% it will work the same way for IPython, but it seems like it could become much cleaner if it does work well.

One thing of note though, modifying sys.excepthook is the only way to suppress tracebacks of all error messages. So if we change to try/except, the user would still get a giant error message if a regular python exception is raised (rather than one of our custom ones). This could be seen as a good thing as it is easier to troubleshoot unknown errors that we haven't accounted for, but maybe the reason for using sys.excepthook in the first place was a desire to control the tracebacks from all errors? How do you want the program to behave here, is it ok if we remove the traceback only for our custom errors that we are expecting to raise, or do you want it to be removed for all possible errors even those we don't raise explicitly ourselves?

Originally posted by @joelostblom in #309

[joelostblom]

Thanks for creating the issue! What do you think about the last part of my comment above:

...but maybe the reason for using sys.excepthook in the first place was a desire to control the tracebacks from all errors? How do you want the program to behave here, is it ok if we remove the traceback only for our custom errors that we are expecting to raise, or do you want it to be removed for all possible errors even those we don't raise explicitly ourselves?

[lwjohnst86]

Hmm, true. Ideally, the user would never see any tracebacks.

[joelostblom]

We could achieve that with a small modification to the lowest level hook in our current except hook chain. The issue is that anyone that imports check-datapackage in any context (ie, also developers and not users), will have all tracebacks (from any package) suppressed in that Python session since we are modifying the global excepthook.

I increasingly think that try/except is a cleaner solution. We can still achieve the goal of a user of the CLI not seeing (almost) any tracebacks by making sure we have custom errors without tracebacks for all cases where we think the user will encounter an error. We can keep adding to this list if we encounter situations we didn't account for initially, and it seems to me that this behavior leads to less unexpected situations. I would be keen to draft a PR for this to see if it gets as much cleaner as I think, as long as you are OK with the outcome of taking this path.

A third solution would be something like modifying the global excepthook only if the program is invoked from the cli. This would mean it behaves differently from the cli and the python interface though.

[lwjohnst86]

I like the third option, since that's where it is most relevant. It's unpleasant to get a full traceback for Python when using the CLI.

[martonvago]

I think cutting the traceback only in a CLI context is a great idea! I was a bit uneasy about doing it globally.

I wonder how this will play with Cyclopts, like where we can add this logic. If there is a good place to add code that only runs in a CLI context, then maybe we could get away with simply try-catching all exceptions there?

[joelostblom]

Ok, let's give that a try!

I looked at the cyclopts docs in hope of finding an already documented way of doing this, but I can only find that they recommend using rich which still includes long tracebacks. So I think we will use a wrapper function with a catch all for bare exceptions when running from the CLI and update the entry point accordingly.

If that works well, then maybe we don't even need the hook traceback suppression logic at all. Otherwise we can create a wrapper that setups the traceback suppression before launching app(). One positive thing with keeping the suppression hook around as a utility function would be to use it in the docs to not show the full traceback there. But we could of course catch errors individually there too.

[joelostblom]

A suggestion for how to solve this in seedcase-project/seedcase-flower#216, curious to hear if you see any gotchas with that approach!