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

Refactor ap_pipe to use CmdLineTask primitives

    XMLWordPrintable

Details

    • 10
    • AP S18-2, AP S18-3, AP S18-4
    • 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

            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.

            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.

            Hi rowen, 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).

            krzys Krzysztof Findeisen added a comment - Hi rowen , 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).
            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.

            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.
            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.

            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

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

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.