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

Refactor ap_pipe to use CmdLineTask primitives

    Details

    • Story Points:
      10
    • Epic Link:
    • Sprint:
      AP S18-2, AP S18-3, AP S18-4
    • Team:
      Alert Production

      Description

      Because ap_pipe was developed with a CI verification workflow in mind (in concert with ap_verify), it includes ingestion and does not use the Stack primitives. We are moving away from this approach; this ticket is refactor ap_pipe to have a separate ingestion routine as needed and to have the remaining processing be a CmdLineTask.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Progress report: ap_pipe is now a CmdLineTask, although it requires quite a few hacks (including significant duplication of pipe_base code) to deal with DM-11767, DM-11865, DM-13602, DM-13626, and DM-13672. We'll see how horrified the reviewer is.

            I have not yet made any attempt to modify ap_verify to work with the new ap_pipe API. Since the new ap_pipe drivers take datarefs as inputs, this will require adding some butler code to ap_verify.

            The command line for ap_pipe has changed slightly; see https://gist.github.com/kfindeisen/2d3d4fcc7252b727761f172a59d15c48#file-gistfile1-txt for examples.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Progress report: ap_pipe is now a CmdLineTask , although it requires quite a few hacks (including significant duplication of pipe_base code) to deal with DM-11767 , DM-11865 , DM-13602 , DM-13626 , and DM-13672 . We'll see how horrified the reviewer is. I have not yet made any attempt to modify ap_verify to work with the new ap_pipe API. Since the new ap_pipe drivers take datarefs as inputs, this will require adding some butler code to ap_verify . The command line for ap_pipe has changed slightly; see https://gist.github.com/kfindeisen/2d3d4fcc7252b727761f172a59d15c48#file-gistfile1-txt for examples.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen, can you please review this refactoring of ap_pipe? I'd be particularly interested in any misuse of the task framework that wasn't deliberate (i.e., flagged by a comment).

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , can you please review this refactoring of ap_pipe ? I'd be particularly interested in any misuse of the task framework that wasn't deliberate (i.e., flagged by a comment).
            Hide
            rowen Russell Owen added a comment -

            My main request is to fix DM-11865 by adding support to pipe_base ArgumentParser for additional repo arguments. I think it would be fairly simple to do that and it would allow you to remove a lot of duplicate code and "TODO" work, leaving the new code in ap_pipe much cleaner and easier to follow.

            I also suggest that you make ingest a task (a subclass of pipe_base Task) as it is easy to do and would simplify some of the things you are doing now and hopefully eliminate the need to pass fullMetadata around.

            Other than that it looks good. Thank you for the excellent documentation.

            Show
            rowen Russell Owen added a comment - My main request is to fix DM-11865 by adding support to pipe_base ArgumentParser for additional repo arguments. I think it would be fairly simple to do that and it would allow you to remove a lot of duplicate code and "TODO" work, leaving the new code in ap_pipe much cleaner and easier to follow. I also suggest that you make ingest a task (a subclass of pipe_base Task) as it is easy to do and would simplify some of the things you are doing now and hopefully eliminate the need to pass fullMetadata around. Other than that it looks good. Thank you for the excellent documentation.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Thank you for the helpful review. Conversion of the ap_verify's ingestion module into a task will happen on DM-13530, as it would be a significant scope change for this ticket (in particular, reimplementing ingestion does not affect ap_pipe in any way). Adding multi-repository support to pipe_base is, as agreed on the ap_pipe pull request, something we will RFC separately. I think I've addressed everything else.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Thank you for the helpful review. Conversion of the ap_verify 's ingestion module into a task will happen on DM-13530 , as it would be a significant scope change for this ticket (in particular, reimplementing ingestion does not affect ap_pipe in any way). Adding multi-repository support to pipe_base is, as agreed on the ap_pipe pull request, something we will RFC separately. I think I've addressed everything else.

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                ebellm Eric Bellm
                Reviewers:
                Russell Owen
                Watchers:
                Eric Bellm, Krzysztof Findeisen, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: