Details

    • Team:
      Data Release Production

      Description

      This ticket adds registration, certify/decertify, and single-dataset lookups for calibration collections (it's just commits moved from the main DM-24432 branch for easier review). Complete calibration collection support will require making them queryable in queryDataIds and queryDatasets, and be done later.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Andy Salnikov, sorry I've been asking for a lot of reviews lately but you're probably the right one to look at this one, too.

            All changes are in daf_butler. As noted on the PR, I think it's ready for review but plan to delay the merge until other tickets are closer to being done, so there is no rush.

            Show
            jbosch Jim Bosch added a comment - Andy Salnikov , sorry I've been asking for a lot of reviews lately but you're probably the right one to look at this one, too. All changes are in daf_butler. As noted on the PR, I think it's ready for review but plan to delay the merge until other tickets are closer to being done, so there is no rush.
            Hide
            jbosch Jim Bosch added a comment -

            Christopher Waters, I'd also be interested in getting your feedback on the interfaces, but you don't need to look at implementations. I am only aiming for necessary-but-not-sufficient here, but I'd be interested to know what is missing as well as anything you'd do differently.

            Show
            jbosch Jim Bosch added a comment - Christopher Waters , I'd also be interested in getting your feedback on the interfaces, but you don't need to look at implementations. I am only aiming for necessary-but-not-sufficient here, but I'd be interested to know what is missing as well as anything you'd do differently.
            Hide
            czw Christopher Waters added a comment -

            I've read through the PR, focusing on the `certify` and `decertify` methods.  I have a few questions, but this seems to handle the basics of what's needed.

            1. `calibration_label` does not exist in the PR.  Is this now hidden/automatically determined?
            2. Where does the `is_calibration` information defined by a `datasetType`?  Is this globally set, or is it based on the dataset definition used in the `pipeBase` `Connection` class?
            3. `certify` looks reasonable.  Is there a way to copy the calibration collection before doing this?  If I want to create a new calibration collection, do I need to manually recertify all datasets that were in the old one?  I ask because I think this will be necessary for testing newly generated calibrations.
            4. Am I correct that the docstring on `decertify.timespan` means that if I want to certify a Defect map for one day only, I can `decertify` the existing Defects for that one day, and then re-`certify` the update for that day only without worrying about the end points for the existing Defects?  This is a case I can imagine happening, and is a lot more flexible than having to do the same thing with multiple operations.  This also handles what I think will be the usual `decertify` case (in which calibrations don't receive end dates until they've been replaced).
            Show
            czw Christopher Waters added a comment - I've read through the PR, focusing on the `certify` and `decertify` methods.  I have a few questions, but this seems to handle the basics of what's needed. `calibration_label` does not exist in the PR.  Is this now hidden/automatically determined? Where does the `is_calibration` information defined by a `datasetType`?  Is this globally set, or is it based on the dataset definition used in the `pipeBase` `Connection` class? `certify` looks reasonable.  Is there a way to copy the calibration collection before doing this?  If I want to create a new calibration collection, do I need to manually recertify all datasets that were in the old one?  I ask because I think this will be necessary for testing newly generated calibrations. Am I correct that the docstring on `decertify.timespan` means that if I want to certify a Defect map for one day only, I can `decertify` the existing Defects for that one day, and then re-`certify` the update for that day only without worrying about the end points for the existing Defects?  This is a case I can imagine happening, and is a lot more flexible than having to do the same thing with multiple operations.  This also handles what I think will be the usual `decertify` case (in which calibrations don't receive end dates until they've been replaced).
            Hide
            jbosch Jim Bosch added a comment -

            1. `calibration_label` does not exist in the PR. Is this now hidden/automatically determined?

            calibration_label will be going away entirely, just not on this ticket. So when this ticket lands, the ability to have CALIBRATION collections will exist, but neither the QuantumGraph generation code nor any concrete PipelineTask will use them.

            2. Where does the `is_calibration` information defined by a `datasetType`? Is this globally set, or is it based on the dataset definition used in the `pipeBase` `Connection` class?

            It will be based on the connections in the PipelineTask(s) that define the dataset. I'll be adding a field for it there.

            3. `certify` looks reasonable. Is there a way to copy the calibration collection before doing this? If I want to create a new calibration collection, do I need to manually recertify all datasets that were in the old one? I ask because I think this will be necessary for testing newly generated calibrations.

            I've just created DM-26638 for this, but also note that it may not be as necessary for that use case as you might think; you can also test new calibrations by having a multi-collection search path that starts with a new collection that has just the new calibrations (for just the range they should be valid) and then has the old calibration collection. Of course, that imposes some constraints on how you use collections in that workflow, which may not be desirable, so I'll be sure to add copying as well.

            4. Am I correct that the docstring on `decertify.timespan` means that if I want to certify a Defect map for one day only, I can `decertify` the existing Defects for that one day, and then re-`certify` the update for that day only without worrying about the end points for the existing Defects? This is a case I can imagine happening, and is a lot more flexible than having to do the same thing with multiple operations. This also handles what I think will be the usual `decertify` case (in which calibrations don't receive end dates until they've been replaced).

            Yup, that was exactly the intent.

            Show
            jbosch Jim Bosch added a comment - 1. `calibration_label` does not exist in the PR. Is this now hidden/automatically determined? calibration_label will be going away entirely, just not on this ticket. So when this ticket lands, the ability to have CALIBRATION collections will exist, but neither the QuantumGraph generation code nor any concrete PipelineTask will use them. 2. Where does the `is_calibration` information defined by a `datasetType`? Is this globally set, or is it based on the dataset definition used in the `pipeBase` `Connection` class? It will be based on the connections in the PipelineTask(s) that define the dataset. I'll be adding a field for it there. 3. `certify` looks reasonable. Is there a way to copy the calibration collection before doing this? If I want to create a new calibration collection, do I need to manually recertify all datasets that were in the old one? I ask because I think this will be necessary for testing newly generated calibrations. I've just created DM-26638 for this, but also note that it may not be as necessary for that use case as you might think; you can also test new calibrations by having a multi-collection search path that starts with a new collection that has just the new calibrations (for just the range they should be valid) and then has the old calibration collection. Of course, that imposes some constraints on how you use collections in that workflow, which may not be desirable, so I'll be sure to add copying as well. 4. Am I correct that the docstring on `decertify.timespan` means that if I want to certify a Defect map for one day only, I can `decertify` the existing Defects for that one day, and then re-`certify` the update for that day only without worrying about the end points for the existing Defects? This is a case I can imagine happening, and is a lot more flexible than having to do the same thing with multiple operations. This also handles what I think will be the usual `decertify` case (in which calibrations don't receive end dates until they've been replaced). Yup, that was exactly the intent.
            Hide
            czw Christopher Waters added a comment -

            All of those answers sound great. Thanks!

            Show
            czw Christopher Waters added a comment - All of those answers sound great. Thanks!
            Hide
            salnikov Andy Salnikov added a comment -

            Looks good if I understand everything correctly (my brain gets fragmented with too many unrelated projects ).

            Show
            salnikov Andy Salnikov added a comment - Looks good if I understand everything correctly (my brain gets fragmented with too many unrelated projects ).
            Hide
            jbosch Jim Bosch added a comment -

            This has been merged to the DM-26680 integration branch, where we're aggregating a number of schema changes to reduce master disruption. I'll close this ticket when that is merged to master.

            Show
            jbosch Jim Bosch added a comment - This has been merged to the DM-26680 integration branch, where we're aggregating a number of schema changes to reduce master disruption. I'll close this ticket when that is merged to master.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Andy Salnikov
              Watchers:
              Andy Salnikov, Christopher Waters, Jim Bosch
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.