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

Make a plan to address pybind11 build size issues

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      12
    • Sprint:
      DRP F17-3, DRP F17-4, DRP F17-5
    • Team:
      Data Release Production

      Description

      Following the discussion on DM-9786, make a plan to restructure our code to reduce the size overhead introduced by the switch to pybind11.

      This should lead to an RFCed proposal which leads to an update to the Developer Guide. The work doesn't have to be done on this ticket — we'll make more tickets for that, or (if the plan is ready in time) we'll do it as a hack session at LSST 2017.

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            Looked over dm_dev_guide and pybind11_example, as requested. Minor comments; removed my name from the reviewers.

            Show
            ktl Kian-Tat Lim added a comment - Looked over dm_dev_guide and pybind11_example, as requested. Minor comments; removed my name from the reviewers.
            Hide
            Parejkoj John Parejko added a comment -

            Just as I've started to get the hang of working with our pybind11...

            I think I understand why you're taking the new approach, but it doesn't help pybind11 clarity for those who aren't C++ gurus.

            I'm not sure if there's much I could/should have looked for in the lsst.geom changes. Let me know if there are particular aspects of the code flow you want me to ponder.

            Other than my various requests for clarification, the code looks fine.

            (Warning: check for "hidden" comments: github seems to want to hide too much.)

            Show
            Parejkoj John Parejko added a comment - Just as I've started to get the hang of working with our pybind11... I think I understand why you're taking the new approach, but it doesn't help pybind11 clarity for those who aren't C++ gurus. I'm not sure if there's much I could/should have looked for in the lsst.geom changes. Let me know if there are particular aspects of the code flow you want me to ponder. Other than my various requests for clarification, the code looks fine. (Warning: check for "hidden" comments: github seems to want to hide too much.)
            Hide
            jbosch Jim Bosch added a comment -

            Thanks for the extremely quick review!

            And yes, I am a bit worried about the prevalence of C++ lambdas in the new approach in particular - they're wonderful things once you get used to them, but the syntax is very jarring at first.  Unfortunately I think they are the only way out of the pickle we're in with circular dependencies, at least in afw, and at least without a ton of boilerplate.

            Happily I don't think there was much to look for in the geom changes; given the stuff in utils, the changes in geom (and soon elsewhere) are quite mechanical, and I don't think it's a good use of anyone's time to do anything like a detailed review of them.  I do expect there to be tricky bits worth a closer look when I get to afw, and I'll be sure to call them out for whichever poor souls get saddled with that review.

            Show
            jbosch Jim Bosch added a comment - Thanks for the extremely quick review! And yes, I am a bit worried about the prevalence of C++ lambdas in the new approach in particular - they're wonderful things once you get used to them, but the syntax is very jarring at first.  Unfortunately I think they are the only way out of the pickle we're in with circular dependencies, at least in afw, and at least without a ton of boilerplate. Happily I don't think there was much to look for in the geom changes; given the stuff in utils, the changes in geom (and soon elsewhere) are quite mechanical, and I don't think it's a good use of anyone's time to do anything like a detailed review of them.  I do expect there to be tricky bits worth a closer look when I get to afw, and I'll be sure to call them out for whichever poor souls get saddled with that review.
            Hide
            jbosch Jim Bosch added a comment -

            Unfortunately, this is now on hold until we can figure out why the linking apparently doesn't work on OS X (which is not something I'm in a position to do easily myself).  sconsUtils+base+utils seems to be enough to reproduce the problem.  As far as I can tell from looking at the Jenkins output, the main module translation unit is just not able to the wrapX definitions provided by the other translation units.  

            Show
            jbosch Jim Bosch added a comment - Unfortunately, this is now on hold until we can figure out why the linking apparently doesn't work on OS X (which is not something I'm in a position to do easily myself).  sconsUtils+base+utils seems to be enough to reproduce the problem.  As far as I can tell from looking at the Jenkins output, the main module translation unit is just not able to the wrapX  definitions provided by the other translation units.  
            Hide
            tjenness Tim Jenness added a comment -

            The problem is that macOS has a case insensitive case preserving file system and you can't have _Utils.cc and _utils.cc in the same directory.

            Show
            tjenness Tim Jenness added a comment - The problem is that macOS has a case insensitive case preserving file system and you can't have _Utils.cc and _utils.cc in the same directory.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              swinbank John Swinbank
              Reviewers:
              John Parejko
              Watchers:
              Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.