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

Change root to config in config override files

    XMLWordPrintable

    Details

    • Story Points:
      2
    • Team:
      Alert Production

      Description

      Implement RFC-62 by using config rather than root in config override files for the root of the config.

      Note that I propose not modifying astrometry_net_data configs because those are numerous and hidden. They have their own special loader in LoadAstrometryNetObjectsTask._readIndexFiles which could easily be updated later. if desired. An obvious time to make such a transition would be when overhauling the way this data is unpersisted.

        Attachments

          Issue Links

            Activity

            Hide
            price Paul Price added a comment -

            It's not just when we release, but as soon as it goes to master, anyone who might be using master needs to know.

            Show
            price Paul Price added a comment - It's not just when we release, but as soon as it goes to master, anyone who might be using master needs to know.
            Hide
            rowen Russell Owen added a comment -

            To answer Paul Price: I did not find any instances of "config" used as a variable inside config override files. That means in theory we could continue to support root for backwards compatibility, while also supporting config. Doing just that much is trivial, but not adequate. To do it right we need to warn users when they load an outdated config override file, and I have not figured out how to do that.

            My inclination is to make a hard break. Converting config override files is easy.

            Show
            rowen Russell Owen added a comment - To answer Paul Price : I did not find any instances of "config" used as a variable inside config override files. That means in theory we could continue to support root for backwards compatibility, while also supporting config . Doing just that much is trivial, but not adequate. To do it right we need to warn users when they load an outdated config override file, and I have not figured out how to do that. My inclination is to make a hard break. Converting config override files is easy.
            Hide
            price Paul Price added a comment -

            I agree about a hard break (though it may be controversial). I was more worried about breaking things if someone was using config inside an override file already.

            Show
            price Paul Price added a comment - I agree about a hard break (though it may be controversial). I was more worried about breaking things if someone was using config inside an override file already.
            Hide
            rowen Russell Owen added a comment - - edited

            I found a safe way to support root:

            • try loading the override file using "configure"
            • if that raises NameError and the exception contains "root" (which is what happens if the file uses "root" instead of "config) then print a warning and try again, using "root".

            This seems safe to me. It will work with files that use "root" and contain a variable named "config". Similarly it will work for files that use "config" but contain a variable named "root. The only way I think it can cause problems is if the config override file has an error and the resulting message is a NameError whose message contains "root". In that case I would expect it to try loading again with "root" (which it should not do) and to fail with a confusing error message.

            I think we should remove this backwards compatibility code at some point, but give people some time to update their personal config files first.

            Show
            rowen Russell Owen added a comment - - edited I found a safe way to support root: try loading the override file using "configure" if that raises NameError and the exception contains "root" (which is what happens if the file uses "root" instead of "config) then print a warning and try again, using "root". This seems safe to me. It will work with files that use "root" and contain a variable named "config". Similarly it will work for files that use "config" but contain a variable named "root. The only way I think it can cause problems is if the config override file has an error and the resulting message is a NameError whose message contains "root". In that case I would expect it to try loading again with "root" (which it should not do) and to fail with a confusing error message. I think we should remove this backwards compatibility code at some point, but give people some time to update their personal config files first.
            Hide
            rowen Russell Owen added a comment -

            Yusra AlSayyad Thank you very much for the review.

            Regarding the two config files: you were right that I had corrupted them. The value of defaultRoot should be "/lsst/DC3root" in both cases.

            Regarding pex_config:python/lsst/pex/config/config.py I agree that the documentation is confusing. I took a cleanup pass on all doc strings for the Config object. I hope it is clearer now. (Field still needs such a pass, but I found enough bits that I didn't understand that I decided to leave it alone.)

            Show
            rowen Russell Owen added a comment - Yusra AlSayyad Thank you very much for the review. Regarding the two config files: you were right that I had corrupted them. The value of defaultRoot should be "/lsst/DC3root" in both cases. Regarding pex_config:python/lsst/pex/config/config.py I agree that the documentation is confusing. I took a cleanup pass on all doc strings for the Config object. I hope it is clearer now. (Field still needs such a pass, but I found enough bits that I didn't understand that I decided to leave it alone.)

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Watchers:
              Hsin-Fang Chiang, Kian-Tat Lim, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: