XMLWordPrintable

#### Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
0.5
• Sprint:
DRP X16-2
• Team:
Data Release Production

#### Description

Within a config override file being executed via Config.load or Config.loadFromStream, using a variable that hasn't been defined results in a NameError exception, but this is silently suppressed and the user has no idea the following overrides have not been executed.

#### Activity

Hide
Paul Price added a comment -

A real easy review, please Russell Owen:

 pprice@tiger-sumire:/tigress/pprice/dm-5124/pex_config (tickets/DM-5729=) $git sub-patch commit c8eb449e2a30d86a236151899ea0463239fb826d Author: Paul Price  Date: Wed Apr 6 12:32:02 2016 -0400    allow NameError in loading to propagate    NameError was being unintentionally suppressed (we want to suppress  it to catch the root->config change, but there are other cases where  it needs to propagate or the user will be blissfully ignorant of  errors). Added a test that demonstrates the problem.   diff --git a/python/lsst/pex/config/config.py b/python/lsst/pex/config/config.py index 8960825..9add5d9 100644 --- a/python/lsst/pex/config/config.py +++ b/python/lsst/pex/config/config.py @@ -562,6 +562,8 @@ class Config(object):  " appears to use 'root' instead of 'config'; trying with 'root'")  local = {"root": self}  exec stream in {}, local + else: + raise    self._imports.update(importer.getModules())   diff --git a/tests/config.py b/tests/config.py index 5182e3b..99661aa 100755 --- a/tests/config.py +++ b/tests/config.py @@ -391,6 +391,12 @@ except ImportError:  self.assert_("Inequality in r['AAA']" in output)  self.assert_("Inequality in r['BBB']" not in output)   + def testLoadError(self): + """Check that loading allows errors in the file being loaded to propagate + """ + self.assertRaises(SyntaxError, self.simple.loadFromStream, "bork bork bork") + self.assertRaises(NameError, self.simple.loadFromStream, "config.f = bork") +  def suite():  utilsTests.init()  suites = []  Show Paul Price added a comment - A real easy review, please Russell Owen : pprice@tiger-sumire:/tigress/pprice/dm-5124/pex_config (tickets/DM-5729=)$ git sub-patch commit c8eb449e2a30d86a236151899ea0463239fb826d Author: Paul Price <price@astro.princeton.edu> Date: Wed Apr 6 12:32:02 2016 -0400   allow NameError in loading to propagate NameError was being unintentionally suppressed (we want to suppress it to catch the root->config change, but there are other cases where it needs to propagate or the user will be blissfully ignorant of errors). Added a test that demonstrates the problem.   diff --git a/python/lsst/pex/config/config.py b/python/lsst/pex/config/config.py index 8960825..9add5d9 100644 --- a/python/lsst/pex/config/config.py +++ b/python/lsst/pex/config/config.py @@ -562,6 +562,8 @@ class Config(object): " appears to use 'root' instead of 'config'; trying with 'root'") local = {"root": self} exec stream in {}, local + else: + raise self._imports.update(importer.getModules()) diff --git a/tests/config.py b/tests/config.py index 5182e3b..99661aa 100755 --- a/tests/config.py +++ b/tests/config.py @@ -391,6 +391,12 @@ except ImportError: self.assert_("Inequality in r['AAA']" in output) self.assert_("Inequality in r['BBB']" not in output) + def testLoadError(self): + """Check that loading allows errors in the file being loaded to propagate + """ + self.assertRaises(SyntaxError, self.simple.loadFromStream, "bork bork bork") + self.assertRaises(NameError, self.simple.loadFromStream, "config.f = bork") + def suite(): utilsTests.init() suites = []
Hide
Paul Price added a comment -

I'm running Jenkins to see if any bad config changes have crept in that will now fail.

Show
Paul Price added a comment - I'm running Jenkins to see if any bad config changes have crept in that will now fail.
Hide
Paul Price added a comment -

Jenkins run passed. Of course, we've got no idea how many config override were silently failing before and are now producing different configs, but it's better than it having failed!

Show
Paul Price added a comment - Jenkins run passed. Of course, we've got no idea how many config override were silently failing before and are now producing different configs, but it's better than it having failed!
Hide
Russell Owen added a comment -

Thank you for catching and fixing this problem. Your fix will certainly work and it is simple. However, please consider removing the "root" support, instead. It was only ever intended to be temporary and removing it simplifies the code.

Show
Russell Owen added a comment - Thank you for catching and fixing this problem. Your fix will certainly work and it is simple. However, please consider removing the "root" support, instead. It was only ever intended to be temporary and removing it simplifies the code.
Hide
Paul Price added a comment -

If you don't mind, I will leave the backward-compatibility support for root in place for now. Removing it would require much wider consultation, and I don't think it's a good idea right now because we're about to move HSC people to the LSST stack and we will want to present them a nice warning rather than a perplexing error message. But please don't let me stop you from RFC-ing your proposal and adding a ticket for the work to be scheduled for the future.

Show
Paul Price added a comment - If you don't mind, I will leave the backward-compatibility support for root in place for now. Removing it would require much wider consultation, and I don't think it's a good idea right now because we're about to move HSC people to the LSST stack and we will want to present them a nice warning rather than a perplexing error message. But please don't let me stop you from RFC-ing your proposal and adding a ticket for the work to be scheduled for the future.
Hide
Paul Price added a comment -

Merged to master after Russell mentioned on HipChat that he agrees.

Show
Paul Price added a comment - Merged to master after Russell mentioned on HipChat that he agrees.
Hide
John Swinbank added a comment -

Mentioned in release notes.

Show
John Swinbank added a comment - Mentioned in release notes.

#### People

Assignee:
Paul Price
Reporter:
Paul Price
Reviewers:
Russell Owen
Watchers:
John Swinbank, Paul Price, Russell Owen