DM-34017: Use the butler API and mock data for run_pipeline tests #19

ebellm · 2022-03-21T23:52:29Z

No description provided.

python/activator/middleware_interface.py

kfindeisen

Looks good, though the current collection handling will cause problems in integration. I also agree with both of K-T's points.

python/activator/middleware_interface.py

tests/test_middleware_interface.py

python/activator/middleware_interface.py

ups/prompt_prototype.table

parejkoj · 2022-03-24T20:16:48Z

python/activator/middleware_interface.py

-        result = pipeline.run(register_dataset_types=False)
+        # If this is a fresh (local) repo, then types like calexp,
+        # *Diff_diaSrcTable, etc. have not been registered.
+        result = pipeline.run(register_dataset_types=True)


Can we do that type registration in prep_butler, possibly as part of _prep_collections?

That would require a list of every connection used by every task in the pipeline (or a traversal of the pipeline itself, which is what I assume the executor does).

Doesn't the QuantumGraph know that, though?

The skip is harder to miss than a comment in the test code, so that we can revisit it once the missing MiddlewareInterface functionality is added.

tests/test_middleware_interface.py

kfindeisen · 2022-03-25T00:22:36Z

python/activator/middleware_interface.py

    def __init__(self, central_butler: Butler, image_bucket: str, instrument: str,
                 butler: Butler,
-                 prefix: str = "gs://"):
+                 prefix: str = "s3://"):


This change is no longer necessary after the last weekly, ~~and I've removed S3 support from the Cloud service~~ [restored]. (Sorry for not realizing this branch was using it!)

* remove PIPELINE_MAP * Move code out of init and _prep_butler into _prep_pipeline

parejkoj · 2022-03-28T08:05:59Z

@kfindeisen : please have another (final?) look. I'll try to combine commits somewhat post-review

kfindeisen

Looks good, but please make sure the tests pass (the flake8 error should have appeared locally), and please avoid special-casing the unit tests. There's not much point to having them if they're free to make different assumptions from the prototype environment.

kfindeisen · 2022-03-28T17:49:59Z

python/activator/middleware_interface.py


        # Refresh butler after configuring it, to ensure all required
-        # dimensions are available.
+        # collections are available.


This one really should be "dimensions"; my earlier correction applied only to the comment in _prep_collections.

As it stands, init_butler has nothing to do, since we have to put off all the work for prep_butler right now. We can leave it around while we wait for middleware fixes, etc. that will let us do more things here, but I've added a note to the method docs about whether it's worth keeping the method.

My above comment is now out of date, since Jim gave us a fix for the instrument.register problem, so _init_butler now has things to do.

python/activator/middleware_interface.py

tests/test_middleware_interface.py

kfindeisen · 2022-03-28T18:27:58Z

tests/test_middleware_interface.py

-            mock_executor.from_pipeline.assert_called_once_with(
-                self.interface.pipeline,
-                where=f"detector={self.next_visit.detector} and exposure in (1)",
-                butler=self.interface.butler
-            )


I think this is a useful test (with maybe some more use of mock.ANY), since the restriction to a specific pipeline and data ID is a key part of this method's purpose. It seems a shame to remove it.

I'm actually building the pipeline now, though: that's how I discovered some of the bugs (like DM-34202). The only thing being mocked is run.

Ok, but does that tell you that you built the right executor, or just any executor? The where clause seems like it would be hard to test by any means other than direct introspection.

Based on the work we did today, I believe we're now doing a good job testing that where.

python/activator/middleware_interface.py

fixup run_pipeline test

Thanks to Jim Bosch for the `elements=[]` fix for this, allowing us to register the instrument on init. However, the skymap has a similar problem that is harder to work around.

fixup local appipe

We need valid metadata for defineVisits.

parejkoj · 2022-03-30T22:30:09Z

On the team-coding call, we all agreed on not trying to clean up the git history on this across the Eric->John handoff. I cleaned up my own commits as best I could.

