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

Fix-up any code that uses butler.repository

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: butler, pipe_drivers
    • Labels:
      None

      Description

      See https://community.lsst.org/t/access-to-root-in-butler/991
      When running constructBias.py in lsst_apps w_2016_34 I get the error:
      AttributeError in map: 'Butler' object has no attribute 'repository'
      The source of the error is probably
      https://github.com/lsst/pipe_drivers/blob/master/python/lsst/pipe/drivers/constructCalibs.py#L585
      All code that uses butler.repository should be fixed. If that's too much work possibly the ticket could get changed to fix constructCalibs only for now.

        Attachments

          Issue Links

            Activity

            No builds found.
            aritter Andreas Ritter created issue -
            aritter Andreas Ritter made changes -
            Field Original Value New Value
            Link This issue blocks DM-7200 [ DM-7200 ]
            swinbank John Swinbank made changes -
            Epic Link DM-6172 [ 24685 ]
            Hide
            swinbank John Swinbank added a comment - - edited

            The code in constructCalibs.py only refers to the Butler's repository to set the CALIB_CREATION_ROOT header. However, I'm not sure this makes sense any more: as I understand it, Butlers may now refer to more than one repository, so there's not a simple mapping of Butler to root. (Nate Pease should be able to confirm if I'm correct.)

            Given that, the easy solution would be to just drop this header. Paul Price, what do you think?

            Show
            swinbank John Swinbank added a comment - - edited The code in constructCalibs.py only refers to the Butler's repository to set the CALIB_CREATION_ROOT header. However, I'm not sure this makes sense any more: as I understand it, Butlers may now refer to more than one repository, so there's not a simple mapping of Butler to root. ( Nate Pease should be able to confirm if I'm correct.) Given that, the easy solution would be to just drop this header. Paul Price , what do you think?
            swinbank John Swinbank made changes -
            Story Points 2
            Team Data Release Production [ 10301 ]
            Hide
            npease Nate Pease added a comment -

            John Swinbank I would say there is not a 1:1 mapping of Butler to root (probably this is what you meant by "simple mapping"). Since this change was introduced (that is: multiple repositories in Butler) we've discussed the possibility that Butler users may need to be able to get the root value(s) from a butler via a proper API, but so far we've been able to determine that the root value can/should come from somewhere else in the script (as opposed to extracted from Butler).

            If you decide not to drop the header let's discuss ideas for how to get root.

            Show
            npease Nate Pease added a comment - John Swinbank I would say there is not a 1:1 mapping of Butler to root (probably this is what you meant by "simple mapping"). Since this change was introduced (that is: multiple repositories in Butler) we've discussed the possibility that Butler users may need to be able to get the root value(s) from a butler via a proper API, but so far we've been able to determine that the root value can/should come from somewhere else in the script (as opposed to extracted from Butler). If you decide not to drop the header let's discuss ideas for how to get root.
            Hide
            price Paul Price added a comment -

            I'm trying to think what we can replace this with, but haven't been able to come up with anything that would be meaningful. I guess that means I don't oppose just dropping it.

            Show
            price Paul Price added a comment - I'm trying to think what we can replace this with, but haven't been able to come up with anything that would be meaningful. I guess that means I don't oppose just dropping it.
            Hide
            swinbank John Swinbank added a comment -

            Given the above, I'm inclined to think we should simply drop this header to unblock DM-7200 rather than search the rest of the codebase for other places where this might be happening: those, we can handle on a case-by-case basis as we find them. Andreas Ritter, would that satisfy you?

            Show
            swinbank John Swinbank added a comment - Given the above, I'm inclined to think we should simply drop this header to unblock DM-7200 rather than search the rest of the codebase for other places where this might be happening: those, we can handle on a case-by-case basis as we find them. Andreas Ritter , would that satisfy you?
            Hide
            aritter Andreas Ritter added a comment -

            Absolutely fine with me

            Show
            aritter Andreas Ritter added a comment - Absolutely fine with me
            aritter Andreas Ritter made changes -
            Assignee Andreas Ritter [ aritter ]
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            aritter Andreas Ritter added a comment -

            Removed the butler.repository line

            Show
            aritter Andreas Ritter added a comment - Removed the butler.repository line
            aritter Andreas Ritter made changes -
            Reviewers Robert Lupton [ rhl ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            rhl Robert Lupton added a comment -

            The fix is fine.

            Did we capture the desire to save this information? It isn't available from the butler for the reasons given, but I think that the script (constructCalibs.py) does know its root – is that right, Paul Price? If so, couldn't we get it from there?

            Show
            rhl Robert Lupton added a comment - The fix is fine. Did we capture the desire to save this information? It isn't available from the butler for the reasons given, but I think that the script (constructCalibs.py) does know its root – is that right, Paul Price ? If so, couldn't we get it from there?
            rhl Robert Lupton made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            price Paul Price added a comment -

            Note that it's not the butler root directory you want, but the calibs root directory. I think that should be available from the argument parser, but you may have to apply the same defaults logic as in the butler.

            Show
            price Paul Price added a comment - Note that it's not the butler root directory you want, but the calibs root directory. I think that should be available from the argument parser, but you may have to apply the same defaults logic as in the butler.
            Hide
            price Paul Price added a comment -

            This has been merged to master.

            Show
            price Paul Price added a comment - This has been merged to master.
            price Paul Price made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              aritter Andreas Ritter
              Reporter:
              aritter Andreas Ritter
              Reviewers:
              Robert Lupton
              Watchers:
              Andreas Ritter, John Swinbank, Nate Pease, Paul Price, Robert Lupton
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.