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.

Sign up for GitHub

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

Jump to bottom

Add tracebacks to errors in CLI runs #4075

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Add tracebacks to errors in CLI runs #4075

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
26afb1b
Add tracebacks to errors in CLI runs
lrcouto Aug 7, 2024
cd5aa20
Raise exception instead of logging it
lrcouto Aug 8, 2024
4b2aac0
Update tests
lrcouto Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions kedro/framework/cli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

logger.error(f"{traceback.format_exc()}")

self._cli_hook_manager.hook.after_command_run(
project_metadata=self._metadata, command_args=args, exit_code=1
)
Expand Down