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 tracebacks to errors in CLI runs #4075

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Aug 7, 2024

Description

Fixes #4073

Development notes

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Laura Couto <laurarccouto@gmail.com>
@@ -210,6 +210,8 @@ def main(
sys.exit(exc.code)
except Exception as error:
logger.error(f"An error has occurred: {error}")
Copy link
Member

Choose a reason for hiding this comment

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

This message was introduced in 0.19.7; however, if we want to spit out the full trace, why is this necessary? Why can't you just doing something like

            self._cli_hook_manager.hook.after_command_run(
                project_metadata=self._metadata, command_args=args, exit_code=1
            )
            hook_called = True
            raise

?

This would be closer to the previous behavior.

In case you're unsure, your finally block will still be called:

>>> try:
...    print("do or do not")
...    raise Exception("oh no")
... except Exception as exc:
...    print("there is no try")
...    raise
... finally:
...    print("rip")
... 
do or do not
there is no try
rip
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
Exception: oh no
>>> 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does output the stack trace but IMO is not as aesthetically nice. This is what it looks like using raise:

image

This is what it looks like using traceback.format_exc

image

The content of the trace is the same, but I think the second is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option would be using the rich console traceback, but that would require importing rich.

image

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be using the rich console traceback, but that would require importing rich.

I'm not sure if this is what you mean, but you get the Rich traceback as long as Rich is installed—you don't need to change the code.

In fact, one downside of catching and printing the formatted exception yourself is that you don't get the Rich traceback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without manually logging formatted exception and just using raise, it looks like the first image.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the solution @deepyaman proposes. I've compared that with how it looks in 0.19.6 and it's exactly the same. I wouldn't worry too much about the formatting.

Side note: I don't know what's going on with the rich formatting. I used to have logs formatted like in the ticket description with the rectangle around them (#4073), but now I'm not seeing that.. If it's an issue in general I'd suggest tackling that separately.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if #3682 has changed stuff.. I noticed that even if I use the default rich handler it doesn't pick it up in the catalog messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also noticed that our little hack of uninstalling rich and downgrading cookiecutter does not work anymore. I can take a look at that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@merelcht #3682 shouldn't change this as it targets at two very specific log message with rich markup.

@lrcouto lrcouto marked this pull request as ready for review August 7, 2024 19:30
@lrcouto lrcouto requested a review from merelcht as a code owner August 7, 2024 19:30
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Copy link
Member

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating!

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM!

@lrcouto lrcouto merged commit 62431bb into main Aug 8, 2024
34 checks passed
@lrcouto lrcouto deleted the display-stack-trace-for-run-error branch August 8, 2024 16:24
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.

Reenable stack trace on error, removed in 0.19.7
4 participants