Should xrandom be defensive against ZeroDivisionError?

[kinow]

Hi,

At the moment if the parameter provided for the percent in the xrandom random external trigger is zero, there will be a ZeroDivisionError.

2019-03-24T18:10:24+13:00 DEBUG - END TASK PROCESSING (took 0.002267599105834961 seconds)
2019-03-24T18:10:25+13:00 DEBUG - [xtrigger-func cmd] cylc-function-run xrandom '[]' '{"percent": 0, "secs": 2, "_": "1"}' /home/kinow/Development/python/workspace/cylc/etc/examples/xtrigger/xrandom
	[xtrigger-func ret_code] 1
	[xtrigger-func err]
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc/bin/cylc-function-run", line 37, in <module>
	    run_function(sys.argv[1], sys.argv[2], sys.argv[3], sys.argv[4])
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 85, in run_function
	    res = func(*func_args, **func_kwargs)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/xtriggers/xrandom.py", line 44, in xrandom
	    satisfied = (1 == randint(1, 100 / int(percent)))
	ZeroDivisionError: division by zero
2019-03-24T18:10:25+13:00 ERROR - the JSON object must be str, bytes or bytearray, not NoneType
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 257, in start
	    self.run()
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 1534, in run
	    self.proc_pool.process()
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 174, in process
	    self._proc_exit(proc, "", ctx, callback, callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 165, in _proc_exit
	    self._run_command_exit(ctx, callback, callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 360, in _run_command_exit
	    callback(ctx, *callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/xtrigger_mgr.py", line 263, in callback
	    satisfied, results = json.loads(ctx.out)
	  File "/home/kinow/Development/python/anaconda3/lib/python3.7/json/__init__.py", line 341, in loads
	    raise TypeError(f'the JSON object must be str, bytes or bytearray, '
	TypeError: the JSON object must be str, bytes or bytearray, not NoneType
2019-03-24T18:10:25+13:00 ERROR - error caught: cleaning up before exit
2019-03-24T18:10:25+13:00 INFO - Suite shutting down - ERROR: the JSON object must be str, bytes or bytearray, not NoneType
2019-03-24T18:10:25+13:00 ERROR - [Errno 3] No such process
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 257, in start
	    self.run()
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 1534, in run
	    self.proc_pool.process()
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 174, in process
	    self._proc_exit(proc, "", ctx, callback, callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 165, in _proc_exit
	    self._run_command_exit(ctx, callback, callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 360, in _run_command_exit
	    callback(ctx, *callback_args)
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/xtrigger_mgr.py", line 263, in callback
	    satisfied, results = json.loads(ctx.out)
	  File "/home/kinow/Development/python/anaconda3/lib/python3.7/json/__init__.py", line 341, in loads
	    raise TypeError(f'the JSON object must be str, bytes or bytearray, '
	TypeError: the JSON object must be str, bytes or bytearray, not NoneType
	
	During handling of the above exception, another exception occurred:
	
	Traceback (most recent call last):
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 283, in start
	    self.shutdown('ERROR: ' + str(exc))
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/scheduler.py", line 1727, in shutdown
	    self.proc_pool.terminate()
	  File "/home/kinow/Development/python/workspace/cylc/lib/cylc/subprocpool.py", line 263, in terminate
	    os.killpg(proc.pid, SIGKILL)
	ProcessLookupError: [Errno 3] No such process
2019-03-24T18:10:25+13:00 INFO - DONE

Simply doing

diff --git a/lib/cylc/xtriggers/xrandom.py b/lib/cylc/xtriggers/xrandom.py
index 43f4ca4cb..0a36c3b86 100644
--- a/lib/cylc/xtriggers/xrandom.py
+++ b/lib/cylc/xtriggers/xrandom.py
@@ -41,7 +41,7 @@ def xrandom(percent, secs=0, _=None, debug=False):
     """
     sleep(float(secs))
     results = {}
-    satisfied = (1 == randint(1, 100 / int(percent)))
+    satisfied = int(percent) != 0 and (1 == randint(1, 100 / int(percent)))
     if satisfied:
         results = {
             'COLOR': COLORS[randint(0, len(COLORS) - 1)],

Would return False when zero, however, the trigger would simply never succeed. So the following could be useful to prevent the ZeroDivisionError? If for whatever reason the percent is zero, then just consider it as done.

diff --git a/lib/cylc/xtriggers/xrandom.py b/lib/cylc/xtriggers/xrandom.py
index 43f4ca4cb..4793411d2 100644
--- a/lib/cylc/xtriggers/xrandom.py
+++ b/lib/cylc/xtriggers/xrandom.py
@@ -41,7 +41,7 @@ def xrandom(percent, secs=0, _=None, debug=False):
     """
     sleep(float(secs))
     results = {}
-    satisfied = (1 == randint(1, 100 / int(percent)))
+    satisfied = int(percent) == 0 or (1 == randint(1, 100 / int(percent)))
     if satisfied:
         results = {
             'COLOR': COLORS[randint(0, len(COLORS) - 1)],

[hjoliver]

@kinow - good spotting. This "xrandom" things is only meant to be a trivial example of an external trigger, that doesn't actually require any real connection to the external world (and it is therefore very easy to demo the functionality). However, it still shouldn't be buggy!

As for your suggested fixes though, I think the first choice is what logically conforms to the documented functionality: "succeed with x percent likelihood" ... means never succeed if x is 0.

[kinow]

it still shouldn't be buggy!

+1 🎉

As for your suggested fixes though, I think the first choice is what logically conforms to the documented functionality: "succeed with x percent likelihood" ... means never succeed if x is 0.

Agreed, PR on its way. Thanks @hjoliver !

[kinow]

We are now:

• simplifying the formula to calculate the likelihood to random() < float(percent) / 100
• making the percent parameter now a float instead of an int

See discussion in pull requests for complete comments.

[kinow]

Thanks @matthewrmshin ! I swear I was adding label and milestone because I realized I forgot doing that in a few tickets and noticed you fixed that for me. So I thought this time I would have everything done in this ticket. Very close! Will try to remember the assignee next time 👍