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

Determine strategy for dealing with butler proxies in pybind11

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: butler
    • Labels:
      None
    • Story Points:
      6
    • Sprint:
      DRP S17-2, DRP S17-3
    • Team:
      Data Release Production

      Description

      The Butler uses daf.persistence.ReadProxy to support lazy reading, returning these objects in place of things like SourceCatalog and Exposure. In Python, these proxies behave almost entirely like their subjects, because they forward all attributes, but they can't be converted to their C++ counterparts through pybind11's type conversion system. This worked with Swig essentially due to luck: Swig stores its C++ instances in a this attribute that's publicly accessible from Python, so it automatically invokes the proxy's attribute forwarding when trying to do conversions.

      I think there are essentially three approaches to dealing with this in pybind11:

      • We could remove ReadProxy entirely. A few of us have long been annoyed with them, and we're not yet getting any use out of the lazy loading functionality. We do anticipate needing lazy loading someday, however, and it looks like an RFC to do this would be more contentious and more work (to design a replacement interface for lazy loading) than I originally anticipated.
      • We could live with the conversion failure, making Python code that calls wrapped C++ routines call ReadProxy.__subject__ directly (or via a new convenience function) and making the error message when conversions fail much more helpful (e.g. by changing ReadProxy.__repr__. I really don't have a good sense for how much Python we'd have to modify if we take this approach.
      • We could add a pybind11 type_caster that does the dereference for any types that could be proxied. If it's possible to add a new type_caster for a class that's already by wrapped with pybind11::class_, and to delegate within it to the original converters defined by pybind11::class_, this will be easy and the course we should take. I'm hoping Pim Schellart [X] knows if it's possible (my best guess is that it is, but it will require using some undocumented and possible internal parts of pybind11).

      This ticket is for exploratory work and making a decision (at least with the story points it has now). It should be updated or a new ticket spawned if we identify an approach that will require significant further work.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Some comments on the GitHub PR, but overall I like this quite a bit. Very good idea to add a base class for ReadProxy.

            Show
            jbosch Jim Bosch added a comment - Some comments on the GitHub PR, but overall I like this quite a bit. Very good idea to add a base class for ReadProxy .
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged after implementing PR comments.
            Registering proxy conversions for all relevant types will be done on a separate ticket.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged after implementing PR comments. Registering proxy conversions for all relevant types will be done on a separate ticket.
            Hide
            rhl Robert Lupton added a comment -

            Does this being closed mean that we can resolve

            python/lsst/pipe/tasks/mocks/mockCoadd.py:            # TODO: pybind11 remove `immediate=True` once DM-9112 is resolved
            python/lsst/pipe/tasks/transformMeasurement.py:        # TODO: pybind11 remove `immediate=True` once DM-9112 is resolved
            

            Show
            rhl Robert Lupton added a comment - Does this being closed mean that we can resolve python/lsst/pipe/tasks/mocks/mockCoadd.py: # TODO: pybind11 remove `immediate=True` once DM-9112 is resolved python/lsst/pipe/tasks/transformMeasurement.py: # TODO: pybind11 remove `immediate=True` once DM-9112 is resolved
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Yes, although see the previous comment about registering a proxy. We have yet to come up with a list of types for which this should be done (I think, Jim Bosch might know more about this).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Yes, although see the previous comment about registering a proxy. We have yet to come up with a list of types for which this should be done (I think, Jim Bosch might know more about this).
            Hide
            price Paul Price added a comment -

            And immediate=True is currently the default in the butler anyway.

            Show
            price Paul Price added a comment - And immediate=True is currently the default in the butler anyway.

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch
              Watchers:
              Gregory Dubois-Felsmann, Jim Bosch, Kian-Tat Lim, Krzysztof Findeisen, Nate Pease [X] (Inactive), Paul Price, Pim Schellart [X] (Inactive), Robert Lupton, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.