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

checksums.json support clutters log with false crash messages #4301

Closed
Flamefire opened this issue Jul 24, 2023 · 7 comments · Fixed by #4261
Closed

checksums.json support clutters log with false crash messages #4301

Flamefire opened this issue Jul 24, 2023 · 7 comments · Fixed by #4261
Milestone

Comments

@Flamefire
Copy link
Contributor

Flamefire commented Jul 24, 2023

Since #3749 introduced support for checksums.json the log will now contain the following highly misleading error on almost every build:

ERROR EasyBuild crashed with an error (at easybuild/base/exceptions.py:126 in init): Couldn't find file checksums.json anywhere, and downloading it is disabled... Paths attempted (in order): ...

I recently got this from another admin who is debugging a failure and thought this is the first error, which is understandable given the message "EasyBuild crashed with an error" which is clearly wrong at this point: The failure is very much expected.
So EasyBuildError should not log anything on construction because it might be caught invalidating the logged message.

I found sys.excepthook which is called for unhandled exceptions which is likely a better place for logging the exception then logging every (such) exception on creation even if it is caught and handled as part of an expected code path.

Alternative: Let obtain_file return None in case the file was not found, maybe based on a parameter expect_success for backwards compatibility.

CC @mboisson

@mboisson
Copy link
Contributor

🤔 Could you be more specific on which recipe crashes and what file it is attempting to download ? I doubt that the PR made almost every build fail (otherwise it would not have been merged)

@Flamefire
Copy link
Contributor Author

Oh sorry for the confusion, the build is not crashing but the log contains the message "EasyBuild crashed with an error" because almost no ECs have a matching checksums.json but obtain_file doesn't (yet) care and raises an EasyBuildError which on construction logs that message.

@mboisson
Copy link
Contributor

🤔 if that exception does not make it crash (because it is then handled by the caller), why is it logged at all ? The obtain_file method called with no_download = True should raise an exception if the file is not found without downloading it.

@Flamefire
Copy link
Contributor Author

thinking if that exception does not make it crash (because it is then handled by the caller), why is it logged at all ?

Because EasyBuildError logs that message on construction. I.e. at that point it is not known if it is caught. This turns up in other situations too

@boegel boegel added this to the 4.x milestone Aug 2, 2023
@boegel
Copy link
Member

boegel commented Aug 2, 2023

@Flamefire The problem you're reporting is a very valid one, there should be no scary-looking " EasyBuild crashed with an error" messages in the middle of the log.

It's being fixed in #4261, by letting obtain_file only log a warning.

@Flamefire
Copy link
Contributor Author

I see, so #4261 implements the alternative approach I suggested above.

However there are a dozen other instances where we catch this error (still logging the scary message) as identified by grep -rF "except EasyBuildError".
Some are even in easyblocks, e.g. openssl.py catches it and then logs an info message.

So maybe we want to change that log-on-construction logic anyway.

@boegel boegel modified the milestones: 4.x, next release (4.8.1?) Sep 9, 2023
@boegel
Copy link
Member

boegel commented Sep 9, 2023

@Flamefire This was closed because the exact issue being reported here was fixed by #4261

Let's open a dedicated issue for coming up with a good way to prevent errors that should just be warnings from appearing in the log file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants