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

Empty Mapper location templates are not detected

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: butler
    • Labels:
      None
    • Story Points:
      10
    • Sprint:
      DB_S17_01, DB_S17_2, DB_S17_4, DB_S17_5, DB_S17_7, DB_S17_8
    • Team:
      Data Access and Database

      Description

      When using a dataset definition in a mapping, check that the template is populated (IE that it exists and is not a null string or None), and if it fails this check raise an error message with information about the dataset & template value that needs to be defined.

        Attachments

          Issue Links

            Activity

            Hide
            npease Nate Pease added a comment - - edited

            Kian-Tat Lim recently you & I had a discussion where I proposed using Cerberus to validate policy dicts after loading them, but as I recall your take was that a full validation was overkill and a simpler check for required keys & values could be done when constructing the CameraMapper.

            I took a cut at implementing a checker that can test for required & allowed keys, and can check that the value evaluates to true for if(val), as so e.g. an empty string would get caught as an illegal value. It uses getter functions which would allow CameraMapper subclasses to define allowed keys, and a verifier function to allow CameraMapper subclasses to write their own verification code if needed. Please take a look, at u/npease/8432_in_mapping in obs_base, and let me know if you like this approach better, or if not let's talk about it some more. Thanks!

            Show
            npease Nate Pease added a comment - - edited Kian-Tat Lim recently you & I had a discussion where I proposed using Cerberus to validate policy dicts after loading them, but as I recall your take was that a full validation was overkill and a simpler check for required keys & values could be done when constructing the CameraMapper. I took a cut at implementing a checker that can test for required & allowed keys, and can check that the value evaluates to true for if(val) , as so e.g. an empty string would get caught as an illegal value. It uses getter functions which would allow CameraMapper subclasses to define allowed keys, and a verifier function to allow CameraMapper subclasses to write their own verification code if needed. Please take a look, at u/npease/8432_in_mapping in obs_base, and let me know if you like this approach better, or if not let's talk about it some more. Thanks!
            Hide
            npease Nate Pease added a comment -

            Kian-Tat Lim please also look at u/npease/8432_justtemplate, I only check that the
            policy['template'] != ''.
            It's certainly the simplest implementation if it seems like it does enough of what's needed.

            Note any of these solutions are producing "incomplete policy" errors in the obs_base unit tests (an example is pasted in below), because lots of template are set to '', and the expectation is that derived packages complete the package-specific portions of the policy.

            ======================================================================
            ERROR: testReuseOutputRoot (_main_.OutputRootTestCase)
            Set up an output repositoriy and verify its parent relationship to the input repository.
            ----------------------------------------------------------------------
            Traceback (most recent call last):
            File "tests/testOutputRoot.py", line 162, in testReuseOutputRoot
            outputs=testOutput)
            File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/butler.py", line 543, in _init_
            repoData.repo = Repository(repoData)
            File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/repository.py", line 138, in _init_
            self._initMapper(repoData)
            File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/repository.py", line 167, in _initMapper
            **mapperArgs)
            File "tests/testOutputRoot.py", line 57, in _init_
            policy=policy, repositoryDir=testPath, **kwargs)
            File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/cameraMapper.py", line 266, in _init_
            self._initMappings(policy, self.rootStorage, calibStorage, provided=None)
            File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/cameraMapper.py", line 372, in _initMappings
            mapping = cls(datasetType, subPolicy, self.registry, rootStorage, provided=provided)
            File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/mapping.py", line 314, in _init_
            Mapping._init_(self, datasetType, policy, registry, root, **kwargs)
            File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/mapping.py", line 96, in _init_
            datasetType))
            RuntimeError: Template may not be an empty string. Check the policy for datasetType deepCoadd_psfMatchedWarp

            Show
            npease Nate Pease added a comment - Kian-Tat Lim please also look at u/npease/8432_justtemplate , I only check that the policy ['template'] != '' . It's certainly the simplest implementation if it seems like it does enough of what's needed. Note any of these solutions are producing "incomplete policy" errors in the obs_base unit tests (an example is pasted in below), because lots of template are set to '' , and the expectation is that derived packages complete the package-specific portions of the policy. ====================================================================== ERROR: testReuseOutputRoot (_ main _.OutputRootTestCase) Set up an output repositoriy and verify its parent relationship to the input repository. ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/testOutputRoot.py", line 162, in testReuseOutputRoot outputs=testOutput) File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/butler.py", line 543, in _ init _ repoData.repo = Repository(repoData) File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/repository.py", line 138, in _ init _ self._initMapper(repoData) File "/Users/n8pease/3/lsstsw/stack/DarwinX86/daf_persistence/13.0-25-g49e493d/python/lsst/daf/persistence/repository.py", line 167, in _initMapper **mapperArgs) File "tests/testOutputRoot.py", line 57, in _ init _ policy=policy, repositoryDir=testPath, **kwargs) File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/cameraMapper.py", line 266, in _ init _ self._initMappings(policy, self.rootStorage, calibStorage, provided=None) File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/cameraMapper.py", line 372, in _initMappings mapping = cls(datasetType, subPolicy, self.registry, rootStorage, provided=provided) File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/mapping.py", line 314, in _ init _ Mapping._ init _(self, datasetType, policy, registry, root, **kwargs) File "/Users/n8pease/3/lsstsw/build/obs_base/python/lsst/obs/base/mapping.py", line 96, in _ init _ datasetType)) RuntimeError: Template may not be an empty string. Check the policy for datasetType deepCoadd_psfMatchedWarp
            Hide
            npease Nate Pease added a comment -

            changing the description of this story per RFC-369.

            Show
            npease Nate Pease added a comment - changing the description of this story per RFC-369 .
            Hide
            npease Nate Pease added a comment -

            Hi Kenny, can you please review this for me? The only branch that is affected (IE needs review) is obs_base. The PR isn't showing up yet in Jira, but you can find it at https://github.com/lsst/obs_base/pull/62

            Show
            npease Nate Pease added a comment - Hi Kenny, can you please review this for me? The only branch that is affected (IE needs review) is obs_base. The PR isn't showing up yet in Jira, but you can find it at https://github.com/lsst/obs_base/pull/62
            Hide
            kennylo Kenny Lo added a comment -

            I approve the changes.

            Show
            kennylo Kenny Lo added a comment - I approve the changes.

              People

              Assignee:
              npease Nate Pease
              Reporter:
              ktl Kian-Tat Lim
              Reviewers:
              Kenny Lo
              Watchers:
              Kenny Lo, Kian-Tat Lim, Nate Pease
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: