fix(label): raise CSVError instead of sys.exit on duplicate identifier#65
fix(label): raise CSVError instead of sys.exit on duplicate identifier#65gkorland wants to merge 3 commits into
CSVError instead of sys.exit on duplicate identifier#65Conversation
Calling `sys.exit(1)` from within a library method raises `SystemExit`, which is awkward to catch (callers must explicitly handle a base-class exception) and makes unit testing more difficult. Raise a domain `CSVError` with a descriptive message instead. The CLI entry point already lets exceptions propagate and exits non-zero on uncaught errors, so user-visible behaviour for the default (`skip_invalid_nodes=False`) case is preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughCSV validation error handling is refactored to use exceptions instead of direct process termination. When duplicate node identifiers or CSV parsing failures occur, Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Copilot Code Review✅ No significant issues found. |
When label.py raises CSVError on a duplicate node identifier, the exception now propagates to the CLI entry point where it is caught, printed to stderr, and converted to sys.exit(1). This restores the clean non-zero exit behaviour that existed when sys.exit(1) was called directly from label.py, and fixes the failing test_reused_identifier test that uses catch_exceptions=False. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
034bb36 to
709ae57
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #65 +/- ##
==========================================
+ Coverage 89.52% 89.57% +0.05%
==========================================
Files 9 9
Lines 611 614 +3
==========================================
+ Hits 547 550 +3
Misses 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Agent Review SummaryPR Assessment ✅PR title: This PR correctly replaces Issue Found & Fixed 🔧CI was failing (Test with Python 3.10 & 3.11) because Root cause: Fix applied (commit 709ae57): In try:
process_entities(labels)
process_entities(reltypes)
except CSVError as e:
sys.exit(str(e))Using CI Results ✅All 9 checks now pass:
|
|
@copilot resolve the merge conflicts in this pull request |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
…dling with debug logging Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Merge conflicts resolved in eb19db3. The only conflict was in |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve library-usage and testability by removing a sys.exit(1) call from Label.update_node_dictionary() and replacing it with a typed exception (CSVError) when a duplicate node identifier is encountered.
Changes:
- Raise
CSVError(instead of callingsys.exit) on duplicate node identifiers inLabel.update_node_dictionary. - Import
CSVErrorinlabel.py. - Catch
CSVErrorinbulk_insertand terminate the CLI run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
falkordb_bulk_loader/label.py |
Replaces sys.exit(1) with raising CSVError for duplicate node IDs. |
falkordb_bulk_loader/bulk_insert.py |
Adds CSVError handling around entity processing to exit on CSV-level errors. |
Comments suppressed due to low confidence (2)
falkordb_bulk_loader/bulk_insert.py:231
sys.exit(str(e))inside a Click command is likely to not display the error message to the user (Click catchesSystemExitand typically only uses it for the exit code). If the goal is to surface theCSVErrormessage on stderr while exiting non-zero, prefer converting it to a Click-friendly exception (e.g., raise aclick.ClickException) or explicitly echo the message to stderr and then exit with a numeric status code.
module_list = [m["name"] for m in client.connection.module_list()]
if "graph" not in module_list:
logger.error("falkordb module not loaded on connected server.")
sys.exit(1)
logger.debug("FalkorDB module is loaded on the server.")
falkordb_bulk_loader/bulk_insert.py:231
- The PR description says an "unhandled
CSVError" will still terminate the process, but this change explicitly catchesCSVErrorand converts it toSystemExit. Either update the PR description to reflect this behavior change, or adjust the implementation to match the stated intent (e.g., letCSVErrorpropagate and be handled consistently elsewhere).
module_list = [m["name"] for m in client.connection.module_list()]
if "graph" not in module_list:
logger.error("falkordb module not loaded on connected server.")
sys.exit(1)
logger.debug("FalkorDB module is loaded on the server.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| % (identifier, self.infile.name, self.reader.line_num) | ||
| ) | ||
| sys.stderr.flush() | ||
| if self.config.skip_invalid_nodes is False: | ||
| sys.exit(1) | ||
| raise CSVError( |
![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 `@falkordb_bulk_loader/bulk_insert.py`:
- Around line 260-266: The CSVError except block is too narrow—expand the
try/except to include the parse_schemas(...) call so any CSVError raised during
schema parsing or during process_entities(labels)/process_entities(reltypes) is
caught; specifically wrap the call to parse_schemas(...) along with the
logger.debug("Processing nodes...")/process_entities(labels) and
logger.debug("Processing relations...")/process_entities(reltypes) in the same
try and keep the existing except CSVError as e: sys.exit(str(e)) behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43bde68f-c1bc-4cef-b5d8-cbd704daf61c
📒 Files selected for processing (2)
falkordb_bulk_loader/bulk_insert.pyfalkordb_bulk_loader/label.py
| try: | ||
| logger.debug("Processing nodes...") | ||
| process_entities(labels) | ||
| logger.debug("Processing relations...") | ||
| process_entities(reltypes) | ||
| except CSVError as e: | ||
| sys.exit(str(e)) |
There was a problem hiding this comment.
CSVError handling scope is too narrow for consistent CLI behavior.
Line 260 only wraps process_entities(...). parse_schemas(...) (Lines 252-257) can also raise CSVError (e.g., relation/header validation), which would still escape uncaught in CliRunner(catch_exceptions=False) scenarios.
Suggested fix
- # Read the header rows of each input CSV and save its schema.
- logger.debug("Parsing node CSV schemas...")
- labels = parse_schemas(Label, query_buf, nodes, nodes_with_label, config)
- logger.debug(f"Parsed {len(labels)} node CSV file(s).")
- logger.debug("Parsing relation CSV schemas...")
- reltypes = parse_schemas(
- RelationType, query_buf, relations, relations_with_type, config
- )
- logger.debug(f"Parsed {len(reltypes)} relation CSV file(s).")
-
- try:
+ try:
+ # Read the header rows of each input CSV and save its schema.
+ logger.debug("Parsing node CSV schemas...")
+ labels = parse_schemas(Label, query_buf, nodes, nodes_with_label, config)
+ logger.debug(f"Parsed {len(labels)} node CSV file(s).")
+ logger.debug("Parsing relation CSV schemas...")
+ reltypes = parse_schemas(
+ RelationType, query_buf, relations, relations_with_type, config
+ )
+ logger.debug(f"Parsed {len(reltypes)} relation CSV file(s).")
+
logger.debug("Processing nodes...")
process_entities(labels)
logger.debug("Processing relations...")
process_entities(reltypes)
except CSVError as e:
sys.exit(str(e))📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| logger.debug("Processing nodes...") | |
| process_entities(labels) | |
| logger.debug("Processing relations...") | |
| process_entities(reltypes) | |
| except CSVError as e: | |
| sys.exit(str(e)) | |
| try: | |
| # Read the header rows of each input CSV and save its schema. | |
| logger.debug("Parsing node CSV schemas...") | |
| labels = parse_schemas(Label, query_buf, nodes, nodes_with_label, config) | |
| logger.debug(f"Parsed {len(labels)} node CSV file(s).") | |
| logger.debug("Parsing relation CSV schemas...") | |
| reltypes = parse_schemas( | |
| RelationType, query_buf, relations, relations_with_type, config | |
| ) | |
| logger.debug(f"Parsed {len(reltypes)} relation CSV file(s).") | |
| logger.debug("Processing nodes...") | |
| process_entities(labels) | |
| logger.debug("Processing relations...") | |
| process_entities(reltypes) | |
| except CSVError as e: | |
| sys.exit(str(e)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@falkordb_bulk_loader/bulk_insert.py` around lines 260 - 266, The CSVError
except block is too narrow—expand the try/except to include the
parse_schemas(...) call so any CSVError raised during schema parsing or during
process_entities(labels)/process_entities(reltypes) is caught; specifically wrap
the call to parse_schemas(...) along with the logger.debug("Processing
nodes...")/process_entities(labels) and logger.debug("Processing
relations...")/process_entities(reltypes) in the same try and keep the existing
except CSVError as e: sys.exit(str(e)) behavior.
Summary
Label.update_node_dictionarycalledsys.exit(1)when it encountered a duplicate node identifier and--skip-invalid-nodeswas not set.sys.exitfrom a library function raisesSystemExit(aBaseException), which is awkward for callers to catch and makes unit testing more difficult.Changes
falkordb_bulk_loader/label.py: raiseCSVErrorwith a descriptive "Duplicate node identifier '' at :" message instead ofsys.exit(1).CSVErrorto the existingfrom .exceptions import …line.Testing
Lint clean. CLI behaviour is preserved: an unhandled
CSVErrorstill terminates the process with a non-zero exit code, and the same descriptive line is also written to stderr (existing message unchanged).Memory / Performance Impact
N/A.
Related Issues
From the comprehensive code-review report (BUG-11). Note: this PR addresses BUG-11 only; the related BUG-1 (the duplicate-node row is also still inserted, corrupting the ID mapping when
skip_invalid_nodes=True) is handled separately.Summary by CodeRabbit