Graceful failing with seeking errors #1712
Conversation
WalkthroughThe recent modifications focus on enhancing the resilience and efficiency of frame handling in a video processing library. The first change ensures that the initial frame is always selected for processing, optimizing start-up times. The second adjustment introduces error handling for frame loading issues, improving the user experience by preventing abrupt interruptions during interactive prediction tasks. These changes collectively aim to streamline video analysis workflows. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| test_image = tf.convert_to_tensor( | ||
| self.video.get_frame(self.video.last_frame_idx) | ||
| ) | ||
| test_image = tf.convert_to_tensor(self.video.get_frame(0)) |
There was a problem hiding this comment.
Using 0 as the argument for self.video.get_frame to fetch a test image is a straightforward approach to avoid seeking errors. However, it would be beneficial to add a comment explaining this choice for future maintainability.
+ # Fetching the first frame (index 0) to avoid seeking errors, especially towards the end of the video.
test_image = tf.convert_to_tensor(self.video.get_frame(0))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test_image = tf.convert_to_tensor(self.video.get_frame(0)) | |
| # Fetching the first frame (index 0) to avoid seeking errors, especially towards the end of the video. | |
| test_image = tf.convert_to_tensor(self.video.get_frame(0)) |
sleap/nn/inference.py
Outdated
|
|
||
| except KeyError as e: | ||
| # Gracefully handle seeking errors by early termination. | ||
| if "Unable to load frame" in str(e): | ||
| pass # TODO: Print warning obeying verbosity? (This code path is also | ||
| # called for interactive prediction where we don't want any spam.) | ||
| else: | ||
| raise | ||
|
|
||
|
|
There was a problem hiding this comment.
The code attempts to handle seeking errors gracefully by checking for a specific error message. However, this approach might not be robust as it relies on the error message string, which could change. Consider using exception types for more reliable error handling.
- if "Unable to load frame" in str(e):
+ if isinstance(e, FrameLoadError):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if "Unable to load frame" in str(e): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if isinstance(e, FrameLoadError): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
sleap/nn/inference.py
Outdated
|
|
||
| except KeyError as e: | ||
| # Gracefully handle seeking errors by early termination. | ||
| if "Unable to load frame" in str(e): | ||
| pass # TODO: Print warning obeying verbosity? (This code path is also | ||
| # called for interactive prediction where we don't want any spam.) | ||
| else: | ||
| raise | ||
|
|
There was a problem hiding this comment.
Similar to the previous comment, the error handling here also relies on string matching. It's recommended to use exception types for error handling.
- if "Unable to load frame" in str(e):
+ if isinstance(e, FrameLoadError):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if "Unable to load frame" in str(e): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if isinstance(e, FrameLoadError): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
sleap/nn/inference.py
Outdated
|
|
||
| except KeyError as e: | ||
| # Gracefully handle seeking errors by early termination. | ||
| if "Unable to load frame" in str(e): | ||
| pass # TODO: Print warning obeying verbosity? (This code path is also | ||
| # called for interactive prediction where we don't want any spam.) | ||
| else: | ||
| raise | ||
|
|
There was a problem hiding this comment.
Again, the error handling strategy based on string matching is observed. Using exception types would enhance the robustness of the error handling mechanism.
- if "Unable to load frame" in str(e):
+ if isinstance(e, FrameLoadError):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if "Unable to load frame" in str(e): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if isinstance(e, FrameLoadError): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
sleap/nn/inference.py
Outdated
|
|
||
| except KeyError as e: | ||
| # Gracefully handle seeking errors by early termination. | ||
| if "Unable to load frame" in str(e): | ||
| pass # TODO: Print warning obeying verbosity? (This code path is also | ||
| # called for interactive prediction where we don't want any spam.) | ||
| else: | ||
| raise | ||
|
|
There was a problem hiding this comment.
The error handling pattern based on string matching is repeated here. It's advisable to use exception types for more reliable error handling.
- if "Unable to load frame" in str(e):
+ if isinstance(e, FrameLoadError):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if "Unable to load frame" in str(e): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
| except KeyError as e: | |
| # Gracefully handle seeking errors by early termination. | |
| if isinstance(e, FrameLoadError): | |
| pass # TODO: Print warning obeying verbosity? (This code path is also | |
| # called for interactive prediction where we don't want any spam.) | |
| else: | |
| raise | |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1712 +/- ##
===========================================
- Coverage 73.38% 73.31% -0.07%
===========================================
Files 134 134
Lines 24064 24088 +24
===========================================
+ Hits 17659 17660 +1
- Misses 6405 6428 +23 ☔ View full report in Codecov by Sentry. |
Description
This PR mitigates the longstanding seekability issue. See #1711 for context.
This PR does the simplest thing possible: handling this error by gracefully exiting inference. This should fix many use cases where the seeking error happens towards the end of the video due to inaccurate estimation of the true frame count.
Unfortunately, I don't have any small CI-friendly video files that can be used to write a test, but it's worked locally and the logic is fairly straightforward enough that it shouldn't introduce any weird behavior.
Types of changes
Does this address any currently open issues?
#1711
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit