# obs_decam calibration ingest uses fragile relative paths

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• 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:

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

## Activity

Colin Slater created issue -
Tim Jenness made changes -
Field Original Value New Value
Labels SciencePipelines
Colin Slater made changes -
 Link This issue relates to DM-5899 [ DM-5899 ]
Hsin-Fang Chiang made changes -
 Epic Link DM-6586 [ 25414 ]
Meredith Rawls made changes -
 Assignee Meredith Rawls [ mrawls ]
Meredith Rawls made changes -
 Link This issue relates to DM-5467 [ DM-5467 ]
Meredith Rawls made changes -
 Sprint Alert Production F16 - 11 [ 289 ] Story Points 6 Team Alert Production [ 10300 ]
Meredith Rawls made changes -
 Link This issue relates to DM-5467 [ DM-5467 ]
Hide
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
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..
Hsin-Fang Chiang made changes -
 Link This issue relates to DM-6722 [ DM-6722 ]
Simon Krughoff made changes -
 Sprint Alert Production F16 - 11 [ 289 ] Alert Production F16 - 11, Alert Production F16 - 11b [ 289, 295 ]
Simon Krughoff made changes -
 Rank Ranked higher
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 ]
Meredith Rawls made changes -
 Status To Do [ 10001 ] In Progress [ 3 ]
Meredith Rawls made changes -
 Link This issue relates to DM-5467 [ DM-5467 ]
Meredith Rawls made changes -
 Link This issue relates to DM-5467 [ DM-5467 ]
Meredith Rawls made changes -
 Link This issue relates to DM-5467 [ DM-5467 ]
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 ]
Simon Krughoff made changes -
 Rank Ranked higher
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 ]
Simon Krughoff made changes -
 Rank Ranked higher
Simon Krughoff made changes -
Simon Krughoff made changes -
 Rank Ranked higher
Hide
Paul Price added a comment -

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

Show
Paul Price added a comment - I think the proper fix to this issue is DM-6722 .
Hide
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
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
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
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
Meredith Rawls added a comment -

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

Show
Meredith Rawls added a comment - Would you be willing to review this please, Hsin-Fang Chiang ? It should be relatively quick.
Meredith Rawls made changes -
 Reviewers Hsin-Fang Chiang [ hchiang2 ] Status In Progress [ 3 ] In Review [ 10004 ]
Meredith Rawls made changes -
 Link This issue relates to RFC-299 [ RFC-299 ]
Meredith Rawls made changes -
 Status In Review [ 10004 ] In Progress [ 3 ]
Hide
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
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.
Simon Krughoff made changes -
Simon Krughoff made changes -
 Rank Ranked higher
Hide
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.

Show
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
Meredith Rawls made changes -
 Status In Progress [ 3 ] In Review [ 10004 ]
Hide
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
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.
Hsin-Fang Chiang made changes -
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Meredith Rawls made changes -
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]

## People

• Assignee:
Meredith Rawls
Reporter:
Colin Slater
Reviewers:
Hsin-Fang Chiang
Watchers:
Colin Slater, Hsin-Fang Chiang, Meredith Rawls, Paul Price