DM-43712: Add configurable buffer for template and refcat preload #204

hsinfang · 2024-09-30T19:39:40Z

No description provided.

This should have been together with 674a4b1 Unlike the old DECam test dataset, the new test LSSTComCamSim dataset does not have crosstalk. So, use another dataset type to test. Also fix a time stamp missed in the previous commit.

kfindeisen

Many thanks for the well-organized changes! However, I'm a bit worried that they're eroding the (already shaky) coherence of MiddlewareInterface, specifically with the region state variable and the splitting of queries across multiple _filter_datasets calls. I tried to offer some specific suggestions, but I'd like to take another look at the final result.

kfindeisen · 2024-10-01T19:54:07Z

python/activator/middleware_interface.py

@@ -272,6 +272,8 @@ class MiddlewareInterface:
        Information about which pipelines to run on ``visit``'s raws.
    skymap: `str`
        Name of the skymap in the central repo for querying templates.
+    padding: `int`
+        Number of arcseconds to pad the refcat and template region in preloading.


I'm a bit uncomfortable having this be part of the MiddlewareInterface API, which is already bloated with too many fiddly details. I'm looking into refactoring this class into something more object-oriented, but in the meantime it might make more sense to read PRELOAD_PADDING in middleware_interface.py.

Pre-existing issue with skymap, but this should have an extra space before :.

kfindeisen · 2024-10-01T21:49:07Z

python/activator/middleware_interface.py

+                for dataset_type in graph.inputs_of("retrieveTemplate"):
+                    if dataset_type.endswith("Coadd"):
+                        template_types.add(dataset_type)
+            # For cases where the pipelines do not contain "retrieveTemplate"


I'd really like to avoid hardcoding assumptions about the pipeline (I think parameters:apdb_config is the only necessary evil). This is infringing on DM-40245 a little, but could you use PipelineGraph.iter_overall_inputs instead? If not, could you add a comment that this is temporary until DM-40245?

kfindeisen · 2024-10-01T21:53:20Z

python/activator/middleware_interface.py

+            # For cases where the pipelines do not contain "retrieveTemplate"
+            except KeyError:
+                pass
+        return list(template_types)


Why does it need to be a sequence? I would expect any collection to work, and a set seems the natural choice of the "standard" collection types. Does order matter?

tests/test_middleware_interface.py

kfindeisen · 2024-10-01T22:11:30Z

python/activator/middleware_interface.py

+        center = wcs.pixelToSky(detector.getCenter(lsst.afw.cameraGeom.PIXELS))
+        corners = wcs.pixelToSky(detector.getCorners(lsst.afw.cameraGeom.PIXELS))
+        padded = [c.offset(center.bearingTo(c), self.padding) for c in corners]
+        self.region = lsst.sphgeom.ConvexPolygon.convexHull([c.getVector() for c in padded])


I really don't like the thought of self.region being undefined before preload, and then possibly existing afterward; that requires a lot of redundant state checks throughout the code. If we only use it during preload, do we actually need to make it a member of self, or can we just pass it around between functions?

If it does need to be part of the object's state, I'd recommend the following changes:

Move the computation to __init__ time instead of prep_butler time

Move the actual assignment to self.region out of _compute_region (i.e., make the method have no side effects)

Guarantee that self.region is None (instead of undefined) if and only if _skip_spatial_preload

etc/export_comCamSim.yaml

kfindeisen · 2024-10-01T23:25:59Z

python/activator/middleware_interface.py

-        known_datasets = set()
+        except MissingDatasetTypeError as e:
+            _log.debug("Pre-export query with args '%s' failed with %s", formatted_args, e)
+            known_datasets = set()


"# If dataset type never registered locally, then any such datasets are missing."?

kfindeisen · 2024-10-01T23:27:52Z

python/activator/middleware_interface.py


    # Let exceptions from src_repo query raise: if it fails, that invalidates
    # this operation.
    # "expanded" dimension records are ignored for DataCoordinate equality
    # comparison, so we only need them on src_datasets.
    with lsst.utils.timer.time_this(_log, msg=f"_filter_datasets({formatted_args}) (source datasets)",
                                    level=logging.DEBUG):
-        src_datasets = set(src_repo.registry.queryDatasets(*args, **kwargs).expanded())
+        # Ok with empty query result here, not log an error, and let the downstream
+        # method decide what to do with empty results.


I don't understand this comment. _MissingDatasetError is raised on new line 1794.

More broadly, I'm worried that _filter_datasets has stopped being a useful abstraction, if you're having to duplicate iteration code outside the function. Something to look into in DM-46178?

kfindeisen · 2024-10-01T23:42:54Z

.github/workflows/build-service.yml

@@ -34,7 +34,7 @@ env:
  # relatively stable Pipelines containers that are needed to avoid issues with
  # the "latest" version; they would remain in this list until "latest" becomes
  # usable for all building and testing.
-  BASE_TAG_LIST: '["latest"]'
+  BASE_TAG_LIST: '["w_latest"]'


Is this for access to the new query API? I would think it makes more sense to update latest.

