Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-22599: Develop PipelineTask unit test framework #114

Closed
wants to merge 10 commits into from

Conversation

kfindeisen
Copy link
Member

@kfindeisen kfindeisen commented Jan 23, 2020

This PR adds a module, lsst.pipe.base.tests, with test utilities specific to PipelineTask subclasses. The intent of these tests is to enable unit testing of:

  • whether a task's Connections are correctly written and whether they match the inputs and outputs of the run method
  • any logic in a custom runQuantum method
  • configuration logic, such as optional or alternative inputs or outputs

Note that, because it depends on a real Butler, the test code has some performance limitations: each call to makeTestRepo takes 4-6 seconds on my machine and each call to makeUniqueButler takes an extra second.

The code has been cleaned up, and tests added.
It's hard to explicitly provide correct keys without understanding how
the Butler dimensions system works in detail. Moving key constraints
to automated (if simple-minded) code greatly reduces the burden
on callers.
Each test should have its own collection for isolation, but creating a
completely new repository each time is impractical.
The code has been cleaned up, and minimal tests added.
@timj
Copy link
Member

timj commented Jan 23, 2020

Is all the time in butler.registry.insertDimensionData?

@kfindeisen
Copy link
Member Author

No, it's about 80% from Butler.makeRepo. The entire loop with insertDimensionData is a 10% contribution.

@timj
Copy link
Member

timj commented Jan 23, 2020

Weird because for me running makeButlerRepo on the command line takes 1.2s on my laptop and includes import overhead of all the butler code. I don't understand how Butler.makeRepo can take 5 seconds with everything pre-imported.

@timj
Copy link
Member

timj commented Jan 23, 2020

The daf_butler tests create very many new butler repos and there are only two tests that take longer than one second. One is an S3 test that takes just over a second and the other is a big registry test taking 7 seconds. If making a repository for tests had 5 seconds overhead the tests in daf_butler would take an incredible amount of time. What do you get if you run the daf_butler tests with pytest --durations=10 tests ? Are they all taking a very long time? Or mostly less than a second?

@kfindeisen
Copy link
Member Author

kfindeisen commented Jan 23, 2020

Looks like I slightly overestimated the time for makeUniqueButler, but it is enough to add up across multiple tests:

6.93s setup    tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumCorruptedDataId
4.50s setup    tests/test_pipelineTaskTests.py::ButlerUtilsTestSuite::testButlerDimensions
1.11s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumPatchWithRun
0.87s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumVisitWithRun
0.85s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumInvalidDimension
0.69s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumPatchMockRun
0.61s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testValidateOutputConnectionsSingle
0.58s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumNoSuchDatatype
0.57s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumMissingMultiple
0.56s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumExtraMultiple

While I'm at it, here are the finer-grained timings:

Total: 4.0734123570000005
Butler.makeRepo: 3.4911327360000004 (0.857053602736935)
Butler: 0.20473458100000003 (0.05026119701536517)
_makeRecords: 0.0005014780000000001 (0.00012311005026000612)
insertDimensionData: 0.377043562 (0.09256209019743983)
Total: 6.239485837
Butler.makeRepo: 5.2321940940000005 (0.8385617390095216)
Butler: 0.313362996 (0.050222567081050974)
_makeRecords: 0.0005615160000000001 (8.99939537758422e-05)
insertDimensionData: 0.6933672310000001 (0.11112569995565166)

@timj
Copy link
Member

timj commented Jan 23, 2020

Well, I checked out the branch and ran the tests myself and I see:

0.15s setup    tests/test_pipelineTaskTests.py::ButlerUtilsTestSuite::testButlerDimensions
0.14s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumPatchWithRun
0.14s setup    tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumCorruptedDataId
0.13s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testMakeQuantumInvalidDimension
0.12s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumVisitMockRun
0.11s call     tests/test_pipelineTaskTests.py::ButlerUtilsTestSuite::testButlerDimensions
0.10s call     tests/test_pipelineTaskTests.py::PipelineTaskTestSuite::testRunQuantumVisitWithRun
0.10s call     tests/test_pipelineTaskTests.py::ButlerUtilsTestSuite::testExpandUniqueId
0.10s call     tests/test_pipelineTaskTests.py::ButlerUtilsTestSuite::testUniqueButler

so your machine is really really slow for some reason.

@kfindeisen
Copy link
Member Author

Well, I know that my computer runs tests faster than Jenkins (at the level of individual packages), so I think there's still room for concern.

@timj
Copy link
Member

timj commented Jan 23, 2020

Are you using a local SSD or an NFS mount?

@timj
Copy link
Member

timj commented Jan 23, 2020

I do have a comment on the code itself. I really think that the test code for creating butlers and dataset types should be moved to daf_butler. daf_butler already has some of these functions in the helper packages inside daf_butler tests directory but it seems that they need to be consolidated with your code here and moved to lsst.daf.butler.tests. pipe_base should only have support code relating to testing pipelines not testing butlers.

@kfindeisen
Copy link
Member Author

A suggestion from @timj on Slack (possibly redundant with the merge proposed above): use an in-memory SQLite database for the registry to speed up the Butler operations.

@timj
Copy link
Member

timj commented Jan 23, 2020

Since the config is not being specified in your API I think you can create your own Config to pass to makeRepo.

c = lsst.daf.butler.Config()
c["registry", "db"] = "sqlite:///:memory:"
Butler.makeRepo(root, config=c)

@TallJimbo
Copy link
Member

I can confirm that using an in-memory SQLite database can make things go tremendously faster. I discovered on a recent ticket that one of our Registry tests does an absurdly large number of inserts (playing with spatial indexing on regions covering ~half the sphere), and it was totally fine until I tried running that test against on-disk databases.

@kfindeisen
Copy link
Member Author

kfindeisen commented Jan 23, 2020

I really think that the test code for creating butlers and dataset types should be moved to daf_butler. daf_butler already has some of these functions in the helper packages inside daf_butler tests directory but it seems that they need to be consolidated with your code here and moved to lsst.daf.butler.tests.

Given the scope of this change, I'm closing this PR and will open a new one once the pipe_base code depends on daf.butler.tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants