Skip to content

fix: replace 7 bare except clauses with except Exception in server/query.py#7300

Open
harshadkhetpal wants to merge 1 commit intovoxel51:developfrom
harshadkhetpal:fix/bare-except-server-query
Open

fix: replace 7 bare except clauses with except Exception in server/query.py#7300
harshadkhetpal wants to merge 1 commit intovoxel51:developfrom
harshadkhetpal:fix/bare-except-server-query

Conversation

@harshadkhetpal
Copy link
Copy Markdown
Contributor

@harshadkhetpal harshadkhetpal commented Apr 2, 2026

Summary

Replace 7 bare except: clauses with except Exception: in fiftyone/server/query.py.

Bare except: catches all exceptions including SystemExit, KeyboardInterrupt, and GeneratorExit. In a server query module these broad catches can silently swallow signals that should terminate the process. except Exception: is the correct form for "catch any non-fatal error."

All 7 changes follow this pattern:

# Before
    except:
        pass   # or return None / return BrainRunType / fallback

# After
    except Exception:
        pass   # or return None / return BrainRunType / fallback

Affected lines: 122, 144, 520, 621 (and 3 more)

Testing

No behavior change — correctness/style fix only. All exception-swallowing blocks continue to catch non-fatal errors as intended while allowing KeyboardInterrupt to propagate correctly.

Summary by CodeRabbit

  • Refactor
    • Improved internal exception handling robustness to better capture and manage specific error conditions during query processing and dataset operations.

7 bare except: clauses replaced with Exception or BaseException
based on whether the block re-raises.
@harshadkhetpal harshadkhetpal requested a review from a team as a code owner April 2, 2026 23:45
@voxel51-bot
Copy link
Copy Markdown
Collaborator

Thanks for opening this pull request, @harshadkhetpal! 🎉

A maintainer will review your changes soon. In the meantime, please ensure:

  • You filled out the PR template above, completely to the best of your knowledge
  • You include tests (if applicable)
  • You read the contribution guide

Helpful resources:

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

Walkthrough

This pull request replaces seven bare except: clauses with except Exception: throughout fiftyone/server/query.py. The replacements occur in exception handlers across multiple methods: BrainRunConfig.type(), _create_config(), Query.saved_views(), and serialize_dataset(). The change narrows exception catching to exclude non-Exception types like SystemExit and KeyboardInterrupt. No control flow or functional behavior is altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers the what and why, but is missing the required template sections for Related Issues, Testing details, and Release Notes. Fill in the missing template sections: add Related Issues link, expand Testing section with test coverage details, and specify Release Notes impact (user-facing or not).
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: replacing 7 bare except clauses with except Exception in server/query.py.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/server/query.py`:
- Around line 675-678: The local variable named `type` shadows Python's builtin;
rename it (e.g., to `run_type`) wherever it's assigned and used around the brain
method config retrieval (the expression brain_method.config.type().value and its
subsequent fallback to None) and update all usages (including the places noted
near the assignment and later use around the code that references this value) so
references match the new name; ensure imports or other symbols are unaffected
and run linters/tests to confirm Ruff A001 is resolved.
🪄 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: 25d18e74-2a3f-456f-9958-6fdd48e8d9d4

📥 Commits

Reviewing files that changed from the base of the PR and between bd3db37 and 80c7e1b.

📒 Files selected for processing (1)
  • fiftyone/server/query.py

Comment on lines 675 to 678
try:
type = brain_method.config.type().value
except:
except Exception:
type = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify remaining builtin-shadowing assignments in this file
rg -nP -C2 '\btype\s*=' fiftyone/server/query.py

Repository: voxel51/fiftyone

Length of output: 284


🏁 Script executed:

#!/bin/bash
# Get broader context around lines 675-678, including any setattr calls
sed -n '670,690p' fiftyone/server/query.py | cat -n

Repository: voxel51/fiftyone

Length of output: 834


🏁 Script executed:

#!/bin/bash
# Search for the setattr call mentioned in the review
rg -n 'setattr.*type' fiftyone/server/query.py | head -20

Repository: voxel51/fiftyone

Length of output: 118


🏁 Script executed:

#!/bin/bash
# Get the complete context of this function block to see all type variable usages
sed -n '674,710p' fiftyone/server/query.py | cat -n

Repository: voxel51/fiftyone

Length of output: 1385


🏁 Script executed:

#!/bin/bash
# Verify there are no other occurrences of type variable in this function scope
# and confirm the proposed fix covers all usages
rg -n '\btype\b' fiftyone/server/query.py | grep -A2 -B2 '67[0-9]\|69[0-9]'

Repository: voxel51/fiftyone

Length of output: 238


Rename local type to avoid builtin shadowing.

type shadows Python's builtin and is flagged by Ruff (A001). Rename it (e.g., run_type) at lines 676, 678, and 692 where the variable is assigned and used.

Proposed fix
             try:
-                type = brain_method.config.type().value
+                run_type = brain_method.config.type().value
             except Exception:
-                type = None
+                run_type = None
 
             try:
                 max_k = brain_method.config.max_k()
             except Exception:
                 max_k = None
 
             try:
                 supports_least_similarity = (
                     brain_method.config.supports_least_similarity()
                 )
             except Exception:
                 supports_least_similarity = None
 
-            setattr(brain_method.config, "type", type)
+            setattr(brain_method.config, "type", run_type)
             setattr(brain_method.config, "max_k", max_k)
🧰 Tools
🪛 Ruff (0.15.7)

[error] 676-676: Variable type is shadowing a Python builtin

(A001)


[warning] 677-677: Do not catch blind exception: Exception

(BLE001)


[error] 678-678: Variable type is shadowing a Python builtin

(A001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@fiftyone/server/query.py` around lines 675 - 678, The local variable named
`type` shadows Python's builtin; rename it (e.g., to `run_type`) wherever it's
assigned and used around the brain method config retrieval (the expression
brain_method.config.type().value and its subsequent fallback to None) and update
all usages (including the places noted near the assignment and later use around
the code that references this value) so references match the new name; ensure
imports or other symbols are unaffected and run linters/tests to confirm Ruff
A001 is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants