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

fix(metahyper): Print useful information in logs for exception occuring #53

Closed

Conversation

eddiebergman
Copy link
Contributor

This is more of a bandaid fix such that the error/traceback shows up at least somewhere. For debugging pipelines and initial setup, there should be some option where the error just bubbles all the way up and crashes the program.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is indeed an essential feature. However, I'm a bit puzzled, shouldn't the traceback already be logged with the outer try-except statement, especially since we're using logger.exception? If I'm mistaken or if this behaves differently in certain scenarios, please correct me.

Additionally, I'm wondering if it might be more appropriate to catch the error from the 'except statement' of the initial try block as well. What are your thoughts on this?

@eddiebergman
Copy link
Contributor Author

I learned something new today, I did not know logger.exception does that by default! Good to know :)

In that case, I think the main feature that should be added is to give an option to raise immediatly on failure. This makes developing easier, i.e. no one will get their evaluation function right first time.

As an addendum, I also think that the error logic in the parsing of the result should always mention what it got as the result that it failed to parse. Simply saying something didn't work isn't very informative

I will close this PR as that seems like two things:

  • A feature for on_error: Literal["continue", "raise"] or something like that to help initial development. Maybe even make it default "raise" as that's what is most likely needed while initially trying things
  • Better error messages saying what it got as the result such that parsing it out failed.

@eddiebergman eddiebergman deleted the fix-include-err-and-traceback-in-logger-output branch January 31, 2024 17:16
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.

2 participants