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

Move table versions to Schema

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:

      Description

      We've added a version number to afw::table::BaseTable to help with the transition to a new approach with different naming conventions and FunctorKeys instead of compound keys. However, it looks like Schema objects need to know about this version number as well, in order to change Subschema objects to split/join using underscores instead of periods. That means we'll have to move the version down into the schema object, which may affect a lot of downstream code. Other than touching a lot of code, the changes should be trivial.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Reversing this review; I took a look at Perry's code (which was fine, but could be simplified in some case), and made some modifications, which I'd now like him to look at.

            Updates are on u/jbosch/DM-958 of afw, meas_base, and pipe_tasks. ip_diffim is unchanged, so changes there are still on tickets/DM-958.

            To summarize:

            • in afw, I added a constructor to Schema set the version and made SchemaMapper use the version of the input Schema to set the version of the output Schema when it's not given. I did not remove Schema.setVersion() as we'd discussed before; that would have caused too much trouble with the "minimal schema" mechanisms, and I think it's better to just leave Schema.setVersion() and be careful with it until we can remove it. Looks like being safe hasn't been that difficult so far.
            • in meas_base, I removed a lot of the explicit setVersion() calls in the test code, because the version is now ensured by either the changes in afw or a change in meas/base/tests.py
            • in pipe_tasks, I made it so the tableVersion used by CalibrateTask is set by the version of its own measurement subtask instead of passed by ProcessImage. But I've also added validate checks to both tasks to make sure the tableVersion is the same for all three measurement subtasks (I think this was de facto required before, and if not I think it's an okay thing to require).

            Perry, could you take a quick look at my branches and see if you disagree with anything, and then run your one-off processCcd test suite as well?

            This is not a rush (aside from just the end of the sprint at the end of the week), as I'll do other tickets on top of these branches and rebase later.

            Show
            jbosch Jim Bosch added a comment - Reversing this review; I took a look at Perry's code (which was fine, but could be simplified in some case), and made some modifications, which I'd now like him to look at. Updates are on u/jbosch/ DM-958 of afw, meas_base, and pipe_tasks. ip_diffim is unchanged, so changes there are still on tickets/ DM-958 . To summarize: in afw, I added a constructor to Schema set the version and made SchemaMapper use the version of the input Schema to set the version of the output Schema when it's not given. I did not remove Schema.setVersion() as we'd discussed before; that would have caused too much trouble with the "minimal schema" mechanisms, and I think it's better to just leave Schema.setVersion() and be careful with it until we can remove it. Looks like being safe hasn't been that difficult so far. in meas_base, I removed a lot of the explicit setVersion() calls in the test code, because the version is now ensured by either the changes in afw or a change in meas/base/tests.py in pipe_tasks, I made it so the tableVersion used by CalibrateTask is set by the version of its own measurement subtask instead of passed by ProcessImage. But I've also added validate checks to both tasks to make sure the tableVersion is the same for all three measurement subtasks (I think this was de facto required before, and if not I think it's an okay thing to require). Perry, could you take a quick look at my branches and see if you disagree with anything, and then run your one-off processCcd test suite as well? This is not a rush (aside from just the end of the sprint at the end of the week), as I'll do other tickets on top of these branches and rebase later.
            Hide
            jbosch Jim Bosch added a comment -

            Recording the outcome of a HipChat discussion here (and a little work I did just after):

            • We've agreed to merge this to master after DM-984, so Perry can review and test the combination of the two easier. So when DM-984 is merged, I'll rebase the DM-958 branches on master for testing, and then Perry will do his review.
            • I've added a u/jbosch/DM-958 branch for obs_sdss, which needed modifications to its custom SdssCalibrateTask to mirror those I made to CalibrateTask in pipe_base.
            Show
            jbosch Jim Bosch added a comment - Recording the outcome of a HipChat discussion here (and a little work I did just after): We've agreed to merge this to master after DM-984 , so Perry can review and test the combination of the two easier. So when DM-984 is merged, I'll rebase the DM-958 branches on master for testing, and then Perry will do his review. I've added a u/jbosch/ DM-958 branch for obs_sdss, which needed modifications to its custom SdssCalibrateTask to mirror those I made to CalibrateTask in pipe_base.
            Hide
            pgee Perry Gee added a comment -

            I reviewed your changes to my changes. There really wasn't a lot to review, as you chose not to remove setVersion().

            Hope you reviewed tickets/DM-958 on the other packages, as they are all still my code.

            Show
            pgee Perry Gee added a comment - I reviewed your changes to my changes. There really wasn't a lot to review, as you chose not to remove setVersion(). Hope you reviewed tickets/ DM-958 on the other packages, as they are all still my code.
            Hide
            jbosch Jim Bosch added a comment -

            The tickets/DM-958 branches for other packages look fine.

            I've rebase meas_base u/jbosch/DM-958 on master (following the merge of DM-984). The only conflict was in WrappedSingleFramePlugin.fail() in sfm.py, which was quite easy to resolve. So I'm actually not too worried about those two issues interacting badly.

            We can now build a complete stack of DM-958 with the following branches:

            • afw: u/jbosch/DM-958
            • meas_base: u/jbosch/DM-958
            • meas_algorithms: tickets/DM-958
            • meas_deblender: tickets/DM-958
            • ip_diffim: tickets/DM-958
            • meas_astrom tickets/DM-958
            • pipe_tasks u/jbosch/DM-958
            • obs_sdss u/jbosch/DM-958
              I've done that on my own laptop, and I see no unit test failures, so I'd be very surprised if buildbot turns anything else up. I'm waiting to hear from Robyn as to whether I should kick off a Buildbot run; looks like something new might be broken there.
            Show
            jbosch Jim Bosch added a comment - The tickets/ DM-958 branches for other packages look fine. I've rebase meas_base u/jbosch/ DM-958 on master (following the merge of DM-984 ). The only conflict was in WrappedSingleFramePlugin.fail() in sfm.py, which was quite easy to resolve. So I'm actually not too worried about those two issues interacting badly. We can now build a complete stack of DM-958 with the following branches: afw: u/jbosch/ DM-958 meas_base: u/jbosch/ DM-958 meas_algorithms: tickets/ DM-958 meas_deblender: tickets/ DM-958 ip_diffim: tickets/ DM-958 meas_astrom tickets/ DM-958 pipe_tasks u/jbosch/ DM-958 obs_sdss u/jbosch/ DM-958 I've done that on my own laptop, and I see no unit test failures, so I'd be very surprised if buildbot turns anything else up. I'm waiting to hear from Robyn as to whether I should kick off a Buildbot run; looks like something new might be broken there.
            Hide
            jbosch Jim Bosch added a comment -

            Buildbot run for this issue is now in progress: http://lsst-buildx.ncsa.illinois.edu:8010/builders/DM_stack/builds/409

            Show
            jbosch Jim Bosch added a comment - Buildbot run for this issue is now in progress: http://lsst-buildx.ncsa.illinois.edu:8010/builders/DM_stack/builds/409

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch, Perry Gee
              Watchers:
              Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.