Fix liftover caching bug in tests #878
Conversation
bpblanken
changed the title
bpblanken
changed the title
| # This runs "before" as task to account for situations where | ||
| # the Hail write fails and we do not have the chance to | ||
| # run this method. | ||
| remove_liftover() |
There was a problem hiding this comment.
I understand why we want this for testing, but in production why wouldn't we want to use the cached liftover form a previous run?
There was a problem hiding this comment.
In some sense yes. But our current setup runs almost task in a fresh dataproc job so we're getting an empty and clean Hail for the most part anyways.
I could definitely be convinced to move this into the tearDown of the tests since that's more logically correct and more performant (but likely not noticeably), but I think it's easier to reason about this in the application code as part of the initialization process for a task. I don't like there being sneaky state inside of a task context without it being clear as to why.
There was a problem hiding this comment.
I don't like there being sneaky state inside of a task context without it being clear as to why.
Maybe update the comment to reflect this to make it clearer why this is being cleaned up at all (the current comment is just explaining why it runs in the before but not why its being run at all)
…s into benb/fix_caching_bug_in_tests
…tute/seqr-loading-pipelines into benb/fix_caching_bug_in_tests
There's a sneaky issue we're running into in the tests with hail's liftover being cached between unit tests. We're also not correctly using the locally stored leftovers but we should be!