# Fix for PreFlight needed after schema migration

XMLWordPrintable

#### Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
1
• Sprint:
BG3_F18_09
• Team:
Data Access and Database

#### 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.

#### Activity

Hide
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
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
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
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
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
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
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
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
Andy Salnikov added a comment -

Merged

Show
Andy Salnikov added a comment - Merged

#### People

Assignee:
Andy Salnikov
Reporter:
Andy Salnikov
Reviewers:
Nate Lust
Watchers:
Andy Salnikov, Nate Lust, Vaikunth Thukral