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

Add pybind11 rules to DM dev-guide

    XMLWordPrintable

    Details

    • Story Points:
      1
    • Sprint:
      DRP S17-2, DRP S17-3, DRP S17-4
    • Team:
      Data Release Production

      Description

      Update DMTN-024 with conventions acquired during pybind11 development.
      These guidelines will then need to be RFC'd before being made an official part of the DM developer guide.

        Attachments

          Issue Links

            Activity

            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I migrated DMTN-024 to the DM Pybind11 Style Guide, incorporating some changes and lessons learned.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I migrated DMTN-024 to the DM Pybind11 Style Guide, incorporating some changes and lessons learned.
            Hide
            jbosch Jim Bosch added a comment -

            Pim Schellart [X], I think I've made all the changes we agreed up on yesterday, but sadly I didn't write them down at the time so I might have forgotten something. Changes include:

            • Removing the numeric suffix from the filename conventions.
            • Mandate use of the "py" namespace alias (at least in source files; open question on whether to do it in headers, I think).
            • Rewrote the type deduction guideline.
            • New guideline on adding kwargs.
            • Permission for letting wrapper module docstrings be empty.

            Please take a look when you get a chance, and let me know if you can think of anything else we should have (or if there's anything you disagree with).

            Show
            jbosch Jim Bosch added a comment - Pim Schellart [X] , I think I've made all the changes we agreed up on yesterday, but sadly I didn't write them down at the time so I might have forgotten something. Changes include: Removing the numeric suffix from the filename conventions. Mandate use of the "py" namespace alias (at least in source files; open question on whether to do it in headers, I think). Rewrote the type deduction guideline. New guideline on adding kwargs. Permission for letting wrapper module docstrings be empty. Please take a look when you get a chance, and let me know if you can think of anything else we should have (or if there's anything you disagree with).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I did some minor cleanup. Mostly standardising on SHALL and MAY, given our strange conventions where SHALL and MUST (and SHOULD and MAY) mean the same.

            I also added some more examples.

            Still to do is adding references and numbering (which I strongly suggest we should do per section to avoid the problem we have with the other guides where adding one rule necessitates renumbering everything).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I did some minor cleanup. Mostly standardising on SHALL and MAY , given our strange conventions where SHALL and MUST (and SHOULD and MAY ) mean the same. I also added some more examples. Still to do is adding references and numbering (which I strongly suggest we should do per section to avoid the problem we have with the other guides where adding one rule necessitates renumbering everything).
            Hide
            jbosch Jim Bosch added a comment -

            Going from the header of the document (which I think you copied from the other style guides), think we're supposed to use MUST, SHOULD, and MAY, and those all mean different things (SHOULD is a recommendation, MAY means it's permitted). SHALL means the same as MUST and should be replaced by it.

            Show
            jbosch Jim Bosch added a comment - Going from the header of the document (which I think you copied from the other style guides), think we're supposed to use MUST, SHOULD, and MAY, and those all mean different things (SHOULD is a recommendation, MAY means it's permitted). SHALL means the same as MUST and should be replaced by it.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Yes, that is what I would expect, but the summary of RFC-2119 (at https://developer.lsst.io/coding/intro.html#style-guide-rfc-2119 ) confusingly seems to suggest that SHOULD==MAY and I distinctly remember John Swinbank telling me that in a review.
            But I agree that we should make the distinction. As a side note I would suggest we remove that confusing page and directly point to the RFC (or copy over its contents verbatim).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Yes, that is what I would expect, but the summary of RFC-2119 (at https://developer.lsst.io/coding/intro.html#style-guide-rfc-2119 ) confusingly seems to suggest that SHOULD==MAY and I distinctly remember John Swinbank telling me that in a review. But I agree that we should make the distinction. As a side note I would suggest we remove that confusing page and directly point to the RFC (or copy over its contents verbatim).
            Hide
            swinbank John Swinbank added a comment -

            I don't remember that review (don't deny it happened, though), but I do think that you have to follow our documented conventions when writing documentation, even if you don't like them.

            That said, I agree that the summary in the Developer Guide looks wrong: it makes no sense to me (and it's certainly not in line with RFC-2119) to suggest that “may” and “should” are synonymous. Perhaps Jonathan Sick can clarify?

            Show
            swinbank John Swinbank added a comment - I don't remember that review (don't deny it happened, though), but I do think that you have to follow our documented conventions when writing documentation, even if you don't like them. That said, I agree that the summary in the Developer Guide looks wrong: it makes no sense to me (and it's certainly not in line with RFC-2119 ) to suggest that “may” and “should” are synonymous. Perhaps Jonathan Sick can clarify?
            Hide
            jbosch Jim Bosch added a comment -

            Huh, I guess I've been reading it incorrectly the whole time. And I suppose if we're talking about how strict a rule is, the degree to which it's recommended is irrelevant if it's ultimately up to the developer. That said, if we're allowed to use both SHOULD and MAY, would you mind leaving those distinctions in as they were? It won't change how binding the rules are, but I think it does convey the right amount of "recommendation" - I tend to use SHOULD for rules that have exceptions, but only rarely. And of course, if you disagree with me on how strongly we should recommend something in particular, please push back.

            Show
            jbosch Jim Bosch added a comment - Huh, I guess I've been reading it incorrectly the whole time. And I suppose if we're talking about how strict a rule is, the degree to which it's recommended is irrelevant if it's ultimately up to the developer. That said, if we're allowed to use both SHOULD and MAY, would you mind leaving those distinctions in as they were? It won't change how binding the rules are, but I think it does convey the right amount of "recommendation" - I tend to use SHOULD for rules that have exceptions, but only rarely. And of course, if you disagree with me on how strongly we should recommend something in particular, please push back.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Yes, I agree with that. Lets try to use MUST, SHOULD and MAY in the spirit of RFC-2119 even though the latter may or may not be distinct in stringency currently.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Yes, I agree with that. Lets try to use MUST , SHOULD and MAY in the spirit of RFC-2119 even though the latter may or may not be distinct in stringency currently.
            Hide
            jsick Jonathan Sick added a comment -

            Interesting. I can say that https://developer.lsst.io/coding/intro.html#stringency-levels was copied verbatim from the original DM coding standards in confluence (along with our original C++ and Python style guides). I'm very much in favor of fixing that section so that it truly matches RFC-2119 and avoids this confusion.

            Before doing so we'd want to ensure that the sprit of our coding standards are still true (we may need to adjust stringency levels in the existing coding standards when we use the correct RFC-2119 terminology). Sounds like this will need a new ticket and Kian-Tat Lim should be brought in the loop as the owner of the coding standards.

            Show
            jsick Jonathan Sick added a comment - Interesting. I can say that https://developer.lsst.io/coding/intro.html#stringency-levels was copied verbatim from the original DM coding standards in confluence (along with our original C++ and Python style guides). I'm very much in favor of fixing that section so that it truly matches RFC-2119 and avoids this confusion. Before doing so we'd want to ensure that the sprit of our coding standards are still true (we may need to adjust stringency levels in the existing coding standards when we use the correct RFC-2119 terminology). Sounds like this will need a new ticket and Kian-Tat Lim should be brought in the loop as the owner of the coding standards.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            If we are going to update the stringency standards I would suggest also updating the rule numbering at the same time. Because the numbering scheme now spans sections it is impossible to add a rule to a section without changing all numbers after it.

            And it might make most sense to do this after DM-8713 is merged (since that removes many rules in the layout section of the C++ style guide).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - If we are going to update the stringency standards I would suggest also updating the rule numbering at the same time. Because the numbering scheme now spans sections it is impossible to add a rule to a section without changing all numbers after it. And it might make most sense to do this after DM-8713 is merged (since that removes many rules in the layout section of the C++ style guide).
            Hide
            jbosch Jim Bosch added a comment -

            Much as I'd like to get both of those issues fixed, I worry linking them will mean fixing neither (in the short term, at least). I'd also like to press forward with this ticket without waiting for the more general guidelines to be updated. I think what we've written is not inconsistent with either version of the general guidelines.

            Show
            jbosch Jim Bosch added a comment - Much as I'd like to get both of those issues fixed, I worry linking them will mean fixing neither (in the short term, at least). I'd also like to press forward with this ticket without waiting for the more general guidelines to be updated. I think what we've written is not inconsistent with either version of the general guidelines.
            Hide
            jbosch Jim Bosch added a comment -

            Given that the RFC is already looking at this ticket branch, I'd like to just keep this ticket in review until the RFC is complete as use it as the implementation ticket for the RFC (it'll both trigger and be triggered by the RFC - I hope that doesn't bother anyone too much).

            Show
            jbosch Jim Bosch added a comment - Given that the RFC is already looking at this ticket branch, I'd like to just keep this ticket in review until the RFC is complete as use it as the implementation ticket for the RFC (it'll both trigger and be triggered by the RFC - I hope that doesn't bother anyone too much).
            Hide
            krzys Krzysztof Findeisen added a comment -

            Jim Bosch, the rule about __truediv__ vs. __div__ should also apply to __itruediv__ and __idiv__. Sorry for not saying so explicitly.

            Show
            krzys Krzysztof Findeisen added a comment - Jim Bosch , the rule about __truediv__ vs. __div__ should also apply to __itruediv__ and __idiv__ . Sorry for not saying so explicitly.
            Hide
            rowen Russell Owen added a comment -

            Looks good. One requested change:

            Must we change from pybind11 to python in our convention for header files and namespaces that provide shared pybind11 support https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id13 ? I find pybind11 clearer since it is specific to our tool, and all our existing code uses it.

            (I'll also say that I wish we used "wrapFoo" instead of "declareFoo" but our existing code goes the other way, so it's probably not worth changing).

            Suggested enhancements to the doc:

            Show
            rowen Russell Owen added a comment - Looks good. One requested change: Must we change from pybind11 to python in our convention for header files and namespaces that provide shared pybind11 support https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id13 ? I find pybind11 clearer since it is specific to our tool, and all our existing code uses it. (I'll also say that I wish we used "wrapFoo" instead of "declareFoo" but our existing code goes the other way, so it's probably not worth changing). Suggested enhancements to the doc: Please provide an example for using pybind11::module::import() in https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id12 Typo "accross" In https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id16 (use "py::") it says "This alias MUST NOT be defined at namespace scope in header files (see C++ rule 4-13), though it MAY be defined locally within functions in headers.". I don't understand. A few more words or an example would help. It is probably worth talking about wrapping enums that are used as ints in the C++ code: we must wrap these as attributes, not enums, in order to avoid pointless casting.
            Hide
            jbosch Jim Bosch added a comment -

            The reason we proposed switching from the pybind11 to python is that pybind11 conflicts with the main pybind11 namespace - even if it's aliased to py, it's still available as pybind11. And while e.g. lsst::afw::geom::pybind11::Foo will be selected in preference to ::pybind11::Foo if you just say pybind11::Foo within lsst::afw::geom, I think it's still confusing.

            I don't have strong feelings about wrapFoo vs. declareFoo. If there's broad consensus, I don't think it'd be hard to change, though.

            I've added the requested examples, added a rule about when to wrap enums as integer attributes (an excellent suggestion), and clarified that the division rule applies to in-place operators.

            Show
            jbosch Jim Bosch added a comment - The reason we proposed switching from the pybind11 to python is that pybind11 conflicts with the main pybind11 namespace - even if it's aliased to py , it's still available as pybind11 . And while e.g. lsst::afw::geom::pybind11::Foo will be selected in preference to ::pybind11::Foo if you just say pybind11::Foo within lsst::afw::geom , I think it's still confusing. I don't have strong feelings about wrapFoo vs. declareFoo . If there's broad consensus, I don't think it'd be hard to change, though. I've added the requested examples, added a rule about when to wrap enums as integer attributes (an excellent suggestion), and clarified that the division rule applies to in-place operators.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            `declare` came from swig macros. I don't care. Easy to change. But not really worth it probably.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - `declare` came from swig macros. I don't care. Easy to change. But not really worth it probably.
            Hide
            rowen Russell Owen added a comment -

            You make a good point about the pybind11 namespace.

            Regarding renaming declareFoo to wrapFoo: how do other team members feel? I would like to switch if folks are comfortable with it, but don't want to get bogged down arguing about minutiae.

            Show
            rowen Russell Owen added a comment - You make a good point about the pybind11 namespace. Regarding renaming declareFoo to wrapFoo : how do other team members feel? I would like to switch if folks are comfortable with it, but don't want to get bogged down arguing about minutiae.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Oops! Sorry about that, closed the wrong ticket.

            Show
            krzys Krzysztof Findeisen added a comment - Oops! Sorry about that, closed the wrong ticket.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Russell Owen wrap sounds slightly off to my ear, but I don't have a strong preference.

            Show
            krzys Krzysztof Findeisen added a comment - Russell Owen wrap sounds slightly off to my ear, but I don't have a strong preference.
            Hide
            ktl Kian-Tat Lim added a comment -

            What's there now looks fine to me, modulo some minor tweaks in the PR. I trust that any changes Jim will ask for are equally fine (or will fix up afterwards if not).

            Show
            ktl Kian-Tat Lim added a comment - What's there now looks fine to me, modulo some minor tweaks in the PR. I trust that any changes Jim will ask for are equally fine (or will fix up afterwards if not).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Overlapping reviews with DM-9098, but all changes have now been implemented.
            Will be merged together with pybind11 switch.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Overlapping reviews with DM-9098 , but all changes have now been implemented. Will be merged together with pybind11 switch.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, John Swinbank, Jonathan Sick, Kian-Tat Lim, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.