ophyd
ophyd copied to clipboard
bluesky
Discuss: Issue a warning if a Status' default timeout is None?
There is no universal good default value for timeout. If a timeout is too large, it delays notification of failure and leaves resources dangling. If a timeout is too short, it could needlessly kill a long task and force a costly restart.
I believe that we used to use default finite timeouts in some places, but after a couple incidents of long operations wrongly interrupted, we have (mostly?) stripped them out of the ophyd codebase, defaulting instead to timeout=None, which we take to mean "infinite". Pervasive use of infinite timeouts is not ideal. Perhaps we should discourage it by issuing a warning to the effect:
The Status {self!r} has been created with timeout=None.
This means that it will wait forever to finish.
It is better to set a specific maximum time that it is allowed to complete
so that it will fail with a clear error beyond that time.
If you really want to wait forever,
use math.inf instead of None to suppress this warning.
When can users choose a timeout?
- Users can choose it in the class definition, as in
class EpicsMotor(Device):
def set(self, ...):
status = DeviceStatus(self, timeout=SOME_HARD_CODED_TIMEOUT)
...
return status
For classes like EpicsMotor that can correspond to a wide range of actual hardware configured in various ways, this is too blunt an approach. Some actions with an EpicsMotor are much longer than others.
- We can choose it at instantiation time, as in
class EpicsMotor(Device):
def __init__(self, ..., timeout, ...):
self.timeout = timeout
def set(self, ...):
status = DeviceStatus(self, timeout=self.timeout)
...
return status
a_particular_motor = EpicsMotor('...', timeout=A_PARTICULAR_TIMEOUT)
That may be good enough if the motor in question performs movements with a well-defined maximum possible time to complete.
- We can choose a timeout per a particular action. This is when we have the most information about how long we should expect it to take. For example, for a motor, we can take into account the current velocity setting, the current position, and the distance to the target position. We don't have to take this into account, but we could.
class ParticularEpicsMotor(EpicsMotor):
def set(self, ...):
timeout = decision_based_on_how_far_we_have_to_go(...)
status = DeviceStatus(self, timeout=timeout)
...
return status
Should we use a warning to nudge users toward (2) and (3)? For EpicsMotor, this means that if it is instantiated without a timeout, it will issue that warning when it is moved.
This reminds me of how the library trio does not allow infinite Queues, unlike Python queue.Queue which does. It's better to demand a specific upper bound from the user. We might even consider converting this warning to an error after a very long deprecation period.
I think a warning is appropriate here, as long as it has some identifying information about which Device is responsible for it when possible. I'm not sure if it will be successful in changing behavior though, it might just push people to learn how to filter warnings.
Haha, yeah. And along those lines I think providing the math.inf escape hatch could be an important part of this.
None warning + math.inf escape-hatch sounds like a good plan to me. I share @ZLLentz's concern but see no viable alternative.
OK. I'm inclined to let this one sit awhile, maybe until after the next NSLS rollout, just because I'm wary of a bombardment of deprecation warnings (thinking of that .value one as well). But I think this is worth doing within the next 6 months.
Definitely. I'm concerned about the impact at beam lines with dozens of motors, most with defaults. As I understand it now, a beam line would subclass EpicsMotor and specify the desired timeout parameter at that point, then use MyEpicsMotor instead?
I think subclassing would be uncommon. If a constant timeout is good enough then Approach (2) above is fine, i.e.
some_motor = EpicsMotor(PV, name=..., timeout=...)
It’s only if someone wants smarter “adaptive” timeouts that are more context-aware that they would resort to Approach (3), a subclass.