-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding timeout functionality to action nodes for ineffectual work functions #62
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pep8 says module names should be lowercase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you don't have to change it because we need to do a mass update later to bring the package into compliance |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import time | ||
|
||
|
||
class Timer(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixed feelings about this class name because it collides with |
||
def __init__(self, | ||
name: str, | ||
timer_period_seconds: float, | ||
auto_start: bool = False, | ||
is_periodic: bool = False): | ||
self.name = name | ||
self.timer_period_seconds = timer_period_seconds | ||
self.is_periodic = is_periodic | ||
self.auto_start = auto_start | ||
if (self.auto_start): | ||
self.timer_start_time = time.monotonic() | ||
else: | ||
self.timer_start_time = -1 | ||
|
||
def start_timer(self) -> None: | ||
self.timer_start_time = time.time() | ||
|
||
def check_valid_timer(self) -> bool: | ||
if (self.timer_start_time == -1): | ||
raise RuntimeError(f"{self.name} timer checked but not started") | ||
|
||
def is_elapsed(self) -> bool: | ||
elapsed = self.get_elapsed() | ||
if (elapsed > self.timer_period_seconds): | ||
if (self.is_periodic): | ||
self.timer_start_time = time.time() | ||
return True | ||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain what this periodic clause would be used for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great point. It was more out of habit from previously implemented timers that I found the pattern of "one shot" vs "periodic" timer distinction helpful. Here I am not sure if there is much (or may even be negative) value add. Let me reflect for a moment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay on further reflection. You are 100% right that the timer within ActionWorker does not need to be periodic. I would however like to keep the notion of periodic timer in its current implementation for this use case:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it I might advocate for adding this when we need it, if we later discover we need it. Is the distinction between a stays-high signal and a periodic-break signal even relevant here in python land? We should design our "signals" in the way that works for our use case |
||
else: | ||
return False | ||
|
||
def get_elapsed(self) -> float: | ||
self.check_valid_timer() | ||
now = time.time() | ||
return now - self.timer_start_time |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,35 @@ def comp_cond(): | |
assert percentage_complete.value == 100 | ||
|
||
|
||
def test_action_node_timeout(): | ||
# For test | ||
percentage_complete = Value("i", 0) | ||
|
||
@wrapped_action_work(loop_period_sec=0.001, work_function_timeout_period_sec=.002) | ||
def work_func(comp_condition: Callable) -> Status: | ||
percentage_complete.value += 10 | ||
if comp_condition(): | ||
return Status.SUCCESS | ||
logger.debug(f"pct complete -> {percentage_complete.value}") | ||
return Status.RUNNING | ||
|
||
def comp_cond(): | ||
return percentage_complete.value >= 100 | ||
|
||
action = ActionNode(name="action", work_func=work_func, | ||
completion_condition=comp_cond) | ||
action.setup() | ||
|
||
while action.status not in ( | ||
Status.SUCCESS, | ||
Status.FAILURE, | ||
): | ||
time.sleep(0.01) | ||
action.tick_once() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this testing the right thing? The timeout is supposed to trigger within a tick right? Should the sleep be within the work function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test is correct, the sleep here is more like the tree's tick rate |
||
assert action.status == Status.FAILURE | ||
assert percentage_complete.value != 100 | ||
|
||
|
||
def test_condition_node(): | ||
def condition_fn(): | ||
return True | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import time | ||
|
||
import pytest | ||
|
||
from beams.sequencer.helpers.Timer import Timer | ||
|
||
|
||
def test_elapsed(): | ||
t = Timer(name="test_elapsed", | ||
timer_period_seconds=0.1, | ||
is_periodic=False) | ||
t.start_timer() | ||
assert not t.is_elapsed() | ||
time.sleep(0.5) | ||
assert t.is_elapsed() | ||
|
||
|
||
def test_timer_error(): | ||
t = Timer(name="test_error_not_started", | ||
timer_period_seconds=0.1, | ||
is_periodic=False) | ||
with pytest.raises(RuntimeError): | ||
t.get_elapsed() | ||
with pytest.raises(RuntimeError): | ||
t.is_elapsed() | ||
|
||
|
||
def test_periodic(): | ||
t = Timer(name="test_error_not_started", | ||
timer_period_seconds=0.1, | ||
auto_start=True, | ||
is_periodic=True) | ||
time.sleep(0.2) | ||
assert t.is_elapsed() | ||
assert not t.is_elapsed() | ||
time.sleep(0.1) | ||
assert t.is_elapsed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only check the timer at the top of the while loop, if a work function never completes / hangs forever, this won't actually timeout will it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, at present this doesn't have a mechanism to send a kill signal to a hung function. Within Worker we can tie these functionalities together... it is a good, deeper idea. The intention here was if futile work was being done, i.e. the function is returning but having no effect on the success condition, to time out that work