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

meaningful error messages #1419

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

meaningful error messages #1419

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion pyiron_base/utils/error.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@ def retry(
return func()
except error as e:
logger.warn(
f"{msg} Trying again in {delay}s. Tried {i + 1} times so far..."
f"{e}"
)
logger.warn(
f"Trying again in {delay}s. Tried {i + 1} times so far..."
Comment on lines +148 to +151
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.

)
time.sleep(delay)
delay *= delay_factor
Expand Down
Loading