Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes

[MohamedAbdeen21]

This PR addresses issue #1321, which is caused by the hard-coded value of MaximumTaskCall. This change adds the option to set number of max task calls before termination through the new --max-runs flag.

Example Taskfile

version: '3'

vars:
  RANGE: '0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120'

tasks:
  loop-over-range:
    vars:
      range: '{{.RANGE}}'
    cmds:
      - for:
          var: range
          split: ','
        vars:
          NUMBER: '{{.ITEM}}'
        task: echo-task

  echo-task:
    cmds:
      - echo '{{.NUMBER}}'

Old behavior

$ task loop-over-range
task: [echo-task] echo '0'
0
task: [echo-task] echo '1'
1
# ...
task: [echo-task] echo '98'
98
task: Failed to run task "loop-over-range": task: Maximum task call exceeded (0) for task "echo-task": probably an cyclic dep or infinite loop
exit status 201

Issue

Notice that in the old behavior:

1. The task runs for 99 (0 to 98) times instead of 100
2. The error message says that the maximum task call is 0.
3. There is no way to set the maximum number of task calls.

New behavior

$ task loop-over-range --max-runs 121
task: [echo-task] echo '0'
0
task: [echo-task] echo '1'
1
# ...
task: [echo-task] echo '119'
119
task: [echo-task] echo '120'
120

Added

• The max-runs flag:
  • Raises an error if set to less than 1
  • If not set, uses task.MaximumTaskCall constant as default value

Fixed

• Maximum Task Call Error
  • Uses the value of max-runs correctly in the error message.
  • Raised when the task is called exactly max-run times.

[pd93]

I'm fine with this as a temporary solution if people are running into this issue a lot. However, the real problem is a bit more fundamental than this. MaximumTaskCall exists as a very arbitrary way of detecting cycles. However, all we're really doing is making an assumption that if you're calling a task many times, you're more likely than not to have a loop. It's not real cycle detection and there is no good value for this variable that will work for everyone.

This problem has been made much worse since we added loops via the for syntax as its now very easy to reach this arbitrary limit by looping over a task many times. The proper solution to this is real cycle detection. Probably via a task DAG.

We're actually very close to adding a Taskfile DAG and the Task DAG will follow on from that and will fix this issue outright (there will be no limit to how many times you can call a Task). It's up to @andreynering if we want to add this in the meantime. If this were to happen, it would probably be deprecated in the future.

[MohamedAbdeen21]

Hey @pd93. While I totally agree with you, I think this feature can still be used even when the DAG is introduced; although the purpose would be slightly different. Instead of it being used as a way of detecting cycles/loops, it'll act similar to a timeout.

In other DAG tools, I've seen people writing code that iterates over a node (task, in this case) hundreds of times, through error retries or just normal looping, effectively wasting resources, and I've always appreciated having a timeout to free resources for other DAGs to run.

However, if you guys already have that case covered in the DAG implementation, then feel free to close the PR. Thank you for the amazing work.

[pd93]

Understood, that makes sense. Perhaps once the DAG refactor happens, we can just remove the arbitrary limit of 100 (so it would be unlimited by default), and this setting can override that if a user wants to impose a manual limit. In that case, I'm happy to leave this open for review.

[pd93]

Not requesting any changes to this now. Just opening this up for thoughts and opinions as naming is always hard. Some other possibilities:

• --limit - Nice and short, but might be too generic (could clash with something like include-limit if we wanted it)
• --call-limit
• --max-task-calls - Very explicit, but longer. I don't think length is a huge issue with these flags as they're most likely to be used in CI or scripts.

[MohamedAbdeen21]

Also, I'd like to shorten the description. I couldn't find more concise (yet complete) description.

[andreynering]

Hi @MohamedAbdeen21,

I decided to just raise the limit as a mitigation for now. As @pd93 said, we'll hopefully have a much better detection soon, and then this arbitrary check will be gone.

a70f5aa