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

Wrap pipe_tasks with pybind11

    Details

      Description

      Fix issues in pybind11 wrapping revealed in pipe_tasks.

        Attachments

          Issue Links

            Activity

            pschella Pim Schellart [X] (Inactive) created issue -
            pschella Pim Schellart [X] (Inactive) made changes -
            Field Original Value New Value
            Epic Link DM-7717 [ 26925 ]
            swinbank John Swinbank made changes -
            Labels SciencePipelines
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8465 [ DM-8465 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8464 [ DM-8464 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8419 [ DM-8419 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8453 [ DM-8453 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8458 [ DM-8458 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8457 [ DM-8457 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8459 [ DM-8459 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8460 [ DM-8460 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8451 [ DM-8451 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8463 [ DM-8463 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8462 [ DM-8462 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8466 [ DM-8466 ]
            pschella Pim Schellart [X] (Inactive) made changes -
            Assignee Krzysztof Findeisen [ krzys ]
            krzys Krzysztof Findeisen made changes -
            Epic Link DM-7717 [ 26925 ] DM-8450 [ 28066 ]
            krzys Krzysztof Findeisen made changes -
            Team Alert Production [ 10300 ]
            Labels SciencePipelines Pybind11 SciencePipelines
            krzys Krzysztof Findeisen made changes -
            Sprint Alert Production S17 - 1 [ 355 ]
            krzys Krzysztof Findeisen made changes -
            Rank Ranked lower
            krzys Krzysztof Findeisen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8877 [ DM-8877 ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen,

            Please review these changes. The changes in each package are individually small, but there are quite a few of them.

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , Please review these changes. The changes in each package are individually small, but there are quite a few of them.
            krzys Krzysztof Findeisen made changes -
            Reviewers Russell Owen [ rowen ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            krzys Krzysztof Findeisen made changes -
            Story Points 0.1 6
            krzys Krzysztof Findeisen made changes -
            Description SP yet to be determined Fix issues in pybind11 wrapping revealed in {{pipe_tasks}}.
            Hide
            rowen Russell Owen added a comment -

            Overall this looks excellent, and it was clearly a heroic effort.

            However, I have one serious concern: some tests in pipe_tasks are being skipped because BaseRecord.get isn't doing the right thing. That suggests the current wrapping is inadequate and should be fixed. The skip message suggests waiting for DM-8716, but unless Jim Bosch agrees with that and plans to merge that soon, I think it would be better to just fix afw without the extra benefits of DM-8716.

            Show
            rowen Russell Owen added a comment - Overall this looks excellent, and it was clearly a heroic effort. However, I have one serious concern: some tests in pipe_tasks are being skipped because BaseRecord.get isn't doing the right thing. That suggests the current wrapping is inadequate and should be fixed. The skip message suggests waiting for DM-8716 , but unless Jim Bosch agrees with that and plans to merge that soon, I think it would be better to just fix afw without the extra benefits of DM-8716 .
            Hide
            jbosch Jim Bosch added a comment -

            I did indeed advise Krzysztof Findeisen to defer fixing problems with BaseRecord.get/set until DM-8716, but if that means we're not testing much of anything in pipe_base as a result, that implies we also won't really be able to run any full integration tests until DM-8716 is complete, and that might cause enough problems with the timeline that we would be better off finding a workaround. But I still don't want us to put much effort into cleaning up BaseRecord.get/set as they stand now, because they're going to be completely blown away in DM-8716.

            Looking back at the original Slack conversation, it seems the main problem was BaseRecord.getL not being present - if that is the only kind of problem we're seeing here, the best workaround may be to replace getL with get (and likewise for other suffixes) in the Python code. The suffixed versions were a workaround for bad performance in BaseRecord.get that should go away in DM-8716, and prior to that this change should let us run the code even if there's a temporary performance regression. As with the workarounds for DM-9112, though, it'd be good to put those changes on a separate commit so we can revert them easily later if we have to. Will that work?

            Show
            jbosch Jim Bosch added a comment - I did indeed advise Krzysztof Findeisen to defer fixing problems with BaseRecord.get/set until DM-8716 , but if that means we're not testing much of anything in pipe_base as a result, that implies we also won't really be able to run any full integration tests until DM-8716 is complete, and that might cause enough problems with the timeline that we would be better off finding a workaround. But I still don't want us to put much effort into cleaning up BaseRecord.get/set as they stand now, because they're going to be completely blown away in DM-8716 . Looking back at the original Slack conversation, it seems the main problem was BaseRecord.getL not being present - if that is the only kind of problem we're seeing here, the best workaround may be to replace getL with get (and likewise for other suffixes) in the Python code. The suffixed versions were a workaround for bad performance in BaseRecord.get that should go away in DM-8716 , and prior to that this change should let us run the code even if there's a temporary performance regression. As with the workarounds for DM-9112 , though, it'd be good to put those changes on a separate commit so we can revert them easily later if we have to. Will that work?
            Hide
            krzys Krzysztof Findeisen added a comment -

            Fine by me. I can make the replacements on Monday.

            Show
            krzys Krzysztof Findeisen added a comment - Fine by me. I can make the replacements on Monday.
            Hide
            rowen Russell Owen added a comment - - edited

            I think we already define these type-specific getters, just using a different name (e.g. _get_L or something like that). If so, I would expect it to be very simple to also generate identical wrappers using the public name (e.g. getL in the pybind11 wrapper. I would expect that to be trivial, and much simpler than changing all the python code, plus it would give the desired performance. Or is there some subtlety that I am missing?

            Show
            rowen Russell Owen added a comment - - edited I think we already define these type-specific getters, just using a different name (e.g. _get_L or something like that). If so, I would expect it to be very simple to also generate identical wrappers using the public name (e.g. getL in the pybind11 wrapper. I would expect that to be trivial, and much simpler than changing all the python code, plus it would give the desired performance. Or is there some subtlety that I am missing?
            Hide
            jbosch Jim Bosch added a comment -

            No subtleties, so it sounds like Russell Owen's proposal is also viable; I just wanted to avoid too much effort on code that will go away. I'll leave it to Krzysztof Findeisen to decide which is easier.

            Show
            jbosch Jim Bosch added a comment - No subtleties, so it sounds like Russell Owen 's proposal is also viable; I just wanted to avoid too much effort on code that will go away. I'll leave it to Krzysztof Findeisen to decide which is easier.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Since it's not clear to me that the suffixed names should be part of the public BaseRecord interface, I would prefer to replace the names with get unless Russell has strong objections.

            Show
            krzys Krzysztof Findeisen added a comment - Since it's not clear to me that the suffixed names should be part of the public BaseRecord interface, I would prefer to replace the names with get unless Russell has strong objections.
            Hide
            rowen Russell Owen added a comment -

            Due to performance I still lean towards leaving the code along and emulating the old API, but either solution is acceptable to me as long as all code is changed, not just the code that happens to be exposed by the unit tests we have run so far. In other words, I think a global search for each of the missing type-specific "get" method is required. It isn't difficult, just a minor nuisance.

            Show
            rowen Russell Owen added a comment - Due to performance I still lean towards leaving the code along and emulating the old API, but either solution is acceptable to me as long as all code is changed, not just the code that happens to be exposed by the unit tests we have run so far. In other words, I think a global search for each of the missing type-specific "get" method is required. It isn't difficult, just a minor nuisance.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Looks like cleaning this up is best left to DM-8716 after all. getD, at least, is used by some very high level packages like analysis and testingPipeQA. I'll wrap the Swig specializations for now.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Looks like cleaning this up is best left to DM-8716 after all. getD , at least, is used by some very high level packages like analysis and testingPipeQA . I'll wrap the Swig specializations for now.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Russell Owen I've updated afw, meas_base, ip_isr, and pipe_tasks. Can you take another look?

            Show
            krzys Krzysztof Findeisen added a comment - Russell Owen I've updated afw , meas_base , ip_isr , and pipe_tasks . Can you take another look?
            Hide
            rowen Russell Owen added a comment -

            Excellent work. I had one small tweak to a comment (remove or reword) in pipe_tasks, but you can change it or not as you see fit.

            Show
            rowen Russell Owen added a comment - Excellent work. I had one small tweak to a comment (remove or reword) in pipe_tasks, but you can change it or not as you see fit.
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            krzys Krzysztof Findeisen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            tjenness Tim Jenness made changes -
            Component/s pipe_tasks [ 10726 ]

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                pschella Pim Schellart [X] (Inactive)
                Reviewers:
                Russell Owen
                Watchers:
                Jim Bosch, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: