Skip to content

DM-53382: Add more descriptive error messages #533

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

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

DM-53382: Add more descriptive error messages #533

Show file tree
Hide file tree
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
33 changes: 29 additions & 4 deletions python/lsst/pipe/base/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,34 @@ def applyConfigOverrides(
if subConfig.dataId is None:
if subConfig.file:
for configFile in subConfig.file:
overrides.addFileOverride(os.path.expandvars(configFile))
configPath = os.path.expandvars(configFile)
try:
overrides.addFileOverride(configPath)
except Exception as e:
e.add_note(
f"Applying config override to task '{label}' from file '{configPath}'"
Copy link
Member

@timj timj Nov 24, 2025

Choose a reason for hiding this comment

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

The error message from addFileOverride doesn't include configPath in it?

Copy link
Author

@fred3m fred3m Nov 24, 2025

Choose a reason for hiding this comment

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

I'm not sure about this error specifically. One of the overrides (actually configOverride) was triggered by analysis_tools and took help from Jim to debug because there was no actual reference to analysis_tools anywhere in the traceback. While we added better error messages to that we decided to add them everywhere just in case.

)
raise
if subConfig.python is not None:
overrides.addPythonOverride(subConfig.python)
try:
overrides.addPythonOverride(subConfig.python)
except Exception as e:
e.add_note(
f"Applying config override to task '{label}' from "
"python: '{subConfig.python}'"
)
raise
for key, value in subConfig.rest.items():
overrides.addValueOverride(key, value)
overrides.applyTo(self)
try:
overrides.addValueOverride(key, value)
except Exception as e:
e.add_note(
f"Applying config override to task '{label}' for "
"key '{key}' with value '{value}'"
Copy link
Member

@timj timj Nov 24, 2025

Choose a reason for hiding this comment

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

f-string? Maybe addValueOverride should itself report key and value in the error message? Or is that API not raising anything itself but giving a KeyError?

)
raise
try:
overrides.applyTo(self)
except Exception as e:
e.add_note(f"Applying config overrides to task '{label}'")
raise
109 changes: 63 additions & 46 deletions python/lsst/pipe/base/configOverrides.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,50 +332,59 @@ def applyTo(self, config):
for otype, override in self._overrides:
if otype is OverrideTypes.File:
with override.open("r") as buffer:
config.loadFromStream(buffer, filename=override.ospath, extraLocals=extraLocals)
try:
config.loadFromStream(buffer, filename=override.ospath, extraLocals=extraLocals)
except Exception as e:
e.add_note(f"Applying config override from file '{override.ospath}'")
Copy link
Member

@timj timj Nov 24, 2025

Choose a reason for hiding this comment

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

loadFromStream knows the filename but doesn't report it?

raise
elif otype is OverrideTypes.Value:
field, value = override
if isinstance(value, str):
value = self._parser(value, configParser)
# checking for dicts and lists here is needed because {} and []
# are valid yaml syntax so they get converted before hitting
# this method, so we must parse the elements.
#
# The same override would remain a string if specified on the
# command line, and is handled above.
if isinstance(value, dict):
new = {}
for k, v in value.items():
if isinstance(v, str):
new[k] = self._parser(v, configParser)
else:
new[k] = v
value = new
elif isinstance(value, list):
new = []
for v in value:
if isinstance(v, str):
new.append(self._parser(v, configParser))
else:
new.append(v)
value = new
# The field might be a string corresponding to a attribute
# hierarchy, attempt to split off the last field which
# will then be set.
parent, *child = field.rsplit(".", maxsplit=1)
if child:
# This branch means there was a hierarchy, get the
# field to set, and look up the sub config for which
# it is to be set
finalField = child[0]
tmpConfig = attrgetter(parent)(config)
else:
# There is no hierarchy, the config is the base config
# and the field is exactly what was passed in
finalField = parent
tmpConfig = config
# set the specified config
setattr(tmpConfig, finalField, value)
try:
field, value = override
if isinstance(value, str):
value = self._parser(value, configParser)
# checking for dicts and lists here is needed because
# {} and [] are valid yaml syntax so they get converted
# before hitting this method,
# so we must parse the elements.
#
# The same override would remain a string if
# specified on the command line, and is handled above.
if isinstance(value, dict):
new = {}
for k, v in value.items():
if isinstance(v, str):
new[k] = self._parser(v, configParser)
else:
new[k] = v
value = new
elif isinstance(value, list):
new = []
for v in value:
if isinstance(v, str):
new.append(self._parser(v, configParser))
else:
new.append(v)
value = new
# The field might be a string corresponding to a attribute
# hierarchy, attempt to split off the last field which
# will then be set.
parent, *child = field.rsplit(".", maxsplit=1)
if child:
# This branch means there was a hierarchy, get the
# field to set, and look up the sub config for which
# it is to be set
finalField = child[0]
tmpConfig = attrgetter(parent)(config)
else:
# There is no hierarchy, the config is the base config
# and the field is exactly what was passed in
finalField = parent
tmpConfig = config
# set the specified config
setattr(tmpConfig, finalField, value)
except Exception as e:
e.add_note(f"Applying config override for field '{field}' with value '{value}'")
raise

elif otype is OverrideTypes.Python:
# exec python string with the context of all vars known. This
Expand All @@ -386,7 +395,15 @@ def applyTo(self, config):
# in a python block will be put into this scope. This means
# other config setting branches can make use of these
# variables.
exec(override, None, localVars)
try:
exec(override, None, localVars)
except Exception as e:
e.add_note(f"Applying config override with python code '{override}'")
raise
elif otype is OverrideTypes.Instrument:
instrument, name = override
instrument.applyConfigOverrides(name, config)
try:
instrument, name = override
Copy link
Member

@timj timj Nov 24, 2025

Choose a reason for hiding this comment

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

Should this be in the try block?

instrument.applyConfigOverrides(name, config)
except Exception as e:
e.add_note(f"Applying config override with instrument '{instrument}' and name '{name}'")
Copy link
Member

@timj timj Nov 24, 2025

Choose a reason for hiding this comment

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

All the information in this note seems to be known to applyConfigOverrides.

raise
Loading