Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5729

Config.loadFromStream suppresses NameError

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pex_config
    • 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.

        Attachments

          Issue Links

            Activity

            Hide
            price 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 = []
            

            Show
            price 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
            price Paul Price added a comment -

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

            Show
            price Paul Price added a comment - I'm running Jenkins to see if any bad config changes have crept in that will now fail.
            Hide
            price 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
            price 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
            rowen 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
            rowen 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
            price 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
            price 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
            price Paul Price added a comment -

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

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

            Mentioned in release notes.

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

              People

              Assignee:
              price Paul Price
              Reporter:
              price Paul Price
              Reviewers:
              Russell Owen
              Watchers:
              John Swinbank, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.