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

Use --fail-fast in ap_verify to halt execution on first error

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ap_verify
    • Labels:
    • Story Points:
      1
    • Sprint:
      AP S22-6 (May), AP F22-1 (June), AP F22-3 (August)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      I found the --fail-fast option to pipetask, which causes the pipeline execution to fail immediately on error, instead of trying to continue execution. I think this is what we want for ap_verify, as it's purpose is to test our pipeline, and any failures should halt.

      Adding `"--fail-fast",` to line 98 of ap_verify's pipeline_driver.py seems to have done the trick.

      Alternately, we could make this a commandline option to ap_verify, if we don't want to always fail immediately (but I think we do?).

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            I'm sorry, but I still don't understand what you are trying to do (with either change), so I don't believe I'm in a position to approve or disapprove. Perhaps Meredith Rawls has a better understanding of what this would mean for our workflows (if she's not too busy)?

            Show
            krzys Krzysztof Findeisen added a comment - I'm sorry, but I still don't understand what you are trying to do (with either change), so I don't believe I'm in a position to approve or disapprove. Perhaps Meredith Rawls has a better understanding of what this would mean for our workflows (if she's not too busy)?
            Hide
            mrawls Meredith Rawls added a comment -

            I feel like you two disagree on the purpose of ap_verify. Both your views make sense to me... it sounds like John wants "fail fast" to be the ap_verify default to make debugging with it go faster, and Krzysztof is concerned this goes counter to the main purpose of ap_verify as a tool to reproduce runs.

            Could we compromise by making "fail fast" an acceptable command-line option for ap_verify (so John can use it for debugging when necessary), but NOT having it be the default (since it's not necessarily desired behavior for checking runs can be reproduced)?

            Show
            mrawls Meredith Rawls added a comment - I feel like you two disagree on the purpose of ap_verify. Both your views make sense to me... it sounds like John wants "fail fast" to be the ap_verify default to make debugging with it go faster, and Krzysztof is concerned this goes counter to the main purpose of ap_verify as a tool to reproduce runs. Could we compromise by making "fail fast" an acceptable command-line option for ap_verify (so John can use it for debugging when necessary), but NOT having it be the default (since it's not necessarily desired behavior for checking runs can be reproduced)?
            Hide
            krzys Krzysztof Findeisen added a comment -

            If Meredith thinks that's best, then I have no objections.

            Show
            krzys Krzysztof Findeisen added a comment - If Meredith thinks that's best, then I have no objections.
            Hide
            Parejkoj John Parejko added a comment -

            Adding it as a commandline option is less trivial than this commit, so it might be a while before I get to it.

            Show
            Parejkoj John Parejko added a comment - Adding it as a commandline option is less trivial than this commit, so it might be a while before I get to it.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Following a discussion on #dm-alert-prod about whether ap_verify still has the same purpose it was originally written for (it does not, and I'm the only one who thought otherwise), I've no further objections to the PR as-is.

            Show
            krzys Krzysztof Findeisen added a comment - Following a discussion on #dm-alert-prod about whether ap_verify still has the same purpose it was originally written for (it does not, and I'm the only one who thought otherwise), I've no further objections to the PR as-is.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Eric Bellm, Ian Sullivan, John Parejko, Krzysztof Findeisen, Meredith Rawls
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.