fix: replace bare except with typed exceptions in service.py#7297
fix: replace bare except with typed exceptions in service.py#7297harshadkhetpal wants to merge 1 commit intovoxel51:developfrom
Conversation
Bare `except:` clauses catch SystemExit and KeyboardInterrupt, masking critical signals. Fixed 3 occurrences: - Lines 252, 262: bare reraise pattern → except BaseException: - Line 317: swallow non-fatal error → except Exception:
|
Thanks for opening this pull request, @harshadkhetpal! 🎉 A maintainer will review your changes soon. In the meantime, please ensure:
Helpful resources: |
WalkthroughThe pull request modifies exception handling in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/core/service.py`:
- Around line 250-255: Replace the broad BaseException handlers around
etau.ensure_dir(database_dir) (and the similar block at lines 257-265) with a
specific filesystem exception catch (e.g., except OSError as e) and re-raise
PermissionError chaining the original exception (raise PermissionError("Database
directory `%s` cannot be written to" % database_dir) from e) so shutdown signals
like KeyboardInterrupt/SystemExit are not masked and the original cause is
preserved; locate the try/except blocks in fiftyone.core.service around
etau.ensure_dir to apply this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bea6690-3285-4174-87cd-171e60e3133f
📒 Files selected for processing (1)
fiftyone/core/service.py
| try: | ||
| etau.ensure_dir(database_dir) | ||
| except: | ||
| except BaseException: | ||
| raise PermissionError( | ||
| "Database directory `%s` cannot be written to" % database_dir | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the Python exception boundary this change relies on.
python - <<'PY'
try:
raise KeyboardInterrupt()
except BaseException:
print("BaseException catches KeyboardInterrupt")
try:
raise KeyboardInterrupt()
except Exception:
print("Exception catches KeyboardInterrupt")
except BaseException:
print("Exception lets KeyboardInterrupt propagate past Exception")
PYRepository: voxel51/fiftyone
Length of output: 157
🏁 Script executed:
# Read the relevant section of fiftyone/core/service.py
head -270 /dev/null 2>/dev/null || echo "checking file path"
find . -name "service.py" -path "*/fiftyone/core/*" -type f | head -5Repository: voxel51/fiftyone
Length of output: 86
🏁 Script executed:
# Check if the file exists and read the relevant lines
if [ -f "fiftyone/core/service.py" ]; then
echo "=== Lines 240-270 ==="
sed -n '240,270p' fiftyone/core/service.py
else
echo "File not found, searching for it"
find . -name "service.py" -type f
fiRepository: voxel51/fiftyone
Length of output: 968
🏁 Script executed:
# Search for etau module and ensure_dir/ensure_basedir/ensure_empty_file definitions
rg -A 10 "def ensure_dir|def ensure_basedir|def ensure_empty_file" --type pythonRepository: voxel51/fiftyone
Length of output: 90
🏁 Script executed:
# Search for etau module and ensure_* functions
rg "def ensure_dir|def ensure_basedir|def ensure_empty_file" --glob "*.py" | head -20Repository: voxel51/fiftyone
Length of output: 160
🏁 Script executed:
# Find where etau is imported from
rg "import.*etau|from.*etau" fiftyone/core/service.pyRepository: voxel51/fiftyone
Length of output: 89
🏁 Script executed:
# Check what exceptions etau functions can raise
rg -A 15 "def ensure_dir" --glob "*.py" | head -40Repository: voxel51/fiftyone
Length of output: 763
🏁 Script executed:
# Search for eta.core.utils usage and exception patterns in the codebase
rg "etau\." fiftyone/core/service.py -B 2 -A 5 | head -50Repository: voxel51/fiftyone
Length of output: 742
🏁 Script executed:
# Check if eta is a dependency and what version
cat setup.py pyproject.toml requirements.txt 2>/dev/null | grep -i etaRepository: voxel51/fiftyone
Length of output: 269
🏁 Script executed:
# Search for other exception handling patterns around filesystem operations in the codebase
rg "ensure_dir|ensure_basedir|ensure_empty_file" -B 3 -A 3 --glob "*.py" | head -80Repository: voxel51/fiftyone
Length of output: 4128
🏁 Script executed:
# Try to find eta package documentation or source in the repo
find . -name "*eta*" -type d | head -10Repository: voxel51/fiftyone
Length of output: 125
🏁 Script executed:
# Check for any try-except blocks around etau.ensure_* calls to see what exceptions are expected
rg -B 2 -A 5 "etau\.(ensure_dir|ensure_basedir|ensure_empty_file)" --glob "*.py" | grep -A 5 "except"Repository: voxel51/fiftyone
Length of output: 664
🏁 Script executed:
# Look for test cases that might show what exceptions are expected
rg "ensure_dir|ensure_basedir|ensure_empty_file" tests/ -A 3 -B 3 --glob "*.py" | head -60Repository: voxel51/fiftyone
Length of output: 1003
🏁 Script executed:
# Check eta package version and see if there's any documentation about exceptions
cat package/voxel51-eta/setup.py 2>/dev/null | head -50Repository: voxel51/fiftyone
Length of output: 42
🏁 Script executed:
# Search for any error handling documentation or patterns in the eta directory
find ./package/voxel51-eta -name "*.py" -type f 2>/dev/null | head -5Repository: voxel51/fiftyone
Length of output: 42
🏁 Script executed:
# Look for what exceptions filesystem operations typically raise
python3 << 'EOF'
import os
try:
# Simulate permission error
os.makedirs("/root/test_perm", exist_ok=True)
except Exception as e:
print(f"Type: {type(e).__name__}, bases: {type(e).__bases__}")
# Check OSError hierarchy
print(f"\nOSError subclasses include:")
for exc in [FileNotFoundError, PermissionError, IsADirectoryError]:
print(f" {exc.__name__}: {issubclass(exc, OSError)}")
EOFRepository: voxel51/fiftyone
Length of output: 215
🏁 Script executed:
# Check if we can find eta source code in the package directory
find ./package/voxel51-eta -name "utils.py" -o -name "core.py" 2>/dev/null | head -10Repository: voxel51/fiftyone
Length of output: 42
🏁 Script executed:
# Look at the entire structure of the eta package
ls -la ./package/voxel51-eta/ 2>/dev/nullRepository: voxel51/fiftyone
Length of output: 226
🏁 Script executed:
# Search for ensure_dir implementation in eta if available
rg "def ensure_dir" ./package/voxel51-eta --glob "*.py" 2>/dev/null || echo "Not found in eta package, it's likely an external dependency"Repository: voxel51/fiftyone
Length of output: 120
🏁 Script executed:
# Since eta is external, check if there's any documentation comment or exception specification in fiftyone storage.py
sed -n '1,50p' fiftyone/core/storage.py | head -30Repository: voxel51/fiftyone
Length of output: 539
BaseException still masks shutdown signals.
Lines 252 and 262 catch BaseException, which includes KeyboardInterrupt, SystemExit, and GeneratorExit. These signals will be rewritten as PermissionError, preventing clean shutdown. Catch the concrete filesystem exceptions from etau.ensure_* instead and preserve the original cause.
Suggested fix:
Exception handling changes
try:
etau.ensure_dir(database_dir)
- except BaseException:
+ except OSError as e:
raise PermissionError(
"Database directory `%s` cannot be written to" % database_dir
- )
+ ) from e
try:
etau.ensure_basedir(log_path)
if not os.path.isfile(log_path):
etau.ensure_empty_file(log_path)
- except BaseException:
+ except OSError as e:
raise PermissionError(
"Database log path `%s` cannot be written to" % log_path
- )
+ ) from eAlso applies to: 257-265
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 252-252: Do not catch blind exception: BaseException
(BLE001)
[warning] 253-255: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 254-254: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fiftyone/core/service.py` around lines 250 - 255, Replace the broad
BaseException handlers around etau.ensure_dir(database_dir) (and the similar
block at lines 257-265) with a specific filesystem exception catch (e.g., except
OSError as e) and re-raise PermissionError chaining the original exception
(raise PermissionError("Database directory `%s` cannot be written to" %
database_dir) from e) so shutdown signals like KeyboardInterrupt/SystemExit are
not masked and the original cause is preserved; locate the try/except blocks in
fiftyone.core.service around etau.ensure_dir to apply this change.
2 participants
Summary
Replace 3 bare
except:clauses with typed exceptions infiftyone/core/service.py.Bare
except:intercepts all exceptions includingSystemExit,KeyboardInterrupt, andGeneratorExit, which can suppress critical signals and make services impossible to stop cleanly.Changes:
Testing
No behavior change — correctness/style fix only. Reraise blocks preserve the same semantics; the swallow block now correctly allows
KeyboardInterruptto propagate.Summary by CodeRabbit