Indeed I added it when the latest was too old at the time (>a month ago), but it's not needed right now.

Thanks for the reminder that I should keep in mind of the option of updating latest in this case.

kfindeisen · 2024-10-01T23:54:36Z

python/activator/middleware_interface.py

        indexer = HtmIndexer(depth=7)
-        shard_ids, _ = indexer.getShardIds(center, radius+self.padding)
+        shard_ids, _ = indexer.getShardIds(center, radius)


With the new query system, can this be replaced with a query directly by region?

No with w_2024_37 when the API first became public and when I started working on this, but yes with w_2024_38 or newer now.

So, we can switch to use region query too. Thanks for motivating me to test again.

Are we bothered that the pipelines using refcats might not be using butler in getting the shards?

I'm not sure I understand. You mean a task might have an old-style loader and it might demand specific shards that weren't in the region?

You mean a task might have an old-style loader and it might demand specific shards that weren't in the region?

Honestly I don't know and have not looked enough to understand how the loader decides what shards to use. I thought it was via HtmIndexer like the current PP code. Also, for the few examples I tried manually, the two methods (HtmIndexer versus butler query) give identical results. I don't know if they always do.

I would expect all modern tasks would get them from the Butler, I think that was part of the Gen2->3 transition.

The optimal value likely depends on the instrument and the skymap choice.

Instead of determining the instrument's wcsFlipX here, it is more robust to use its formatter, which knows its camera orientation from its obs package.

This changes the APIs of _export_skymap_and_templates and _export_refcats to take a lsst.sphgeom.Region directly. Note that the centroid of the region is not the same of the detector center, but it should not matter. Because htm7 can be too coarse compared to the patch size, using htm7 indices to search for templates may lead to preloading more patches than necessary and wasting time. This feature of using htm7 to search for overlapping templates is also about to be deprecated and replaced by the arbitrary spatial region query in Butler. The usage will be replaced when switching to the new butler query system.

This is a preparation step before switching to the new butler query system. In the new query system, query_datasets takes one datasetType at a time, while butler.registry.queryDatasets can take a list of dataset types in the old system. So, we change to query (via _filter_datasets) one type of calibs/refcats/templates at a time. Currently we can preload more types of calibs/refcats/templates than the actual pipelines really need. It's possible that some types are not preloaded but it's okay. For now we allow _MissingDatasetError.

The new Butler query systems supports spatial-constraint query via lsst.sphgeom.Region directly. With this change, we use it in template and refcat search. This needs stack w_2024_38 or newer. make_export.py uses _filter_datasets so it needs to adjust to the new underlying API too.

Some unit tests were temporarily marked expectedFailure in 674a4b1. Now that we switch to the new query system, make them work again. The test repo was put together using middleware tools, which intrinsically uses butler repo's visit-detector regions with its padding from defineVisits config. That padding config is not the same as the preload region padding in prompt processing. This explains the patch differences in template selection.

Update unit tests with the test data change

30770f0

This should have been together with 674a4b1 Unlike the old DECam test dataset, the new test LSSTComCamSim dataset does not have crosstalk. So, use another dataset type to test. Also fix a time stamp missed in the previous commit.

kfindeisen requested changes Oct 2, 2024

View reviewed changes

hsinfang added 5 commits October 2, 2024 11:57

Fix a missing space

6e01665

Pad the detector region in the template search

0bb8cb6

Make the preload padding configurable at the service level

67bd7f1

The optimal value likely depends on the instrument and the skymap choice.

Use the instrument's formatter to get the sky wcs

898fa56

Instead of determining the instrument's wcsFlipX here, it is more robust to use its formatter, which knows its camera orientation from its obs package.

Use the actual template coadd types instead of wildcarding

2696a76

hsinfang force-pushed the tickets/DM-43712 branch 2 times, most recently from f8b6ae7 to 6839a4b Compare October 4, 2024 17:04

hsinfang added 6 commits October 4, 2024 10:05

Factor out the region computation

f97422c

Warn if preload region padding is smaller than defineVisits padding

eff9ca6

hsinfang force-pushed the tickets/DM-43712 branch from 6839a4b to 5653980 Compare October 4, 2024 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-43712: Add configurable buffer for template and refcat preload #204

DM-43712: Add configurable buffer for template and refcat preload #204

hsinfang commented Sep 30, 2024

kfindeisen left a comment •

edited

Loading

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

kfindeisen Oct 1, 2024

hsinfang Oct 2, 2024

kfindeisen Oct 1, 2024

hsinfang Oct 2, 2024

hsinfang Oct 2, 2024

kfindeisen Oct 2, 2024

hsinfang Oct 2, 2024 •

edited

Loading

kfindeisen Oct 2, 2024

DM-43712: Add configurable buffer for template and refcat preload #204

Are you sure you want to change the base?

DM-43712: Add configurable buffer for template and refcat preload #204

Conversation

hsinfang commented Sep 30, 2024

kfindeisen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsinfang Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen left a comment •

edited

Loading

hsinfang Oct 2, 2024 •

edited

Loading