# Add pybind11 rules to DM dev-guide

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
• Story Points:
1
• Epic Link:
• 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.

#### Activity

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

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

Show
Pim Schellart [X] (Inactive) added a comment - I migrated DMTN-024 to the DM Pybind11 Style Guide, incorporating some changes and lessons learned.
Hide
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Krzysztof Findeisen added a comment -

Oops! Sorry about that, closed the wrong ticket.

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

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

Show
Krzysztof Findeisen added a comment - Russell Owen wrap sounds slightly off to my ear, but I don't have a strong preference.
Hide
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
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
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
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
Pim Schellart [X] (Inactive) added a comment -

Merged

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

#### People

Assignee:
Pim Schellart [X] (Inactive)
Reporter:
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.