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

Fix obs_* packages and ci tests broken by DM-4683

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ci_hsc, obs_cfht, obs_decam
    • Labels:
      None

      Description

      The butler changes in DM-4683, in particular the removal of .mapper from the interface exposed by a Butler object, broken obs_cfht, obs_decam, and ci_hsc.

      This issue will fix those changes, and search for additional broken things.

      This work is proceeding in conjunction with DM-5370 to test that the CI system, e.g. lsst_ci, is sensitive to these breakages and fixes.

        Attachments

          Activity

          No builds found.
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) created issue -
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Confirmed obs_cfht fails on the package name test from the change in DM-4683:

          https://ci.lsst.codes/job/stack-os-matrix/8973/label=centos-6/console

          Confirmed obs_decam fails on the package name test from the change in DM-4683 as tested by building lsst_ci (which happens to build obs_decam before obs_cfht).

          https://ci.lsst.codes/job/stack-os-matrix/8974/label=centos-6/consoleFull

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Confirmed obs_cfht fails on the package name test from the change in DM-4683 : https://ci.lsst.codes/job/stack-os-matrix/8973/label=centos-6/console Confirmed obs_decam fails on the package name test from the change in DM-4683 as tested by building lsst_ci (which happens to build obs_decam before obs_cfht ). https://ci.lsst.codes/job/stack-os-matrix/8974/label=centos-6/consoleFull
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Confirm that obs_decam and obs_cfht now pass with updates to the package name tests.

          https://ci.lsst.codes/job/stack-os-matrix/8978/label=centos-6/console

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Confirm that obs_decam and obs_cfht now pass with updates to the package name tests. https://ci.lsst.codes/job/stack-os-matrix/8978/label=centos-6/console
          Hide
          nlust Nate Lust added a comment - - edited

          This seems to affect obs_subaru as well This is not true, my branch was somehow only partially rebased off master, and should be fixed now

          Show
          nlust Nate Lust added a comment - - edited This seems to affect obs_subaru as well This is not true, my branch was somehow only partially rebased off master, and should be fixed now
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Nate Lust Yes, obs_subaru was already fixed as part of DM-4683 because it was part of lsst_distrib and thus it became clear when it broke. In the near future we will have these other obs_* packages in something that will be regularly built.

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Nate Lust Yes, obs_subaru was already fixed as part of DM-4683 because it was part of lsst_distrib and thus it became clear when it broke. In the near future we will have these other obs_* packages in something that will be regularly built.
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          ci_hsc is now fixed under this ticket. Build here

          https://ci.lsst.codes/job/stack-os-matrix/9013/label=centos-7/console

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - ci_hsc is now fixed under this ticket. Build here https://ci.lsst.codes/job/stack-os-matrix/9013/label=centos-7/console
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Nate Pease [X] 's suggestions for how to do do what the ci_hsc test was doing was indeed either Hsin-Fang Chiang 's hack:

          -        mappable = self.butler.repository._mapper.datasets.get(dataset, None)
          +        mappable = self.butler.mapper.datasets.get(dataset, None)
          

          or the only slightly better fix now in DM-5372:

          -        mappable = self.butler.mapper.datasets.get(dataset, None)
          +        mappers = self.butler.repository.mappers()
          +        mapper = mappers[0]  # Should only be one right now, but someday there can be several.
          +        mappable = mapper.datasets.get(dataset, None)
          

          But, in general, he was a bit troubled by this access pattern.

          """
          repository.mappers() is probably less brittle than repository._mapper, I'd go with that I guess. To Do The Right Thing, I think we would add a way to get something like a ButlerLocation by calling butler.get, and that contains the the persistence type. But that would take a little time (a day or 2, I expect)
          and would have to be scheduled.
          """

          Kian-Tat Lim didn't like the idea of checking the Python type of things returned with a dataset.
          Michael Wood-Vasey [X] and John Swinbank agreed we weren't really happy with this access pattern in the ci_hsc tests to work around DM-4927, but that it was acceptable to make immediate progress.

          Kian-Tat Lim then felt re-motivation to fix DM-4927 and there was much rejoicing.

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Nate Pease [X] 's suggestions for how to do do what the ci_hsc test was doing was indeed either Hsin-Fang Chiang 's hack: - mappable = self.butler.repository._mapper.datasets.get(dataset, None) + mappable = self.butler.mapper.datasets.get(dataset, None) or the only slightly better fix now in DM-5372 : - mappable = self.butler.mapper.datasets.get(dataset, None) + mappers = self.butler.repository.mappers() + mapper = mappers[0] # Should only be one right now, but someday there can be several. + mappable = mapper.datasets.get(dataset, None) But, in general, he was a bit troubled by this access pattern. """ repository.mappers() is probably less brittle than repository._mapper, I'd go with that I guess. To Do The Right Thing, I think we would add a way to get something like a ButlerLocation by calling butler.get, and that contains the the persistence type. But that would take a little time (a day or 2, I expect) and would have to be scheduled. """ Kian-Tat Lim didn't like the idea of checking the Python type of things returned with a dataset. Michael Wood-Vasey [X] and John Swinbank agreed we weren't really happy with this access pattern in the ci_hsc tests to work around DM-4927 , but that it was acceptable to make immediate progress. Kian-Tat Lim then felt re-motivation to fix DM-4927 and there was much rejoicing.
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          I found it interesting (reassuring?) that the breakage so far has all been tests that were trying to inspect something, but that method of inspection was changed.  It will be good to have CI improved to catch this (DM-5222 to build ci_hsc daily and on Pull Requests is now done and DM-5370 is close).

          But there has been no breakage reported by people actually just using Butler to do things with data. The re-factoring of DM-4692 being the understandable (and understandably frustrating) exception.

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - I found it interesting (reassuring?) that the breakage so far has all been tests that were trying to inspect something, but that method of inspection was changed.  It will be good to have CI improved to catch this ( DM-5222 to build ci_hsc daily and on Pull Requests is now done and DM-5370 is close). But there has been no breakage reported by people actually just using Butler to do things with data. The re-factoring of DM-4692 being the understandable (and understandably frustrating) exception.
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) made changes -
          Field Original Value New Value
          Status To Do [ 10001 ] In Progress [ 3 ]
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Quick review. These are all small changes.

          The GitHub Pull Requests are:
          https://github.com/lsst/obs_cfht/pull/10
          https://github.com/lsst/obs_decam/pull/19
          https://github.com/lsst/ci_hsc/pull/7

          (note that the list Pull Request for ci_hsc is using a new mechanism to automatically build pull requests to GitHub using Jenkins. It unfortunately doesn't actually work quite yet, so please ignore the message of failure listed by this pull request.

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Quick review. These are all small changes. The GitHub Pull Requests are: https://github.com/lsst/obs_cfht/pull/10 https://github.com/lsst/obs_decam/pull/19 https://github.com/lsst/ci_hsc/pull/7 (note that the list Pull Request for ci_hsc is using a new mechanism to automatically build pull requests to GitHub using Jenkins. It unfortunately doesn't actually work quite yet, so please ignore the message of failure listed by this pull request.
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) made changes -
          Reviewers Nate Pease [ npease ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Nate Pease [X]
          The successful manually Jenkins-submitted build is the one listed above, 9013.

          https://ci.lsst.codes/job/stack-os-matrix/9013/label=centos-7/console

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Nate Pease [X] The successful manually Jenkins-submitted build is the one listed above, 9013. https://ci.lsst.codes/job/stack-os-matrix/9013/label=centos-7/console
          Hide
          npease Nate Pease [X] (Inactive) added a comment -

          looks good. thanks.

          Show
          npease Nate Pease [X] (Inactive) added a comment - looks good. thanks.
          npease Nate Pease [X] (Inactive) made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          Hide
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

          Merged to master.
          Changes in obs_decam, obs_cfht, ci_hsc.

          Show
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - Merged to master. Changes in obs_decam , obs_cfht , ci_hsc .
          wmwood-vasey Michael Wood-Vasey [X] (Inactive) made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]
          frossie Frossie Economou made changes -
          Epic Link DM-3864 [ 20140 ]

            People

            Assignee:
            wmwood-vasey Michael Wood-Vasey [X] (Inactive)
            Reporter:
            wmwood-vasey Michael Wood-Vasey [X] (Inactive)
            Reviewers:
            Nate Pease [X] (Inactive)
            Watchers:
            Michael Wood-Vasey [X] (Inactive), Nate Lust, Nate Pease [X] (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.