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

Move PexConfigFormatter to obs_base

    XMLWordPrintable

    Details

    • Story Points:
      1
    • Team:
      Architecture
    • Urgent?:
      No

      Description

      PexConfigFormatter is anomalous in that it's defined in daf_butler but can't be used by daf_butler, shouldn't be used by daf_butler and can't be tested in daf_butler.

      It should move to obs_base where the other lsst-specific formatters exist. It can then be tested properly in obs_base.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Adding DM-26008 as a blocker because that comes with a pexConfig formatter rewrite and test changes and I don't want to integrate the changes twice.

            Show
            tjenness Tim Jenness added a comment - Adding DM-26008 as a blocker because that comes with a pexConfig formatter rewrite and test changes and I don't want to integrate the changes twice.
            Hide
            tjenness Tim Jenness added a comment -

            Kian-Tat Lim, Jim Bosch I can merge the obs_base side of this (and daf_butler config change) without removing the daf_butler copy of the formatter if we want to proceed with this. The issue being that every time we move a formatter we de facto change registry compatibility since we record the formatter in datastore records. Rather than wait on DM-26008 one option is to merge what I have here and leave it in both places. The motivation for doing it this week is that we already know we have broken registry compatibility so it's a good candidate to do this along with the other breakage.

            Comments?

            Show
            tjenness Tim Jenness added a comment - Kian-Tat Lim , Jim Bosch I can merge the obs_base side of this (and daf_butler config change) without removing the daf_butler copy of the formatter if we want to proceed with this. The issue being that every time we move a formatter we de facto change registry compatibility since we record the formatter in datastore records. Rather than wait on DM-26008 one option is to merge what I have here and leave it in both places. The motivation for doing it this week is that we already know we have broken registry compatibility so it's a good candidate to do this along with the other breakage. Comments?
            Hide
            jbosch Jim Bosch added a comment -

            Merging the obs_base branch with a daf_butler commit that makes it default without removing the old formatter seems like the way we'll handle this in the future, and maybe worth practicing.

            But I never followed the discussion on why we're switching to yaml format for these, so I don't think I have anything to say on the relationship with that ticket.

            Show
            jbosch Jim Bosch added a comment - Merging the obs_base branch with a daf_butler commit that makes it default without removing the old formatter seems like the way we'll handle this in the future, and maybe worth practicing. But I never followed the discussion on why we're switching to yaml format for these, so I don't think I have anything to say on the relationship with that ticket.
            Hide
            tjenness Tim Jenness added a comment -

            This ticket is not switching to YAML. No change to see here. The YAML ticket is for things like pipetask to serialize the config but it has some test clean ups in Daf_butler (since tests relied on pexConfig existing).

            I will put in review.

            Show
            tjenness Tim Jenness added a comment - This ticket is not switching to YAML. No change to see here. The YAML ticket is for things like pipetask to serialize the config but it has some test clean ups in Daf_butler (since tests relied on pexConfig existing). I will put in review.
            Hide
            jbosch Jim Bosch added a comment -

            Reviewed. Do you plan to make this the default in daf_butler now, too? I don't think it's necessary, but it might keep repos made next week from breaking a bit longer.

            I only brought up YAML before because your first comment on this page made me think you'd be rewriting this formatter to use yaml instead on DM-26008. If you're just planning to add a new formatter for that, fine.

            Show
            jbosch Jim Bosch added a comment - Reviewed. Do you plan to make this the default in daf_butler now, too? I don't think it's necessary, but it might keep repos made next week from breaking a bit longer. I only brought up YAML before because your first comment on this page made me think you'd be rewriting this formatter to use yaml instead on DM-26008 . If you're just planning to add a new formatter for that, fine.
            Hide
            tjenness Tim Jenness added a comment -

            Yes, the motivation here is to switch daf_butler over to obs_base (and there was a branch to do that but the PR was not made) – it's a one line change.

            The DM-26008 change for YAML introduces a new class method constructor that can work out the correct class from the serialized form. This allows me to remove the try/except block you put in this formatter and lets it be significantly more straightforward. Also on that ticket I cleaned up the tests to not rely on a specific public formatter existing and switched the tests to using a special test formatter. I'll reapply the DM-26008 formatter clean ups to obs_base once this is merged but moving early means we can switch over any repos using configs when they are remade to support all the other compatibility changes from this week.

            Show
            tjenness Tim Jenness added a comment - Yes, the motivation here is to switch daf_butler over to obs_base (and there was a branch to do that but the PR was not made) – it's a one line change. The DM-26008 change for YAML introduces a new class method constructor that can work out the correct class from the serialized form. This allows me to remove the try/except block you put in this formatter and lets it be significantly more straightforward. Also on that ticket I cleaned up the tests to not rely on a specific public formatter existing and switched the tests to using a special test formatter. I'll reapply the DM-26008 formatter clean ups to obs_base once this is merged but moving early means we can switch over any repos using configs when they are remade to support all the other compatibility changes from this week.

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.