ebellm requested a review from kfindeisen March 21, 2022 23:52

ebellm force-pushed the tickets/DM-34017 branch from 3372548 to d219100 Compare March 23, 2022 00:02

ktlim reviewed Mar 23, 2022

View reviewed changes

python/activator/middleware_interface.py Outdated Show resolved Hide resolved

ktlim reviewed Mar 23, 2022

View reviewed changes

python/activator/middleware_interface.py Outdated Show resolved Hide resolved

kfindeisen approved these changes Mar 23, 2022

View reviewed changes

parejkoj reviewed Mar 24, 2022

View reviewed changes

parejkoj and others added 13 commits March 24, 2022 15:12

Begin defining collections to run on

f009159

Use SimplePipelineExecutor to run a pipeline

d4ff29b

resolve test failures

dabb74a

make raw ingest collection

c44ba8e

use ApPipe.yaml appropriate for specific cameras

acca09b

Clarify comments.

33c2683

Use mock executor in test_run_pipeline.

2bb6b17

Replace commented-out test with skip annotation.

b76d639

The skip is harder to miss than a comment in the test code, so that we can revisit it once the missing MiddlewareInterface functionality is added.

use env var for APDB; restore pipeline map

3d9bb92

mock env var IP_APDB

9c5a331

use kind=SURVEY

a19fa6a

add defaults collection

ecefb30

fix test case for PIPELINE_MAP call signature

e9da706

kfindeisen reviewed Mar 24, 2022

View reviewed changes

tests/test_middleware_interface.py Show resolved Hide resolved

kfindeisen reviewed Mar 25, 2022

View reviewed changes

parejkoj added 2 commits March 27, 2022 20:58

Restore gs bucket prefix

977a2de

Add _prep_pipeline

e586461

* remove PIPELINE_MAP * Move code out of init and _prep_butler into _prep_pipeline

parejkoj force-pushed the tickets/DM-34017 branch from 501d704 to 99f4521 Compare March 28, 2022 08:05

kfindeisen approved these changes Mar 28, 2022

View reviewed changes

Fix DECam mock data flipX and rotator

96220a3

parejkoj force-pushed the tickets/DM-34017 branch from 99f4521 to 6515e78 Compare March 29, 2022 04:25

parejkoj added 2 commits March 30, 2022 15:22

Fix prep_collections, workaround for DM-34202

084116f

Working run_pipeline test

8075327

fixup run_pipeline test

parejkoj added 7 commits March 30, 2022 15:22

Fix pre-registration bug

d0ffc63

Thanks to Jim Bosch for the `elements=[]` fix for this, allowing us to register the instrument on init. However, the skymap has a similar problem that is harder to work around.

fix apdb setup

564bf5d

Fix prep_butler test

c0bafa8

Call defineVisits before creating the pipeline

3cd4137

Use local ApPipe.yaml file

a38c6fa

fixup local appipe

Fix fake data to use correct date and coords

ea4dbe6

We need valid metadata for defineVisits.

Add tests for failing run_pipeline calls

12d6db9

parejkoj force-pushed the tickets/DM-34017 branch from 6515e78 to 12d6db9 Compare March 30, 2022 22:29

parejkoj merged commit bc7a2d8 into main Mar 30, 2022

parejkoj deleted the tickets/DM-34017 branch March 30, 2022 22:39

DM-34017: Use the butler API and mock data for run_pipeline tests #19

DM-34017: Use the butler API and mock data for run_pipeline tests #19

Uh oh!

Conversation

ebellm commented Mar 21, 2022

Uh oh!

Uh oh!

Uh oh!

kfindeisen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfindeisen Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfindeisen Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parejkoj commented Mar 28, 2022

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parejkoj Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parejkoj commented Mar 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kfindeisen left a comment •

edited

Loading

kfindeisen Mar 24, 2022 •

edited

Loading

kfindeisen Mar 25, 2022 •

edited

Loading

parejkoj Mar 30, 2022 •

edited

Loading