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

Fix for PreFlight needed after schema migration

    Details

      Description

      DM-15336 changed API of the DataUnits which breaks PreFlight. Apparently unit tests that we have for PreFlight in daf_butler do not catch this sort of change. Need to check unit test to see if it can cover something like this. And fix the issue of course.

        Attachments

          Issue Links

            Activity

            Hide
            salnikov Andy Salnikov added a comment -

            Looking at the commits it looks like the most of the preflight was updated to changes in API. The error comes from spacial merge code:

                            # We also have to include regions from each side of the join
                            # into resultset so that we can filter-out non-overlapping
                            # regions.
                            regionColumns[regionHolder.name] = len(selectColumns)
            >               selectColumns.append(regionHolder.regionColumn)
            E               AttributeError: 'DataUnit' object has no attribute 'regionColumn'
             
            ../daf_butler/python/lsst/daf/butler/registries/sqlPreFlight.py:291: AttributeError
            

            and this codeis indeed not tested in daf_butler unit tests (it needs a reasonable SkyPix in a test registry which is hard to do in a simple unit test). But the error that I see comes when running unit test in pip_supertask, maybe I can move part of that test into daf_butler to improve coverage.

            Show
            salnikov Andy Salnikov added a comment - Looking at the commits it looks like the most of the preflight was updated to changes in API. The error comes from spacial merge code: # We also have to include regions from each side of the join # into resultset so that we can filter-out non-overlapping # regions. regionColumns[regionHolder.name] = len(selectColumns) > selectColumns.append(regionHolder.regionColumn) E AttributeError: 'DataUnit' object has no attribute 'regionColumn'   ../daf_butler/python/lsst/daf/butler/registries/sqlPreFlight.py:291: AttributeError and this codeis indeed not tested in daf_butler unit tests (it needs a reasonable SkyPix in a test registry which is hard to do in a simple unit test). But the error that I see comes when running unit test in pip_supertask, maybe I can move part of that test into daf_butler to improve coverage.
            Hide
            salnikov Andy Salnikov added a comment -

            Nate, I'm assigning this to you since you merged DM-15336, but let me know if you feel terrified by pre-flight. This is actually a trivial one-line fix for pre-flight, I think it was not spotted because we did not have unit test coverage for that piece of code. I have added a trivial unit test for that, and this also removes couple of methods from dataUnit module which cannot work any more. All unit tests work OK, Jenkins has started but will take some time as usual.

            Show
            salnikov Andy Salnikov added a comment - Nate, I'm assigning this to you since you merged DM-15336 , but let me know if you feel terrified by pre-flight. This is actually a trivial one-line fix for pre-flight, I think it was not spotted because we did not have unit test coverage for that piece of code. I have added a trivial unit test for that, and this also removes couple of methods from dataUnit module which cannot work any more. All unit tests work OK, Jenkins has started but will take some time as usual.
            Hide
            nlust Nate Lust added a comment -

            A few questions on how the unit test is implemented, but the fix itself looks fine. Sorry for introducing this bug in the first place.

            Show
            nlust Nate Lust added a comment - A few questions on how the unit test is implemented, but the fix itself looks fine. Sorry for introducing this bug in the first place.
            Hide
            salnikov Andy Salnikov added a comment -

            Nate Lust, thanks for review, it was really my fault that we did not have test coverage for whole preflight thing before.

            I made one small update to unit test to improve variable naming and reduce number of magic string literals. I do not see your approval on PR, but I guess I can merge it anyways as you marked JIRA ticket reviewed.

            Show
            salnikov Andy Salnikov added a comment - Nate Lust , thanks for review, it was really my fault that we did not have test coverage for whole preflight thing before. I made one small update to unit test to improve variable naming and reduce number of magic string literals. I do not see your approval on PR, but I guess I can merge it anyways as you marked JIRA ticket reviewed.
            Hide
            salnikov Andy Salnikov added a comment -

            Merged

            Show
            salnikov Andy Salnikov added a comment - Merged

              People

              • Assignee:
                salnikov Andy Salnikov
                Reporter:
                salnikov Andy Salnikov
                Reviewers:
                Nate Lust
                Watchers:
                Andy Salnikov, Nate Lust, Vaikunth Thukral
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel