DM-43479: Add ability to use where/bind clauses in LogBrowser #38
Conversation
laurenam
force-pushed
the
tickets/DM-43479
branch
from
ad71db6
to
5ab8128
Compare
| @@ -82,7 +88,7 @@ def _getDataRefs(self): | |||
| dataRefs : `list` [`lsst.daf.butler.core.datasets.ref.DatasetRef`] | |||
| """ | |||
| results = self.butler.registry.queryDatasets( | |||
| f"{self.taskName}_log", collections=self.collection, findFirst=True | |||
| f"{self.taskName}_log", collections=self.collection, findFirst=True, where=self.whereStr | |||
There was a problem hiding this comment.
I assume the default/null option here is an empty string and not None, right?
There was a problem hiding this comment.
Yup, it seems to be "" for where and None for bind:
https://github.com/lsst/daf_butler/blob/main/python/lsst/daf/butler/registry/_registry.py#L1103-L1116
| @@ -50,7 +55,7 @@ class LogBrowser: | |||
| animal. | |||
|
|
|||
| example usage: | |||
| logBrowser = LogBrowser(butler, taskName, collection) | |||
| logBrowser = LogBrowser(butler, taskName, collection, whereStr=whereStr) | |||
There was a problem hiding this comment.
If we're supporting where then I wonder if we should also support passing in a bind option too. Don't worry if you don't think it's useful/can't be bothered, and as this is summit_extras (and will hopefully be absorbed into pipetask report anyway) I don't think it matters much, but just checking, especially given how you're using .format() in the example.
There was a problem hiding this comment.
I wasn't aware of this binding option! Definitely worth adding, so I did (once I figured out what it was!) 😄
There was a problem hiding this comment.
There are a few reasons one should always use bind, I am told. There is a crazy foot-gun there though, where you mustn't use dimension names as the keys in the dict, which includes day_obs, but other than that, it's recommended.
There was a problem hiding this comment.
Coolio...I added a note about the avoiding foot-gun :)
laurenam
force-pushed
the
tickets/DM-43479
branch
3 times, most recently
from
cba9f5d
to
dc99f23
Compare
laurenam
changed the title
| @@ -64,13 +77,23 @@ class LogBrowser: | |||
| "with gufunc signature (n?,k),(k,m?)->(n?,m?)", | |||
| ] | |||
|
|
|||
| def __init__(self, butler, taskName, collection): | |||
| def __init__(self, butler, taskName, collection, whereStr="", bindMap=None): | |||
There was a problem hiding this comment.
Personally, I'd call bindMap just bind, but then I'd probably also call whereStr just where tbh. I think these should be familiar terms for people, and also results in the common where=where, bind=bind pattern.
| f"Key '{key}' in bindMap is not in the whereStr provided:\n" | ||
| f"{self.whereStr}\nso no binding will take effect." |
There was a problem hiding this comment.
Do you really want two newlines inside the single log message? That's unusual. I'm not even sure one is recommended tbh, though it's summit_extras, so I don't care much.
laurenam
force-pushed
the
tickets/DM-43479
branch
from
dc99f23
to
65aefa7
Compare
Loading in all logs associated with a given task in a given collection can take a very long time (i.e. depending on how many dataRefs exist for that task.) This adds the ability to add a "where" clause to the dataId search such that the log loading time can be greatly reduced. A "bind" mapping can (optionally) be used with the where clause. If a given key provided in the bind mapping does not appear in the where clause string, a warning will be logged (stating that no binding will take effect on that key).
laurenam
force-pushed
the
tickets/DM-43479
branch
from
65aefa7
to
f3104b7
Compare
No description provided.