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

Eliminate distutils.util.strtobool #4477

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Eliminate distutils.util.strtobool #4477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c6b7f7
Eliminate `distutils.util.strtobool`.
bartoldeman Mar 6, 2024
9cd91f0
Check for EasyBuildError for strtobool replacement
bartoldeman Mar 6, 2024
bce95ec
Apply suggestions from code review
bartoldeman Mar 7, 2024
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
10 changes: 8 additions & 2 deletions easybuild/framework/easyconfig/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
* Caroline De Brouwer (Ghent University)
* Kenneth Hoste (Ghent University)
"""
from distutils.util import strtobool

from easybuild.base import fancylogger
from easybuild.framework.easyconfig.format.format import DEPENDENCY_PARAMETERS
Expand Down Expand Up @@ -280,7 +279,14 @@ def to_toolchain_dict(spec):
res = {'name': spec[0].strip(), 'version': spec[1].strip()}
# 3-element list
elif len(spec) == 3:
res = {'name': spec[0].strip(), 'version': spec[1].strip(), 'hidden': strtobool(spec[2].strip())}
hidden = spec[2].strip().lower()
if hidden in {'yes', 'true', 't', 'y', '1', 'on'}:
hidden = True
elif hidden in {'no', 'false', 'f', 'n', '0', 'off'}:
hidden = False
else:
raise EasyBuildError("Invalid truth value %s", hidden)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd not use EasyBuildError here, we use it way to much IMO. Raising this will log an "Easybuild crashed" message which I'm not sure we usually want. See #4301
I don't see the purpose of this error type at all. Python has enough standard error types (like the previous ValueError) and we could just catch any error in the main function and log the "EasyBuild crashed" there with the stacktrace of the error.

However we use EasyBuildError below so this is at least consistent. Just wanted to bring this topic up that we shouldn't keep replacing specific errors with the generic EasyBuildError

Copy link
Member

Choose a reason for hiding this comment

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

You're right, we should improve our error reporting quite a bit.

My hope was to improve that while developing EasyBuild v5.0, but the focus is on different currently (and I don't want to keep postponing the release of EasyBuild v5.0 either, so this may need to be an iterative effort)

Copy link
Contributor

Choose a reason for hiding this comment

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

True. Point here was my last sentence: We shouldn't make it worse going forward and check for introducing "wrong" EasyBuildErrors in PRs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning was indeed consistency with the EasyBuildError below. In this case it catches a syntax error in the easyconfig, which I see as semantically different from a ValueError which (if not handled) I'd consider a bug in EasyBuild itself (this is just my perception, it may not be correct!).

Copy link
Contributor

Choose a reason for hiding this comment

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

From what EasyBuildError does it should be used when EasyBuild ran into a state where the build must be aborted (it currently logs "EasyBuild crashed")
In this case ValueError is correct: The function "Convert a comma-separated string or 2/3-element list of strings to a dictionary", i.e. a value to a toolchain dict. If it can't do that, the value is wrong. Same as int("abc") would fail as the value is not an integer.

Note that this function is not parsing an EasyConfig. It is used when parsing an EasyConfig. And if that EasyConfig isn't a valid EasyConfig due to wrong values then again this is a ValueError. But it might be used in other places. The function that parses the EasyConfig may catch this ValueError and handle it. One way of handling would be to throw an EasyBuildError from this ValueError. However the function could as well just ignore the EasyConfig and try another one. And especially as the EasyConfig might not be from EasyBuild but from a user I wouldn't consider it "a bug in EasyBuild itself"

Using exceptions/errors is actually very common in Python which has the philosophy "Ask for forgiveness not for permission". IIRC the dict.get method is implemented like this:

def get(self, key, default):
  try:
    return self[key]
  catch KeyError:
    return default

So using this errors and handling them on a higher level is actually good practice in Python (where exceptions are supposedly very cheap)

I hope that makes sense and helps making the error handling better/more consistent.

res = {'name': spec[0].strip(), 'version': spec[1].strip(), 'hidden': hidden}
else:
raise EasyBuildError("Can not convert list %s to toolchain dict. Expected 2 or 3 elements", spec)

Expand Down
6 changes: 3 additions & 3 deletions test/framework/type_checking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ def test_to_toolchain_dict(self):
self.assertErrorRegex(EasyBuildError, errstr, to_toolchain_dict, ['gcc', '4', 'False', '7'])

# invalid truth value
errstr = "invalid truth value .*"
self.assertErrorRegex(ValueError, errstr, to_toolchain_dict, "intel, 2015, foo")
self.assertErrorRegex(ValueError, errstr, to_toolchain_dict, ['gcc', '4', '7'])
errstr = "Invalid truth value .*"
self.assertErrorRegex(EasyBuildError, errstr, to_toolchain_dict, "intel, 2015, foo")
self.assertErrorRegex(EasyBuildError, errstr, to_toolchain_dict, ['gcc', '4', '7'])

# missing keys
self.assertErrorRegex(EasyBuildError, "Incorrect set of keys", to_toolchain_dict, {'name': 'intel'})
Expand Down
Loading