Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-33600

Inconsistencies in queryDimensionRecords

    XMLWordPrintable

Details

    • 5
    • Data Access and Database
    • No

    Description

      rowen points out some inconsistencies in the use of queryDimensionRecords.

      TLDR:

      • Inconsistency between using kwargs for missing instruments vs where clause with missing instrument.
      • Bind parameter not working for governor dimensions.

      Using the pipelines_check repo since I have it lying around and it has one instrument (HSC):

      from lsst.daf.butler import Butler
       
      b = Butler("DATA_REPO")
      registry = b.registry
      where = ""
      bind = {}
      for instrument in ("HSC", "LATISS"):
       
          record_iter = b.registry.queryDimensionRecords(
              "exposure",
              instrument=instrument,
              bind=bind,
              where=where,
          )
          print(f"Got {record_iter.count()} results")
      

      This fails on the second loop with:

      Got 1 results
      Traceback (most recent call last):
        File "x.py", line 9, in <module>
          record_iter = b.registry.queryDimensionRecords(
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registries/sql.py", line 1102, in queryDimensionRecords
          dataIds = self.queryDataIds(
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registries/sql.py", line 1026, in queryDataIds
          standardizedDataId = self.expandDataId(dataId, **kwargs)
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registries/sql.py", line 718, in expandDataId
          raise LookupError(
      LookupError: Could not fetch record for required dimension instrument via keys {'instrument': 'LATISS'}.
      

      if though instead of using kwargs I specify the instrument in the where clause it all works fine:

      from lsst.daf.butler import Butler
       
      b = Butler("DATA_REPO")
      registry = b.registry
      where = ""
      bind = {}
      for instrument in ("HSC", "LATISS"):
       
          record_iter = b.registry.queryDimensionRecords(
              "exposure",
              # instrument=instrument,
              bind=bind,
              where=f"instrument = '{instrument}'",
          )
          print(f"Got {record_iter.count()} results")
      

      Result:

      Got 1 results
      Got 0 results
      

      This inconsistency is not great.

      Furthermore, if we then say that we will commit to the WHERE clause and use bind I can't get it to work at all:

      from lsst.daf.butler import Butler
       
      b = Butler("DATA_REPO")
      registry = b.registry
      for instrument in ("HSC", "LATISS"):
       
          record_iter = b.registry.queryDimensionRecords(
              "exposure",
              # instrument=instrument,
              bind={"inst": instrument},
              where="instrument = inst",
          )
          print(f"Got {record_iter.count()} results")
      

      which gives:

      Traceback (most recent call last):
        File "x.py", line 15, in <module>
          print(f"Got {record_iter.count()} results")
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registry/queries/_results.py", line 1102, in count
          return self._dataIds.count(exact=exact)
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registry/queries/_results.py", line 450, in count
          return self._query.count(self._db, exact=exact)
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registry/queries/_results.py", line 201, in _query
          self._cached_query = self._query_factory(self._order_by, self._limit)
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registries/sql.py", line 1056, in query_factory
          summary = queries.QuerySummary(
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registry/queries/_structs.py", line 395, in __init__
          self.where = expression.attach(
        File "/Users/timj/work/lsstsw3/build/daf_butler/python/lsst/daf/butler/registry/queries/_structs.py", line 161, in attach
          raise RuntimeError(msg) from None
      RuntimeError: Error in query expression "instrument = inst": No value(s) for governor dimensions {instrument} in expression that references dependent dimensions. 'Governor' dimensions must always be specified completely in either the query expression (via simple 'name=<value>' terms, not 'IN' terms) or in a data ID passed to the query method.
      

      implying that governor dimensions are checked before the bind parameters are handled? Other bind parameters seem to work fine.

      Attachments

        Issue Links

          Activity

            No builds found.
            tjenness Tim Jenness created issue -
            tjenness Tim Jenness added a comment -

            salnikov could you do a quick analysis of the bind parameter problem? Am I doing something wrong? Is it an easy fix if I'm doing it right? This seems to be a fairly serious limitation of bind when we are trying to encourage people to use bind.

            tjenness Tim Jenness added a comment - salnikov could you do a quick analysis of the bind parameter problem? Am I doing something wrong? Is it an easy fix if I'm doing it right? This seems to be a fairly serious limitation of bind when we are trying to encourage people to use bind.
            tjenness Tim Jenness made changes -
            Field Original Value New Value
            Link This issue is triggering DM-33601 [ DM-33601 ]
            salnikov Andy Salnikov made changes -
            Attachment image-2022-02-08-13-54-28-136.png [ 56594 ]
            salnikov Andy Salnikov made changes -
            Attachment image-2022-02-08-13-54-28-136.png [ 56594 ]

            I had a quick look at the code and it looks like CheckVisitor class does not handle bind parameters entirely correctly. We probably need some unit tests for it, I'm busy today but can work on it tomorrow if you assign the ticket to me.

            salnikov Andy Salnikov added a comment - I had a quick look at the code and it looks like CheckVisitor class does not handle bind parameters entirely correctly. We probably need some unit tests for it, I'm busy today but can work on it tomorrow if you assign the ticket to me.
            rowen Russell Owen made changes -
            Link This issue relates to DM-33601 [ DM-33601 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-33601 [ DM-33601 ]
            tjenness Tim Jenness made changes -
            Assignee Andy Salnikov [ salnikov ]
            tjenness Tim Jenness made changes -
            Team Architecture [ 10304 ] Data Access and Database [ 10204 ]

            Bind is an easy fix, but I'm not sure what to do about kwargs vs. query issue, do we want to make them consistent? It may make sense to raise an exception when a query specifies non-existing dimension, but I'm not sure how hard is it to implement.

            salnikov Andy Salnikov added a comment - Bind is an easy fix, but I'm not sure what to do about kwargs vs. query issue, do we want to make them consistent? It may make sense to raise an exception when a query specifies non-existing dimension, but I'm not sure how hard is it to implement.
            salnikov Andy Salnikov made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            tjenness Tim Jenness added a comment -

            It's a tricky one because the exception being raised is a way of telling you that your query is fundamentally never going to return anything. On the other hand not returning anything is also a legitimate answer.

            tjenness Tim Jenness added a comment - It's a tricky one because the exception being raised is a way of telling you that your query is fundamentally never going to return anything. On the other hand not returning anything is also a legitimate answer.
            tjenness Tim Jenness added a comment -

            jbosch do you have an opinion?

            tjenness Tim Jenness added a comment - jbosch do you have an opinion?
            jbosch Jim Bosch added a comment -

            I'm torn.  I think Russell is right that it's a slightly nicer interface for the data ID and kwargs inputs to be treated consistently with the where clause.  But doing that consistently across all of the query methods is at least a fair bit of work, and I'm worried that it might break some existing code that relies on the current behavior (since that's also not unreasonable behavior in isolation), and if that's the case, where we've drawn the line right now between failures an empty results may still be the best place.  We also need to make sure the returned proxy object knows why it's empty, so we don't lose the explanation, but I don't think that should be hard.

            So, I guess I think we give this a try - not just for queryDimensionRecords, but for queryDatasets and queryDataIds, too (the other query methods don't take data IDs or kwargs), but we reconsider if we run into downstream-breakage trouble in CI.

            jbosch Jim Bosch added a comment - I'm torn.  I think Russell is right that it's a slightly nicer interface for the data ID and kwargs inputs to be treated consistently with the where clause.  But doing that consistently across all of the query methods is at least a fair bit of work, and I'm worried that it might break some existing code that relies on the current behavior (since that's also not unreasonable behavior in isolation), and if that's the case, where we've drawn the line right now between failures an empty results may still be the best place.  We also need to make sure the returned proxy object knows why it's empty, so we don't lose the explanation, but I don't think that should be hard. So, I guess I think we give this a try - not just for queryDimensionRecords, but for queryDatasets and queryDataIds, too (the other query methods don't take data IDs or kwargs), but we reconsider if we run into downstream-breakage trouble in CI.

            jbosch, just to make sure I understand your proposal correctly - the methods that take kwargs/dataId should return empty result instead of raising? There is also findDataset() that takes kwargs/dataId, I suppose that should still raise instead of returning None?

            salnikov Andy Salnikov added a comment - jbosch , just to make sure I understand your proposal correctly - the methods that take kwargs/dataId should return empty result instead of raising? There is also findDataset() that takes kwargs/dataId, I suppose that should still raise instead of returning None?
            tjenness Tim Jenness added a comment -

            findDataset is documented to return None if none could be found except for all the myriad ways in which it would be impossible for a dataset to be found in which case raise. That's probably okay because you aren't using a WHERE clause anywhere to introduce inconsistent behavior.

            tjenness Tim Jenness added a comment - findDataset is documented to return None if none could be found except for all the myriad ways in which it would be impossible for a dataset to be found in which case raise. That's probably okay because you aren't using a WHERE clause anywhere to introduce inconsistent behavior.
            jbosch Jim Bosch added a comment -

            Yeah, I think we only want to treat the query* methods that take kwargs/dataId this way; anything else that takes kwargs/dataId should continue to raise.

            jbosch Jim Bosch added a comment - Yeah, I think we only want to treat the query* methods that take kwargs/dataId this way; anything else that takes kwargs/dataId should continue to raise.
            rowen Russell Owen added a comment - - edited

            From my perspective it is crucial that my own code can reliably determine that the query was valid, but returned no matches vs. the query was not valid. What I mean by "valid" is that all the fields referenced exist, the data provided for those fields is in the correct format, the query logically makes sense, etc.

            If you want to raise an exception, then in my opinion it is essential that it be specialized enough that it clearly means "this query returned no records but was otherwise valid". Is that the situation? I have no idea what range of reasons can trigger a LookupError, because the doc string for this method does not document the exceptions it can raise. Naively I would think LookupError could easily mean "invalid query", for example specifying a field in the where clause that does not exist. If so, then I think LookupError is not a good exception to raise in this situation.

            Please add Raises documentation to all your query methods.

            I will say that I am trying to use Registry as a (read-only) database. So from my perspective returning no records is the most sensible thing to do. But as I said above, I can handle an exception being raised as long as it reliably means "valid query, but no records found".

            rowen Russell Owen added a comment - - edited From my perspective it is crucial that my own code can reliably determine that the query was valid, but returned no matches vs. the query was not valid. What I mean by "valid" is that all the fields referenced exist, the data provided for those fields is in the correct format, the query logically makes sense, etc. If you want to raise an exception, then in my opinion it is essential that it be specialized enough that it clearly means "this query returned no records but was otherwise valid". Is that the situation? I have no idea what range of reasons can trigger a LookupError, because the doc string for this method does not document the exceptions it can raise. Naively I would think LookupError could easily mean "invalid query", for example specifying a field in the where clause that does not exist. If so, then I think LookupError is not a good exception to raise in this situation. Please add Raises documentation to all your query methods. I will say that I am trying to use Registry as a (read-only) database. So from my perspective returning no records is the most sensible thing to do. But as I said above, I can handle an exception being raised as long as it reliably means "valid query, but no records found".

            jbosch, one thing that bothered me for a while - if we want clients to use explain_no_results when result set is empty (and other methods like any(), order_by()), should we then document and annotate Registry query methods to return QueryResult instances instead of current Iterables? Maybe add/move abstract QueryResult classes to interfaces?

            salnikov Andy Salnikov added a comment - jbosch , one thing that bothered me for a while - if we want clients to use explain_no_results when result set is empty (and other methods like any(), order_by()), should we then document and annotate Registry query methods to return QueryResult instances instead of current Iterables? Maybe add/move abstract QueryResult classes to interfaces ?
            jbosch Jim Bosch added a comment -

            should we then document and annotate Registry query methods to return QueryResult instances instead of current Iterables

            Yes, we should - they used to be documented and annotated that way, and we switched to Iterable when we split SqlRegistry and RemoteRegistry out into subclasses, because the QueryResults classes are not usable with RemoteRegistry.  But RemoteRegistry is not at all usable right now in other respects, so we shouldn't be making that kind of concession to it yet.  Replacing the current QueryResults classes with things that could be used with RemoteRegistry is part of what I want to do on DM-31725, but for now I think we should make the Registry annotations match SqlRegistry's and update the docs accordingly, and just add some "type: ignore" to RemoteRegistry.

            Maybe add/move abstract QueryResult classes to interfaces?

            Good idea, and feel free to do it if it helps solve any problems.  But I'll do something similar on DM-31725, so I don't think this needs to be priority in its own right.

            jbosch Jim Bosch added a comment - should we then document and annotate Registry query methods to return QueryResult instances instead of current Iterables Yes, we should - they used to be documented and annotated that way, and we switched to Iterable when we split SqlRegistry and RemoteRegistry out into subclasses, because the QueryResults classes are not usable with RemoteRegistry.  But RemoteRegistry is not at all usable right now in other respects, so we shouldn't be making that kind of concession to it yet.  Replacing the current QueryResults classes with things that could be used with RemoteRegistry is part of what I want to do on DM-31725 , but for now I think we should make the Registry annotations match SqlRegistry's and update the docs accordingly, and just add some "type: ignore" to RemoteRegistry. Maybe add/move abstract QueryResult classes to interfaces ? Good idea, and feel free to do it if it helps solve any problems.  But I'll do something similar on DM-31725 , so I don't think this needs to be priority in its own right.

            OK, I have made few improvements, but I'm stuck trying to figure out how it is possible to further improve what is returned by explain_no_results for "incorrect" user queries, so maybe I should stop here.

            Here is what's done so far on this ticket:

            • Trivial fix for bind parameters, it should recognize now that governor is specified via bind rather than by literal in a user expression.
            • User expression is analyzed and value of instrument and skymap is checked against known values, if the value is unknown then an exception is raised. This behavior matches what happens when an unknown value is passed via keyword arguments, I think this is a reasonable approach. Only the governor dimensions are checked this way, for other dimensions I think it makes less sense doing that as expressions for other dimensions can be expected to have missing values (e.g. "visit IN (1..100)"). This also means that if someone says "detector.full_name = 'foo'" then no exception will be raised, an empty result will be returned instead.
            • DimensionRecordQueryResults class (returned from queryDimensionRecords) adds the explain_no_results method which will in some cases produces a list of reasons for why the result is empty. As for other result classes, this method can detect some reasons, but notably it cannot analyze user expression and tell that something is wrong there. If, for example, one gives the same expression "detector.full_name = 'foo'", then explain_no_results will return an empty list.

            I tried to understand how it's possible to improve explain_no_results to handle user expression better, but I believe it's a very hard problem in general for arbitrary expressions (and I think complex expressions is probably what will give people most troubles). Further complication is that in our current implementation explain_no_results does not have enough information about user expression to try to do anything useful. If we need to improve that I think we'll need significant changes in Query/QueryBuilder, and I'm not super comfortable with that code yet. I would suggest to stop at this point, merge above changes and maybe dedicate a separate future ticket to explain_no_results improvements.

            salnikov Andy Salnikov added a comment - OK, I have made few improvements, but I'm stuck trying to figure out how it is possible to further improve what is returned by explain_no_results for "incorrect" user queries, so maybe I should stop here. Here is what's done so far on this ticket: Trivial fix for bind parameters, it should recognize now that governor is specified via bind rather than by literal in a user expression. User expression is analyzed and value of instrument and skymap is checked against known values, if the value is unknown then an exception is raised. This behavior matches what happens when an unknown value is passed via keyword arguments, I think this is a reasonable approach. Only the governor dimensions are checked this way, for other dimensions I think it makes less sense doing that as expressions for other dimensions can be expected to have missing values (e.g. "visit IN (1..100)"). This also means that if someone says "detector.full_name = 'foo'" then no exception will be raised, an empty result will be returned instead. DimensionRecordQueryResults class (returned from queryDimensionRecords ) adds the explain_no_results method which will in some cases produces a list of reasons for why the result is empty. As for other result classes, this method can detect some reasons, but notably it cannot analyze user expression and tell that something is wrong there. If, for example, one gives the same expression "detector.full_name = 'foo'" , then explain_no_results will return an empty list. I tried to understand how it's possible to improve explain_no_results to handle user expression better, but I believe it's a very hard problem in general for arbitrary expressions (and I think complex expressions is probably what will give people most troubles). Further complication is that in our current implementation explain_no_results does not have enough information about user expression to try to do anything useful. If we need to improve that I think we'll need significant changes in Query/QueryBuilder, and I'm not super comfortable with that code yet. I would suggest to stop at this point, merge above changes and maybe dedicate a separate future ticket to explain_no_results improvements.
            jbosch Jim Bosch added a comment -

            That all sounds reasonable to me, and the limitations to explain_no_results are entirely expected.  It is definitely a very hard problem in general, and the battery of checks that are run already are pretty good at catching the most common mistakes, at least in the context of QuantumGraph generation.  I think the only sensible way to improve on it is to be guided by what problems users tend to see empirically.

            It sounds like this resolution might still leave rowen somewhat unhappy, because we do still have a condition (bad governor dimension values) in which we raise instead of returning no results.  We would be consistent about that in a sense - governor dimension problems raise exceptions, other dimension problems don't - but for users who don't understand that these dimensions are special, the behavior will still feel inconsistent.  So we need to at least document the behavior with governor dimensions clearly.

            jbosch Jim Bosch added a comment - That all sounds reasonable to me, and the limitations to explain_no_results are entirely expected.  It is definitely a very hard problem in general, and the battery of checks that are run already are pretty good at catching the most common mistakes, at least in the context of QuantumGraph generation.  I think the only sensible way to improve on it is to be guided by what problems users tend to see empirically. It sounds like this resolution might still leave rowen somewhat unhappy, because we do still have a condition (bad governor dimension values) in which we raise instead of returning no results.  We would be consistent about that in a sense - governor dimension problems raise exceptions, other dimension problems don't - but for users who don't understand that these dimensions are special, the behavior will still feel inconsistent.  So we need to at least document the behavior with governor dimensions clearly.

            I thought that raising an exception would be a more reliable way to report issues that we can detect upfront. As an alternative, I could convert all those exceptions into an empty result with explain_no_results, but reporting in that case will depend on a client calling that method and printing the messages.

            salnikov Andy Salnikov added a comment - I thought that raising an exception would be a more reliable way to report issues that we can detect upfront. As an alternative, I could convert all those exceptions into an empty result with explain_no_results , but reporting in that case will depend on a client calling that method and printing the messages.
            tjenness Tim Jenness added a comment -

            rowen was noting that he couldn't reliably know whether he was going to get an exception or not because it depended on which bit of the system worked out the problem. It sounds like we are saying that kwargs always has a chance of raising but where+bind won't. Maybe that's fine. Having it return no results but with some explanation in some cases is better than we have now and I think rowen is okay with using bind+where once the bind problem is fixed.

            tjenness Tim Jenness added a comment - rowen was noting that he couldn't reliably know whether he was going to get an exception or not because it depended on which bit of the system worked out the problem. It sounds like we are saying that kwargs always has a chance of raising but where+bind won't. Maybe that's fine. Having it return no results but with some explanation in some cases is better than we have now and I think rowen is okay with using bind+where once the bind problem is fixed.

            With these new updates, where+bind (or no bind) will raise if a governor value in the expression is unknown, that will be consistent with kwargs. In general, I think there is always a chance that methods can raise an exception for an incorrect combination of arguments, but they also can return an empty result, the code should be prepared to handle both cases.

            salnikov Andy Salnikov added a comment - With these new updates, where+bind (or no bind) will raise if a governor value in the expression is unknown, that will be consistent with kwargs. In general, I think there is always a chance that methods can raise an exception for an incorrect combination of arguments, but they also can return an empty result, the code should be prepared to handle both cases.
            rowen Russell Owen added a comment - - edited

            I will be content if and only if the exception raised for an unknown governer value both predicable and different than the exception raised for invalid queries – invalid in the database sense. My service does not want to know or care about your special "governer" dimensions. It wants to treat registry as much like a database server as it possibly can. Catching a special "this is a valid query and if this was a database this call would be returning no records" exception is certainly tolerable.

            It almost sounds like Registry is the wrong API to be using, but I don't know of any alternative.

            rowen Russell Owen added a comment - - edited I will be content if and only if the exception raised for an unknown governer value both predicable and different than the exception raised for invalid queries – invalid in the database sense. My service does not want to know or care about your special "governer" dimensions. It wants to treat registry as much like a database server as it possibly can. Catching a special "this is a valid query and if this was a database this call would be returning no records" exception is certainly tolerable. It almost sounds like Registry is the wrong API to be using, but I don't know of any alternative.

            I can agree that the current set of exceptions that we raise in those query methods (and many other methods) is hard to use to identify specific issues. What we have today is a mix of LookupError, KeyError, RuntimeError, ValueError and its subclasses; to make exceptions more useful we probably need more specific and more structured class hierarchy for exceptions. It's a non-trivial problem, of course, to try to update existing code to generate different exceptions, but I can try something reasonably simple on this ticket.

            From the client perspective, I think it's probably worth to try to distinguish following cases for these three query methods:

            • Unknown keyword argument name (e.g. querySomething(..., not_a_thing="thing")), today this raises KeyError, maybe worth defining something DimensionNameError? (Same for the key in dataId argument).
            • Unexpected value for a dimension passed in dataId or keyword arguments, LookupError is raised today, maybe raise new exception InvalidDataIdError?
            • For inconsistent/conflicting coordinate values we already raise InconsistentDataIdError, which inherits ValueError, maybe make a new base class for it common with InvalidDataIdError?
            • Bad syntax in user expression today raises RuntimeError, replace that with more specific UserExpressionSyntaxError?
            • Valid user expression syntax, but containing unknown governor dimension values (e.g. "instrument = 'LSTS'"), with the changes on this ticket raises LookupError, could replace it with the UserExpressionInvalidDataIdError (maybe a subclass of InvalidDataIdError)?
            • In other cases methods can return empty result, and explain_no_results() can be used to try to identify the issue

            This may not cover all possible errors though, other types of exceptions may leak from the code we have no control over, e.g. database connection may break at any moment and produce some SQLAlchemy errors. We could catch them all at registry boundary and re-throw as a generic exception type (RegistryError), or we can leave that to the caller. I believe general good practice in the cases like this is to catch and wrap/re-throw all exceptions at the module/library boundary and not let undocumented exceptions leak.

            salnikov Andy Salnikov added a comment - I can agree that the current set of exceptions that we raise in those query methods (and many other methods) is hard to use to identify specific issues. What we have today is a mix of LookupError, KeyError, RuntimeError, ValueError and its subclasses; to make exceptions more useful we probably need more specific and more structured class hierarchy for exceptions. It's a non-trivial problem, of course, to try to update existing code to generate different exceptions, but I can try something reasonably simple on this ticket. From the client perspective, I think it's probably worth to try to distinguish following cases for these three query methods: Unknown keyword argument name (e.g. querySomething(..., not_a_thing="thing") ), today this raises KeyError, maybe worth defining something DimensionNameError? (Same for the key in dataId argument). Unexpected value for a dimension passed in dataId or keyword arguments, LookupError is raised today, maybe raise new exception InvalidDataIdError? For inconsistent/conflicting coordinate values we already raise InconsistentDataIdError, which inherits ValueError, maybe make a new base class for it common with InvalidDataIdError? Bad syntax in user expression today raises RuntimeError, replace that with more specific UserExpressionSyntaxError? Valid user expression syntax, but containing unknown governor dimension values (e.g. "instrument = 'LSTS'"), with the changes on this ticket raises LookupError, could replace it with the UserExpressionInvalidDataIdError (maybe a subclass of InvalidDataIdError)? In other cases methods can return empty result, and explain_no_results() can be used to try to identify the issue This may not cover all possible errors though, other types of exceptions may leak from the code we have no control over, e.g. database connection may break at any moment and produce some SQLAlchemy errors. We could catch them all at registry boundary and re-throw as a generic exception type (RegistryError), or we can leave that to the caller. I believe general good practice in the cases like this is to catch and wrap/re-throw all exceptions at the module/library boundary and not let undocumented exceptions leak.

            jbosch, I think this is ready for review. We discussed exceptions yesterday, I pushed everything that I have so far in the last commit on this branch. I'm not very content with that myself, mostly because it's hard to test that there are no other exceptions leaking, and of course unit tests don't cover all exception-raising cases. As I said, I'm open to dropping that last commit if you think this is not a big improvement.

            rowen, could you look at the new docstrings for Registry methods to check that it works for you.

            I have not added news snippet yet, if new exceptions survive the review, I'll add something.

            daf_butler PR: https://github.com/lsst/daf_butler/pull/651
            pipe_base PR: https://github.com/lsst/pipe_base/pull/236
            obs_base PR: https://github.com/lsst/obs_base/pull/409

            salnikov Andy Salnikov added a comment - jbosch , I think this is ready for review. We discussed exceptions yesterday, I pushed everything that I have so far in the last commit on this branch. I'm not very content with that myself, mostly because it's hard to test that there are no other exceptions leaking, and of course unit tests don't cover all exception-raising cases. As I said, I'm open to dropping that last commit if you think this is not a big improvement. rowen , could you look at the new docstrings for Registry methods to check that it works for you. I have not added news snippet yet, if new exceptions survive the review, I'll add something. daf_butler PR: https://github.com/lsst/daf_butler/pull/651 pipe_base PR: https://github.com/lsst/pipe_base/pull/236 obs_base PR: https://github.com/lsst/obs_base/pull/409
            salnikov Andy Salnikov made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            jbosch Jim Bosch added a comment -

            Looks good to me; just one comment on a single new exception type and its usage on the daf_butler PR.

            jbosch Jim Bosch added a comment - Looks good to me; just one comment on a single new exception type and its usage on the daf_butler PR.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            salnikov Andy Salnikov made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

            Thanks for review and fixing docs! Merged.

            salnikov Andy Salnikov added a comment - Thanks for review and fixing docs! Merged.
            salnikov Andy Salnikov made changes -
            Link This issue is triggering DM-33826 [ DM-33826 ]
            salnikov Andy Salnikov made changes -
            Link This issue is triggering DM-33890 [ DM-33890 ]
            tjenness Tim Jenness made changes -
            Story Points 5

            People

              salnikov Andy Salnikov
              tjenness Tim Jenness
              Jim Bosch
              Andy Salnikov, Jim Bosch, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.