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

meaningful error messages #1419

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

XzzX
Copy link
Contributor

@XzzX XzzX commented May 2, 2024

Why do you replace the real error message with a custom message that does not know what happened?

f"{msg} Trying again in {delay}s. Tried {i + 1} times so far..."

Example:
FATAL: password authentication failed for user "seibll" is reported as WARNING - Database busy with too many connections. Trying again in 0.1s. Tried 1 times so far...

The real error message is only shown after all retries have failed.

Suggesting to print it immediately.

Print the real error message immediately.
@XzzX XzzX self-assigned this May 2, 2024
@XzzX XzzX requested a review from jan-janssen May 2, 2024 09:50
Comment on lines +148 to +151
f"{e}"
)
logger.warn(
f"Trying again in {delay}s. Tried {i + 1} times so far..."
Copy link
Member

Choose a reason for hiding this comment

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

What about

Suggested change
f"{e}"
)
logger.warn(
f"Trying again in {delay}s. Tried {i + 1} times so far..."
f"{msg} - Caused by {e.__class__.__name__}: {e}. Trying again in {delay}s. Tried {i + 1} times so far..."

to include the message?

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 think showing a general msg for a catch all errors will be misleading more often than it is right. Therefore, I think e should be printed. If you also want msg go ahead, I am ok with that.

Copy link
Member

Choose a reason for hiding this comment

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

The point could be to inform the user what is currently not working - sometimes the 'normal' error messages are not helpful and it might be good to say something about the funcion which did not work like ' Connecting to database failed: {e.class.name: {e}. Will retry ....'. I.e. I also would change the message to not be a error message :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The (not explicitly documented) intent for the function is to silence a specific, known exception temporarily. That's why msg is printed and not the exception itself, because the assumption is that the calling code expects this exception to happen sometimes. It sounds like in your case, we catch too broadly, so I think a better fix is to narrow errors where ever the error originates in your case.

@niklassiemer niklassiemer requested a review from pmrv May 2, 2024 10:30
@pmrv
Copy link
Contributor

pmrv commented May 2, 2024

It sounds more likely that error passed to the function is a too general error class. @XzzX which exception is thrown by sqlalchemy in your case? Can you post a full stack trace?

@jan-janssen jan-janssen marked this pull request as draft May 4, 2024 18:58
@XzzX
Copy link
Contributor Author

XzzX commented May 13, 2024

It sounds more likely that error passed to the function is a too general error class. @XzzX which exception is thrown by sqlalchemy in your case? Can you post a full stack trace?

The error class captured and thrown is sqlalchemy.exc.OperationalError. The error can be easily reproduced by changing the password in .pyiron to something invalid.

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