Hide traceback when automatic annotation has failed #8636
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request focus on enhancing error handling and control flow across multiple files in the CVAT project. Key modifications include the introduction of more robust error reporting mechanisms, simplification of error handling logic, and updates to the structure of various methods to improve clarity and maintainability. Notable alterations involve the removal of functions, updates to existing methods, and adjustments in how error messages are generated and reported in both the backend and frontend components. Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
cvat-core/src/lambda-manager.ts (1)
174-182: Consider enhancing error recovery for status retrieval failures.While the current error handling is good, consider these improvements:
- Add retry logic for transient network errors
- Provide more specific error messages based on error type (network, server, etc.)
Example enhancement:
let retries = 3; const fetchStatus = async (): Promise<void> => { try { const response = await serverProxy.lambda.status(requestID); // existing success handling } catch (error: unknown) { if (retries > 0 && isNetworkError(error)) { retries--; setTimeout(fetchStatus, 1000); return; } // existing error handling with specific messages const message = error instanceof Error ? (isNetworkError(error) ? 'Network error' : error.message) : 'Unknown error'; onUpdate.forEach((update) => update(RQStatus.UNKNOWN, 0, message)); } };cvat-ui/src/actions/models-actions.ts (1)
141-141: Improved error handling for failed automatic annotations.The change effectively wraps the error message in a new Error object, which helps standardize error handling and aligns with the PR objective of hiding traceback information from users.
Consider adding a custom error class to better distinguish automatic annotation failures from other errors:
class AutoAnnotationError extends Error { constructor(message: string) { super(message); this.name = 'AutoAnnotationError'; } } // Usage at line 141: new AutoAnnotationError(message as string),This would make it easier to:
- Filter and handle automatic annotation errors specifically
- Add additional context or metadata if needed in the future
- Maintain consistent error handling across the codebase
cvat-core/src/server-proxy.ts (1)
Line range hint
177-209: Consider scope of error message changesThe simplified error handling affects all server errors, not just automatic annotation failures. While this provides more direct error information, consider:
- Adding a sanitization layer for sensitive information in production
- Documenting the expected error message format for backend developers
Consider introducing an error message sanitizer that can be toggled based on the environment (development/production) to control the level of detail in error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cvat-core/src/lambda-manager.ts(1 hunks)cvat-core/src/server-proxy.ts(1 hunks)cvat-ui/src/actions/models-actions.ts(1 hunks)cvat-ui/src/reducers/notifications-reducer.ts(1 hunks)cvat/apps/engine/views.py(1 hunks)cvat/apps/lambda_manager/views.py(1 hunks)
🔇 Additional comments (10)
cvat-core/src/lambda-manager.ts (3)
152-152: LGTM: Improved progress handling with nullish coalescing.
The change from || to ?? operator is more precise for handling undefined progress values while preserving valid zero progress states.
158-164: LGTM: Enhanced error reporting with clearer messages.
The changes improve error handling by:
- Using appropriate progress defaults (100 for finished, 0 for failed)
- Including exception info in a user-friendly format without raw traceback
166-170: LGTM: Robust handling of unexpected states.
Good defensive programming practice with clear error messaging and appropriate progress defaults.
cvat/apps/lambda_manager/views.py (2)
626-632: LGTM! Error handling improvement aligns with PR objectives.
The changes effectively implement the PR's goal of hiding raw tracebacks by:
- Mapping DEFERRED status to QUEUED for consistency
- Using formatted exceptions from metadata instead of raw tracebacks when available for failed jobs
This improves the user experience by showing more user-friendly error messages.
630-632: Verify security implications of error messages.
While using formatted exceptions improves UX, ensure that the formatted messages in RQJobMetaField.FORMATTED_EXCEPTION don't contain sensitive information like stack traces, internal paths, or system details.
✅ Verification successful
Let me check how the exceptions are formatted by looking at the parse_exception_message function.
Let me try a different approach to find the exception formatting implementation using ripgrep.
Exception handling is secure and properly sanitized
The implementation of parse_exception_message shows that the formatted exceptions only extract user-friendly error messages from REST framework exceptions, removing any sensitive information. The function:
- Specifically handles REST framework validation errors
- Extracts only the error message string
- Strips out implementation details and stack traces
- Has a safe fallback to the original message if parsing fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential sensitive information in formatted exceptions
rg -g '!*.pyc' -g '!*.min.js' "FORMATTED_EXCEPTION.*=.*" -A 5
Length of output: 816
Script:
#!/bin/bash
# Find the parse_exception_message implementation
ast-grep --pattern 'def parse_exception_message($_) { $$$ }'
Length of output: 62
Script:
#!/bin/bash
# Search for parse_exception_message function definition and usage
rg -g '*.py' "def parse_exception_message" -B 2 -A 10
Length of output: 857
cvat-ui/src/reducers/notifications-reducer.ts (2)
888-888: LGTM! Error message simplified for better UX.
The simplified error message for automatic annotation failure aligns well with the PR objective to hide traceback errors from users. This change improves user experience by providing clearer, less technical feedback.
Line range hint 888-894: Verify error handling consistency.
The error handling implementation maintains the established pattern:
- Preserves error logging through
shouldLog - Retains task-specific context via task ID link
- Follows the standard error object structure
Consider documenting these error handling patterns in the codebase to ensure consistent implementation across the application.
✅ Verification successful
Error handling implementation follows consistent pattern across codebase
The error handling implementation in the modified code follows the established pattern used throughout the notifications reducer:
- All error states consistently use the same structure with
message,reason, andshouldLogproperties - The
shouldLogfunction is used uniformly across all error cases (70+ occurrences) to determine logging behavior - Error messages maintain a consistent format, including task-specific context via task ID links where applicable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns to ensure consistency
rg -U "message:.*reason:.*shouldLog:" --type ts
Length of output: 49
Script:
#!/bin/bash
# Let's try a more flexible pattern to find error handling structures
rg -U "message.*error.*shouldLog" --type ts -A 2
Length of output: 284
Script:
#!/bin/bash
# Search for notification state handling patterns
ast-grep --pattern 'state = {
$$$
errors: {
$$$
}
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Look for error object structures
rg "errors: \{" --type ts -A 5
Length of output: 49120
Script:
#!/bin/bash
# Find shouldLog usage patterns
rg "shouldLog\(" --type ts -A 2
Length of output: 24794
cvat-core/src/server-proxy.ts (2)
Line range hint 177-209: LGTM: Error handling changes look good
The simplified error handling implementation:
- Successfully achieves the goal of hiding traceback
- Maintains proper error type differentiation
- Preserves the existing error handling structure
177-177: Verify error message handling for 500 errors
The change to directly use response data for 500 errors aligns with the PR objective to simplify error handling. However, we should ensure that the error messages displayed to users are still user-friendly and don't expose sensitive implementation details.
✅ Verification successful
Let me gather more information about the error handling implementation to ensure a comprehensive verification.
Based on the comprehensive analysis of the error handling implementation, I can now provide the final response:
Error handling implementation is appropriate and secure
The direct usage of error response data in ServerError is safe because:
- The error handling is properly structured with different cases (400s, 500s)
- Error messages are appropriately formatted and sanitized where needed (using toString() for field errors)
- The UI layer (
cvat-ui) has additional error handling logic that filters out sensitive errors (codes 0, 400, 401, 403, 404, 429, 500) - The error messages are consistently wrapped in
ServerErrorclass which provides a controlled interface for error information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing error message formatting or sanitization
rg -g '*.ts' -g '*.tsx' 'errorData\.response\.data.*toString\(\)' -A 3
Length of output: 771
Script:
#!/bin/bash
# Check the complete error handling implementation
ast-grep --pattern 'function generateError($_) {
$$$
}'
# Look for any other error message processing
rg -g '*.ts' -g '*.tsx' 'ServerError|Error.*message' -A 3
Length of output: 15365
cvat/apps/engine/views.py (1)
3255-3260: Error handling enhancement looks good
The rq_exception_handler function correctly formats and parses the exception message before saving it to the job metadata. This aligns with the PR objective of hiding tracebacks from users when automatic annotation fails.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8636 +/- ##
===========================================
+ Coverage 74.28% 74.30% +0.01%
===========================================
Files 401 401
Lines 43395 43402 +7
Branches 3945 3945
===========================================
+ Hits 32237 32250 +13
+ Misses 11158 11152 -6
|
|
Motivation and context
Now it shows the error message when automatic annotations has failed:
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores