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

obs_decam calibration ingest uses fragile relative paths

    Details

    • Story Points:
      6
    • Sprint:
      Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1, Alert Production S17 - 2, Alert Production S17 - 3
    • Team:
      Alert Production

      Description

      When ingesting calibration files with obs_decam, the current configuration in the mapper policy is to save in the registry the path from the current working directory to the ingested files. This is very fragile and can only work if ingestCalibs.py is run from within the calibration repository itself, and there are numerous ways in which this can still fail. The ingest script and mapper policy file should be updated to specify a path template, and to symlink or copy the calibration files into the repository as is done for ingesting images.

        Attachments

          Issue Links

            Activity

            ctslater Colin Slater created issue -
            tjenness Tim Jenness made changes -
            Field Original Value New Value
            Labels SciencePipelines
            ctslater Colin Slater made changes -
            Link This issue relates to DM-5899 [ DM-5899 ]
            hchiang2 Hsin-Fang Chiang made changes -
            Epic Link DM-6586 [ 25414 ]
            mrawls Meredith Rawls made changes -
            Assignee Meredith Rawls [ mrawls ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to DM-5467 [ DM-5467 ]
            mrawls Meredith Rawls made changes -
            Sprint Alert Production F16 - 11 [ 289 ]
            Story Points 6
            Team Alert Production [ 10300 ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to DM-5467 [ DM-5467 ]
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            This and DM-6722 are different but I could see one may potentially affect the other. Linking them just in case..

            Show
            hchiang2 Hsin-Fang Chiang added a comment - This and DM-6722 are different but I could see one may potentially affect the other. Linking them just in case..
            hchiang2 Hsin-Fang Chiang made changes -
            Link This issue relates to DM-6722 [ DM-6722 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11 [ 289 ] Alert Production F16 - 11, Alert Production F16 - 11b [ 289, 295 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11, Alert Production F16 - 11b [ 289, 295 ] Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c [ 289, 295, 296 ]
            mrawls Meredith Rawls made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to DM-5467 [ DM-5467 ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to DM-5467 [ DM-5467 ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to DM-5467 [ DM-5467 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c [ 289, 295, 296 ] Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12 [ 289, 295, 296, 305 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12 [ 289, 295, 296, 305 ] Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1 [ 289, 295, 296, 305, 355 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1 [ 289, 295, 296, 305, 355 ] Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1, Alert Production S17 - 2 [ 289, 295, 296, 305, 355, 361 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Hide
            price Paul Price added a comment -

            I think the proper fix to this issue is DM-6722.

            Show
            price Paul Price added a comment - I think the proper fix to this issue is DM-6722 .
            Hide
            ctslater Colin Slater added a comment -

            I don't see how DM-6722 proposes a fix for this issue?

            As Paul Price suggested on github, I think having paths defined relative to the calib root would be a significant improvement over the current situation. However, I don't quite understand why we are using paths at all instead of templates. This seems to break the pattern established for the rest of the non-calib repositories.

            Show
            ctslater Colin Slater added a comment - I don't see how DM-6722 proposes a fix for this issue? As Paul Price suggested on github, I think having paths defined relative to the calib root would be a significant improvement over the current situation. However, I don't quite understand why we are using paths at all instead of templates. This seems to break the pattern established for the rest of the non-calib repositories.
            Hide
            ctslater Colin Slater added a comment -

            I should have been a bit clearer. Since it's only the community pipeline-produced calibs that use paths, I had assumed it would make sense to have them mirror the LSST-produced calib templates. E.g.,

                cpBias: {
                    template:    "%(path)s[%(ccdnum)d]"
                    ...
            

            becomes

                cpBias: {
                    template:    "cpBIAS/%(calibDate)s/BIAS-%(calibDate)s.fits[%(ccdnum)d]"
                    ...
            

            Is there anything I'm missing that would make this not work?

            Show
            ctslater Colin Slater added a comment - I should have been a bit clearer. Since it's only the community pipeline-produced calibs that use paths, I had assumed it would make sense to have them mirror the LSST-produced calib templates. E.g., cpBias: { template: "%(path)s[%(ccdnum)d]" ... becomes cpBias: { template: "cpBIAS/%(calibDate)s/BIAS-%(calibDate)s.fits[%(ccdnum)d]" ... Is there anything I'm missing that would make this not work?
            Hide
            mrawls Meredith Rawls added a comment -

            Would you be willing to review this please, Hsin-Fang Chiang? It should be relatively quick.

            Show
            mrawls Meredith Rawls added a comment - Would you be willing to review this please, Hsin-Fang Chiang ? It should be relatively quick.
            mrawls Meredith Rawls made changes -
            Reviewers Hsin-Fang Chiang [ hchiang2 ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            mrawls Meredith Rawls made changes -
            Link This issue relates to RFC-299 [ RFC-299 ]
            mrawls Meredith Rawls made changes -
            Status In Review [ 10004 ] In Progress [ 3 ]
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            A few comments for now:

            • Coordinated change in testdata_decam is needed, for example the calibRegistry.sqlite3 there likely needs to be regenerated.
            • How about DECam defects? Currently its template isn't changed in your PR. Do you intend to continue using path for defects?
            • Similar to what Paul Price mentioned, does this affect ingesting LSST-generated calibration data? Sorry a lot more manual tests may be needed than usual, as very little of this code is tested automatically (if at all). (It would be great to add automatic tests somehow, but I understand if you think that's beyond this ticket. )
            • Documentation would be nice. obs_decam README will need updates if this results in any operational changes.
            Show
            hchiang2 Hsin-Fang Chiang added a comment - A few comments for now: Coordinated change in testdata_decam is needed, for example the calibRegistry.sqlite3 there likely needs to be regenerated. How about DECam defects ? Currently its template isn't changed in your PR. Do you intend to continue using path for defects? Similar to what Paul Price mentioned, does this affect ingesting LSST-generated calibration data? Sorry a lot more manual tests may be needed than usual, as very little of this code is tested automatically (if at all). (It would be great to add automatic tests somehow, but I understand if you think that's beyond this ticket. ) Documentation would be nice. obs_decam README will need updates if this results in any operational changes.
            krughoff Simon Krughoff made changes -
            Sprint Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1, Alert Production S17 - 2 [ 289, 295, 296, 305, 355, 361 ] Alert Production F16 - 11, Alert Production F16 - 11b, Alert Production F16 - 11c, Alert Production S17 - 12, Alert Production S17 - 1, Alert Production S17 - 2, Alert Production S17 - 3 [ 289, 295, 296, 305, 355, 361, 605 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Hide
            mrawls Meredith Rawls added a comment -

            I think this is truly ready for review now!

            I did need to update the registry in testdata_decam so all the tests would pass. Updating defect ingestion is outside the scope of this ticket; see DM-5467. I added a note to the obs_decam README about the new mode=link option/suggestion for biases and flats. The only potential backwards compatibility issue I see is with obs_decam users who already have a calibRegistry with the old "path" column in the flat and bias tables. I was thinking I would post on Community about this once the review is complete.

            Successful Jenkins builds #22435
            https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/22435/console
            https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py3/22435/console
            https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py2/22435/console
            https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py3/22435/console

            Show
            mrawls Meredith Rawls added a comment - I think this is truly ready for review now! I did need to update the registry in testdata_decam so all the tests would pass. Updating defect ingestion is outside the scope of this ticket; see DM-5467 . I added a note to the obs_decam README about the new mode=link option/suggestion for biases and flats. The only potential backwards compatibility issue I see is with obs_decam users who already have a calibRegistry with the old "path" column in the flat and bias tables. I was thinking I would post on Community about this once the review is complete. Successful Jenkins builds #22435 https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/22435/console https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py3/22435/console https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py2/22435/console https://ci.lsst.codes/job/stack-os-matrix/label=osx,python=py3/22435/console
            mrawls Meredith Rawls made changes -
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            The code changes look okay to me. Thanks for the hard work.

            For future reference, defect continues to use path. And the paths are relative paths from where the command is executed.

            I don't mind using path; after all obs_base does say it's okay to use path. I was hoping the path could be relative to the calib directory, as Paul Price commented on the PR. But I understand it's already a lot in this ticket and you want to defer that to later.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - The code changes look okay to me. Thanks for the hard work. For future reference, defect continues to use path . And the paths are relative paths from where the command is executed. I don't mind using path ; after all obs_base does say it's okay to use path . I was hoping the path could be relative to the calib directory, as Paul Price commented on the PR. But I understand it's already a lot in this ticket and you want to defer that to later.
            hchiang2 Hsin-Fang Chiang made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            mrawls Meredith Rawls made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              • Assignee:
                mrawls Meredith Rawls
                Reporter:
                ctslater Colin Slater
                Reviewers:
                Hsin-Fang Chiang
                Watchers:
                Colin Slater, Hsin-Fang Chiang, Meredith Rawls, Paul Price
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel