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

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
1
• Team:
SQuaRE

#### 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.

#### Activity

No builds found.
Michael Wood-Vasey [X] (Inactive) created issue -
Hide
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
Michael Wood-Vasey [X] (Inactive) made changes -
Field Original Value New Value
Status To Do [ 10001 ] In Progress [ 3 ]
Hide
Michael Wood-Vasey [X] (Inactive) added a comment -

Quick review. These are all small changes.

(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
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.
Michael Wood-Vasey [X] (Inactive) made changes -
 Reviewers Nate Pease [ npease ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
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
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
Nate Pease [X] (Inactive) added a comment -

looks good. thanks.

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

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

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

#### People

Assignee:
Michael Wood-Vasey [X] (Inactive)
Reporter:
Michael Wood-Vasey [X] (Inactive)
Reviewers:
Nate Pease [X] (Inactive)
Watchers:
Michael Wood-Vasey [X] (Inactive), Nate Lust, Nate Pease [X] (Inactive)