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

Mechanism for deprecating Config Fields

    Details

      Description

      While working on DM-20154, I wanted to mark a Config field as deprecated, but we have no system for doing that (other than just adding a docstring). Kian-Tat Lim suggested an approach on slack, which I have implemented in DM-20378. An update to the developer guide is in progress.

      This design adds a deprecated attribute to pex.config.Field, which defaults to None. If it is not None, it is appended to the docstring of the Field, which lets it be preserved when the Config is saved. For example, if we have Field(doc="Some Text", deprecated="to remove in v1234"), the docstring for field would be "Some Text. Deprecated: to remove in v1234".

      The Field will also emit a FutureWarning if a value is assigned to it, to inform users about the deprecation.

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            Looks good. It would be nice if the entire config path could be given in the warning, rather than just the local field name, but I suspect that that is too hard to do.

            Show
            ktl Kian-Tat Lim added a comment - Looks good. It would be nice if the entire config path could be given in the warning, rather than just the local field name, but I suspect that that is too hard to do.
            Hide
            jsick Jonathan Sick added a comment -

            Super! Thanks to the docstring modification it's compatible with the task documentation extensions for Sphinx. I could imagine an optional ticket to further act on the deprecation state (like shuffle deprecated fields to the bottom of the config field list in the docs, or otherwise emphasize the deprecation. But as-is, this should work well.

            Show
            jsick Jonathan Sick added a comment - Super! Thanks to the docstring modification it's compatible with the task documentation extensions for Sphinx. I could imagine an optional ticket to further act on the deprecation state (like shuffle deprecated fields to the bottom of the config field list in the docs, or otherwise emphasize the deprecation. But as-is, this should work well.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            I think the field validation code (in Config base) can give a full path...

            for supporting deprecation, but I'm wondering what the workflow will be. Removing a config field breaks any config file that sets or reads that field, whether or not it uses the default value; is the plan to have users remove any assignments that trigger FutureWarning, then clean up any leftover references when the field is removed? Is there a way to instrument Field.__setitem__?

            Show
            krzys Krzysztof Findeisen added a comment - - edited I think the field validation code (in Config base) can give a full path... for supporting deprecation, but I'm wondering what the workflow will be. Removing a config field breaks any config file that sets or reads that field, whether or not it uses the default value; is the plan to have users remove any assignments that trigger FutureWarning , then clean up any leftover references when the field is removed? Is there a way to instrument Field.__setitem__ ?
            Hide
            Parejkoj John Parejko added a comment -

            I've modified it to output the full path in the warning.

            Krzysztof Findeisen: what do you mean about instrumenting Field._setitem_? Your sketch of the workflow is basically what I was envisioning. I don't know that there's anything else we can do, other than never remove Config Fields, which seems not ideal.

            Show
            Parejkoj John Parejko added a comment - I've modified it to output the full path in the warning. Krzysztof Findeisen : what do you mean about instrumenting Field._ setitem _ ? Your sketch of the workflow is basically what I was envisioning. I don't know that there's anything else we can do, other than never remove Config Fields, which seems not ideal.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I just looked at the DM-20378 code and you're basically already doing what I meant (i.e., warning on field assignment). From your description here I thought the fields were testing their state for a non-default value.

            Show
            krzys Krzysztof Findeisen added a comment - I just looked at the DM-20378 code and you're basically already doing what I meant (i.e., warning on field assignment). From your description here I thought the fields were testing their state for a non-default value.
            Hide
            Parejkoj John Parejko added a comment -

            Here's the update I plan to make to the developer guide (modulo some questions about pipelines docs links): https://developer.lsst.io/v/DM-20378/stack/deprecating-interfaces.html#config-deprecation

            Show
            Parejkoj John Parejko added a comment - Here's the update I plan to make to the developer guide (modulo some questions about pipelines docs links): https://developer.lsst.io/v/DM-20378/stack/deprecating-interfaces.html#config-deprecation
            Hide
            Parejkoj John Parejko added a comment -

            There were no objections, so I am adopting this with DM-20378 as the implementation ticket.

            Show
            Parejkoj John Parejko added a comment - There were no objections, so I am adopting this with DM-20378 as the implementation ticket.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Watchers:
                John Parejko, John Swinbank, Jonathan Sick, Kian-Tat Lim, Krzysztof Findeisen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel