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

Remove pex_policy usage when not part of gen2 butler

    XMLWordPrintable

    Details

    • Story Points:
      4
    • Sprint:
      Arch 2019-08-26
    • Team:
      Architecture

      Description

      Once DM-21065 merges there are very few places outside of gen 2 butler that depend on pex_policy. Remove those final places and replace them with use of YAML and PropertySet.

        Attachments

          Issue Links

            Activity

            No builds found.
            tjenness Tim Jenness created issue -
            tjenness Tim Jenness made changes -
            Field Original Value New Value
            Component/s skypix [ 10724 ]
            Hide
            tjenness Tim Jenness added a comment - - edited

            Having looked at this a bit more deeply:

            1. Policy is used in Python code in daf_persistence, pex_config (makePolicy), obs_monocam, ip_diffim (for backwards compatibility once DM-21065 merged) and meas_algorithms (DM-21065).
            2. Tests use Policy in daf_persistence and pex_config (makePolicy).
            3. Many obs packages declare a pex_policy dependency even though they have no pex_policy paf files or code usage.
            4. A PAF file is found in meas_extensions_photometryKron but removing it does not cause any failures and there is no obvious place where the paf file would be read.
            5. There are a number of python examples that use Policy but it seems they are long bit-rotted referring to meas_astrom methods and packages that no longer exist.
            6. astrometry_net_data seems to have many .paf files (also one in meas_extensions_astrometryNet) but removing the paf files doesn't break anything.
            7. obs_cfht defines the Megacam filters in a paf file but I don't see the code to read that file.
            8. cat package has two paf files that seem to be unused.

            I think the plan on this ticket is therefore

            1. remove the unused paf files to avoid future confusion.
            2. Convert monocamMapper.paf to standard mapper YAML format used by other obs packages.
            3. Remove pex_policy dependency from obs packages that don't use pex_policy.
            Show
            tjenness Tim Jenness added a comment - - edited Having looked at this a bit more deeply: Policy is used in Python code in daf_persistence, pex_config (makePolicy), obs_monocam, ip_diffim (for backwards compatibility once DM-21065 merged) and meas_algorithms ( DM-21065 ). Tests use Policy in daf_persistence and pex_config (makePolicy). Many obs packages declare a pex_policy dependency even though they have no pex_policy paf files or code usage. A PAF file is found in meas_extensions_photometryKron but removing it does not cause any failures and there is no obvious place where the paf file would be read. There are a number of python examples that use Policy but it seems they are long bit-rotted referring to meas_astrom methods and packages that no longer exist. astrometry_net_data seems to have many .paf files (also one in meas_extensions_astrometryNet) but removing the paf files doesn't break anything. obs_cfht defines the Megacam filters in a paf file but I don't see the code to read that file. cat package has two paf files that seem to be unused. I think the plan on this ticket is therefore remove the unused paf files to avoid future confusion. Convert monocamMapper.paf to standard mapper YAML format used by other obs packages. Remove pex_policy dependency from obs packages that don't use pex_policy.
            tjenness Tim Jenness made changes -
            Component/s obs_cfht [ 10762 ]
            tjenness Tim Jenness made changes -
            Summary Remove pex_policy usage when not part of gen2 butler or cameraGeom Remove pex_policy usage when not part of gen2 butler
            tjenness Tim Jenness made changes -
            Description Once DM-21065 merges there are very few places outside of gen 2 butler or obs_cfht camera geometry that depend on pex_policy. Remove those final places and replace them with use of YAML and PropertySet. Once DM-21065 merges there are very few places outside of gen 2 butler that depend on pex_policy. Remove those final places and replace them with use of YAML and PropertySet.
            Hide
            tjenness Tim Jenness added a comment -

            Kian-Tat Lim I have removed paf files and pex_policy usage everywhere except daf_persistence and the formally deprecated code in DM-21065 (where pex_policy has to be imported for an isinstance check). obs_base does still have an entry for returning a Policy from a butler.

            Most changes are removals. The biggest change is a cleanup of obs_monocam (which I don't think works but I cleaned it up anyway).

            I think this puts us in good shape for the removal of pex_policy.

            Show
            tjenness Tim Jenness added a comment - Kian-Tat Lim I have removed paf files and pex_policy usage everywhere except daf_persistence and the formally deprecated code in DM-21065 (where pex_policy has to be imported for an isinstance check). obs_base does still have an entry for returning a Policy from a butler. Most changes are removals. The biggest change is a cleanup of obs_monocam (which I don't think works but I cleaned it up anyway). I think this puts us in good shape for the removal of pex_policy.
            tjenness Tim Jenness made changes -
            Reviewers Kian-Tat Lim [ ktl ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            tjenness Tim Jenness added a comment -

            There are nominally three distinct types of thing to review:

            1. Removal of unused PAF files: afw, obs_test, meas_extensions_astrometryNet, astrometry_net_data, obs_cfht, cat, meas_extensions_photometryKron
            2. Removal of broken examples that used pexPolicy: meas_astrom, meas_extensions_astrometryNet, meas_deblender, obs_cfht
            3. Removal of unnecessary dependency on pex_policy: obs_decam, obs_lsst, obs_lsstSim, obs_comCam, obs_sdss
            4. Switch from PAF mapper to YAML mapper and other cleanups: obs_monocam

            Jenkins passes with these changes. The only thing that really needs review is obs_monocam.

            Show
            tjenness Tim Jenness added a comment - There are nominally three distinct types of thing to review: Removal of unused PAF files: afw, obs_test, meas_extensions_astrometryNet, astrometry_net_data, obs_cfht, cat, meas_extensions_photometryKron Removal of broken examples that used pexPolicy: meas_astrom, meas_extensions_astrometryNet, meas_deblender, obs_cfht Removal of unnecessary dependency on pex_policy: obs_decam, obs_lsst, obs_lsstSim, obs_comCam, obs_sdss Switch from PAF mapper to YAML mapper and other cleanups: obs_monocam Jenkins passes with these changes. The only thing that really needs review is obs_monocam.
            tjenness Tim Jenness made changes -
            Component/s meas_astrom [ 10745 ]
            Component/s obs_decam [ 12851 ]
            Component/s obs_lsst [ 16504 ]
            Component/s obs_monocam [ 13412 ]
            Hide
            tjenness Tim Jenness added a comment -

            Simon Krughoff I've been asked to ask you about the removal of the examples. Can you please take a look at the pull requests for meas_astrom, meas_extensions_astrometryNet, meas_deblender, and obs_cfht to ensure that there is not some gold in there that needs to be retained?

            I asked about meas_deblender on Slack and Paul Price suggested that there was no problem with deleting those. For obs_cfht we know that the camera code from policy is broken (DM-11560) and is no longer the source of truth (DM-5524). I think you advocated its removal in DM-11560 and we of course will be wanting to rewrite all this using YAMLCamera.

            Show
            tjenness Tim Jenness added a comment - Simon Krughoff I've been asked to ask you about the removal of the examples. Can you please take a look at the pull requests for meas_astrom, meas_extensions_astrometryNet, meas_deblender, and obs_cfht to ensure that there is not some gold in there that needs to be retained? I asked about meas_deblender on Slack and Paul Price suggested that there was no problem with deleting those. For obs_cfht we know that the camera code from policy is broken ( DM-11560 ) and is no longer the source of truth ( DM-5524 ). I think you advocated its removal in DM-11560 and we of course will be wanting to rewrite all this using YAMLCamera.
            tjenness Tim Jenness made changes -
            Reviewers Kian-Tat Lim [ ktl ] Kian-Tat Lim, Simon Krughoff [ ktl, krughoff ]
            Hide
            ktl Kian-Tat Lim added a comment -

            Type 1, 3, 4 are OK.  Awaiting confirmation from others on Type 2.

            Show
            ktl Kian-Tat Lim added a comment - Type 1, 3, 4 are OK.  Awaiting confirmation from others on Type 2.
            Hide
            ktl Kian-Tat Lim added a comment -

            Simon Krughoff The question for obs_cfht is whether the existing broken code will be at all helpful in writing the new YAMLCamera.

            Show
            ktl Kian-Tat Lim added a comment - Simon Krughoff The question for obs_cfht is whether the existing broken code will be at all helpful in writing the new YAMLCamera.
            Hide
            krughoff Simon Krughoff added a comment -

            I absolutely think the broken examples and deprecated camera generation code should go away.

            Show
            krughoff Simon Krughoff added a comment - I absolutely think the broken examples and deprecated camera generation code should go away.
            krughoff Simon Krughoff made changes -
            Reviewers Kian-Tat Lim, Simon Krughoff [ ktl, krughoff ] Kian-Tat Lim [ ktl ]
            tjenness Tim Jenness made changes -
            Reviewers Kian-Tat Lim [ ktl ] Kian-Tat Lim, Simon Krughoff [ ktl, krughoff ]
            krughoff Simon Krughoff made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            tjenness Tim Jenness added a comment -

            Thanks for the reviews. All merged.

            Show
            tjenness Tim Jenness added a comment - Thanks for the reviews. All merged.
            tjenness Tim Jenness made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-358 [ DM-358 ]
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-11560 [ DM-11560 ]

              People

              Assignee:
              tjenness Tim Jenness
              Reporter:
              tjenness Tim Jenness
              Reviewers:
              Kian-Tat Lim, Simon Krughoff
              Watchers:
              Kian-Tat Lim, Simon Krughoff, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.