Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-626

Switch some C++ APIs that use pex_config information from Policy to PropertySet

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      We are trying to remove usage of pex_policy in our code so that we can retire it. There are some places that use Policy to receive configuration information (via pexConfig.makePolicy) that I would like to change to use pexConfig.makePropertySet.

      The routines in question are:

      • meas_algorithms.findCosmicRays (called from pipe_tasks and meas_algorithms)
      • ip_diffim (psf matching, kernels and regularization), seemingly only calling makePolicy internally.

      The proposal is:

      1. Change the C++ to use PropertySet
      2. Update the stack code to use the new interface.
      3. Provide a wrapper Python function that looks for the old API usage and warns, before converting the Policy to PropertySet and proceeding.
      4. Formally deprecate pexConfig.makePolicy and the Policy usage for these APIs.

        Attachments

          Issue Links

            Activity

            tjenness Tim Jenness created issue -
            tjenness Tim Jenness made changes -
            Field Original Value New Value
            Link This issue is triggering DM-21065 [ DM-21065 ]
            tjenness Tim Jenness made changes -
            Watchers John Swinbank, Kian-Tat Lim, Leanne Guy, Tim Jenness, Wil O'Mullane [ John Swinbank, Kian-Tat Lim, Leanne Guy, Tim Jenness, Wil O'Mullane ] Colin Slater, John Swinbank, Kian-Tat Lim, Leanne Guy, Michelle Butler, Tim Jenness, Wil O'Mullane [ Colin Slater, John Swinbank, Kian-Tat Lim, Leanne Guy, Michelle Butler, Tim Jenness, Wil O'Mullane ]
            Hide
            Parejkoj John Parejko added a comment -

            I'm a fan of any unneeded code removal plans. I like your python-wrapper deprecation plan.

            Show
            Parejkoj John Parejko added a comment - I'm a fan of any unneeded code removal plans. I like your python-wrapper deprecation plan.
            Hide
            ktl Kian-Tat Lim added a comment - - edited

            I suspect that a deprecation warning from makePolicy will be sufficient; no other means of creating a Policy to pass to these routines is likely to exist (except for test cases).

            Show
            ktl Kian-Tat Lim added a comment - - edited I suspect that a deprecation warning from makePolicy will be sufficient; no other means of creating a Policy to pass to these routines is likely to exist (except for test cases).
            Hide
            tjenness Tim Jenness added a comment -

            Warning from makePolicy won't stop the command breaking though. Are we saying I need to keep the old C++ routine around and also write a new one that takes a PropertySet? Or do I write a wrapper that notices the Policy, issues the warning, and then converts the Policy to PropertySet before using the updated C++ routine?

            Show
            tjenness Tim Jenness added a comment - Warning from makePolicy won't stop the command breaking though. Are we saying I need to keep the old C++ routine around and also write a new one that takes a PropertySet? Or do I write a wrapper that notices the Policy, issues the warning, and then converts the Policy to PropertySet before using the updated C++ routine?
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            I think for backward compatibility you would need to support both Policy and PropertySet in both C++ and Python, so the former. It would make migration when Policy is deleted simpler, too.

            Show
            krzys Krzysztof Findeisen added a comment - - edited I think for backward compatibility you would need to support both Policy and PropertySet in both C++ and Python, so the former. It would make migration when Policy is deleted simpler, too.
            Hide
            tjenness Tim Jenness added a comment -

            I want to change the code to use PropertySet. I don't want to also have to have the C++ code lying around for Policy (nothing in our code will be calling it)

            Show
            tjenness Tim Jenness added a comment - I want to change the code to use PropertySet. I don't want to also have to have the C++ code lying around for Policy (nothing in our code will be calling it)
            Hide
            tjenness Tim Jenness added a comment -

            Representative PR for meas_algorithms: https://github.com/lsst/meas_algorithms/pull/178

            Show
            tjenness Tim Jenness added a comment - Representative PR for meas_algorithms: https://github.com/lsst/meas_algorithms/pull/178
            tjenness Tim Jenness made changes -
            Description We are trying to remove usage of pex_policy in our code so that we can retire it. There are some places that use Policy to receive configuration information (via {{pexConfig.makePolicy}}) that I would like to change to use {{pexConfig.makePropertySet}}.

            The routines in question are:

            * meas_algorithms.findCosmicRays (called from pipe_tasks and meas_algorithms)
            * ip_diffim (psf matching, kernels and regularization), seemingly only calling makePolicy internally.

            Should I try to wrap the C++ in a new python layer that warns (following the deprecation policy) if a Policy is provided?

            Additionally we would like to formally deprecate {{pexConfig.makePolicy}}.
            We are trying to remove usage of pex_policy in our code so that we can retire it. There are some places that use Policy to receive configuration information (via {{pexConfig.makePolicy}}) that I would like to change to use {{pexConfig.makePropertySet}}.

            The routines in question are:

            * meas_algorithms.findCosmicRays (called from pipe_tasks and meas_algorithms)
            * ip_diffim (psf matching, kernels and regularization), seemingly only calling makePolicy internally.

            The proposal is:

            1. Change the C++ to use {{PropertySet}}
            2. Update the stack code to use the new interface.
            3. Provide a wrapper Python function that looks for the old API usage and warns, before converting the Policy to PropertySet and proceeding.
            4. Formally deprecate {{pexConfig.makePolicy}} and the Policy usage for these APIs.

            tjenness Tim Jenness made changes -
            Description We are trying to remove usage of pex_policy in our code so that we can retire it. There are some places that use Policy to receive configuration information (via {{pexConfig.makePolicy}}) that I would like to change to use {{pexConfig.makePropertySet}}.

            The routines in question are:

            * meas_algorithms.findCosmicRays (called from pipe_tasks and meas_algorithms)
            * ip_diffim (psf matching, kernels and regularization), seemingly only calling makePolicy internally.

            The proposal is:

            1. Change the C++ to use {{PropertySet}}
            2. Update the stack code to use the new interface.
            3. Provide a wrapper Python function that looks for the old API usage and warns, before converting the Policy to PropertySet and proceeding.
            4. Formally deprecate {{pexConfig.makePolicy}} and the Policy usage for these APIs.

            We are trying to remove usage of pex_policy in our code so that we can retire it. There are some places that use Policy to receive configuration information (via {{pexConfig.makePolicy}}) that I would like to change to use {{pexConfig.makePropertySet}}.

            The routines in question are:

            * meas_algorithms.findCosmicRays (called from pipe_tasks and meas_algorithms)
            * ip_diffim (psf matching, kernels and regularization), seemingly only calling makePolicy internally.

            The proposal is:

            # Change the C++ to use {{PropertySet}}
            # Update the stack code to use the new interface.
            # Provide a wrapper Python function that looks for the old API usage and warns, before converting the Policy to PropertySet and proceeding.
            # Formally deprecate {{pexConfig.makePolicy}} and the Policy usage for these APIs.

            Hide
            tjenness Tim Jenness added a comment -

            I've updated the text of the RFC to reflect my current understanding on implementation.

            Show
            tjenness Tim Jenness added a comment - I've updated the text of the RFC to reflect my current understanding on implementation.
            tjenness Tim Jenness made changes -
            Status Proposed [ 10805 ] Flagged [ 10606 ]
            gcomoretto Gabriele Comoretto made changes -
            Remote Link This issue links to "Page (Confluence)" [ 21437 ]
            gcomoretto Gabriele Comoretto made changes -
            Status Flagged [ 10606 ] Board Recommended [ 11405 ]
            tjenness Tim Jenness made changes -
            Status Board Recommended [ 11405 ] Adopted [ 10806 ]
            tjenness Tim Jenness made changes -
            Resolution Done [ 10000 ]
            Status Adopted [ 10806 ] Implemented [ 11105 ]
            gcomoretto Gabriele Comoretto made changes -
            Remote Link This issue links to "Page (Confluence)" [ 21647 ]

              People

              • Assignee:
                tjenness Tim Jenness
                Reporter:
                tjenness Tim Jenness
                Watchers:
                Colin Slater, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Leanne Guy, Michelle Butler, Tim Jenness, Wil O'Mullane
              • Votes:
                0 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel