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

Feat: Allow disabling rich tracebacks via env var #1695

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

fg91
Copy link
Member

@fg91 fg91 commented Jun 15, 2023

TL;DR

In flytekit==1.6.0 rich tracebacks were introduced (#1582).

Before, exceptions were shown like this:
Screenshot 2023-06-15 at 15 37 04

After, they are shown like this:

Screenshot 2023-06-15 at 15 36 44

This happens as long as import flytekit is called somewhere in the imported code base, also when not directly working with flyte.


Style ultimately is a question of taste but several of our engineers have complained about this change:

When/Why did the stacktrace get so ugly colorful. It takes up way too much space and pycharm cannot create crosslinks to the files anymore for me 🙈 And long lines are truncated

I feel the same, does not help me

Can we configure this

I wanted to complain about this too 😄

The tracebacks contain way more (white space) characters, meaning I can paste a lot less of them into chat gpt

Because of this, in this PR I propose to introduce an environment variable which disables rich tracebacks. I chose a naming inspired by "FLYTE_SDK_LOGGING_LEVEL" or "FLYTE_SDK_LOGGING_FORMAT" in flytekit/loggers.py.

Would this change be be acceptable to you @cosmicBboy @kumare3 @eapolinario ?

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

NA

Tracking Issue

NA

Follow-up issue

NA

Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
@fg91 fg91 force-pushed the fg91/feat/allow-disabling-rich-tracebacks branch from 49c57de to 37b9039 Compare June 15, 2023 13:52
@kumare3
Copy link
Contributor

kumare3 commented Jun 15, 2023

I like the idea, just find the bar name too big. Want to start making smaller feature flags

@fg91
Copy link
Member Author

fg91 commented Jun 15, 2023

I like the idea, just find the bar name too big. Want to start making smaller feature flags

What do you think about FLYTE_RICH_TRC or FLYTE_RICH_TRACE instead of FLYTE_SDK_RICH_TRACEBACKS?

Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

I'm ok with more descriptive environment variables if their use is restricted (which is the case of this PR).

@eapolinario eapolinario merged commit 572a33f into master Jun 15, 2023
@fg91 fg91 deleted the fg91/feat/allow-disabling-rich-tracebacks branch June 16, 2023 07:38
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.

3 participants