-
Notifications
You must be signed in to change notification settings - Fork 87
Add custom timeout exception to be raised upon timeout in status.wait(timeout=2...) #1263
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @ZLLentz as discussed during the ICALEPCS Bluesky workshop and Poster session, I prepare 3 little PRs. This is one of them, please let me know if there are things to adapt. |
ZLLentz
left a comment
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.
I'm a fan of this and would be in favor of merging as-is.
Since this is backwards compatible and we already use a variety of exception types in these classes, I see no reason to reject it. I could find myself using this to provide more precise feedback for timeout errors in certain cases.
| """ | ||
| exception_to_raise = ( | ||
| timeout_exception | ||
| if isinstance(timeout_exception, Exception) |
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.
One thing to consider: we might want to fail loudly with a TypeError if the user passes a non-exception non-None here.
| timeout: Union[Number, None], optional | ||
| If None (default) wait indefinitely until the status finishes. | ||
| timeout_exception: Exception, optional |
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.
This should be the class to raise not an instantiated exception.
| indicates that the action itself raised ``TimeoutError``, distinct | ||
| from ``WaitTimeoutError`` above. | ||
| """ | ||
| exception_to_raise = ( |
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.
Creating the exception grabs things about the current stack, we should not create it until we need it.
tacaswell
left a comment
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.
I have concerns about passing in an instantiated exception object.
|
I'm also not convinced by the use-case of avoiding try-except. I would be much more sold on the ability to raise a custom sub-class that let you do better filtering in try-except. I would also expect this to be set at Status init time not at wait time. Devices have control over how the the status objects are created but no control over how they are waited on. If the goal is to get better text in the error messages, then a better option would be to let return DeviceStatus(self, wait_msg_func=lambda sts, timeout: f'{sts.device.name} did not complete trigger in {timeout}s') |
I agree with your statement that this should be moved to when the Status is created, and apply to any TimoutException raised by the status object. My initial thought was to make the PR as simple as possible, and therefore only adapt The use-case in particular is that I create a status object with a pending timeout, or a status which will be awaited for later on in a different section of the code. If the status runs into a timeout exception, I would like to have more control about what is raised. So in short: (A) a descriptive error, and (B) a custom error message. |
Summary
This PR adds the option to specify a custom exception to the .wait() method.
Use Case
If you want to raise a specific Exception upon running into a Timeout during the
.wait(timeout=...)method, a try/except statement is needed. This PR adds the option to in addition specify a 'timeout_exception' to the.wait(timeout=5, timeout_exception=...)method of StatusBase. This allows the simpler syntax below, without try/except.