Add entity deduplication and extraction validation for improved knowledge graph quality #138
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces entity deduplication and extraction validation features to GraphRAG-SDK through two new modules: EntityResolver for normalization and duplicate detection, and ExtractionValidator for ontology-aware validation. Configuration parameters are added to enable/disable these features, integration points expand the extraction pipeline, and comprehensive examples and tests demonstrate the functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KG as KnowledgeGraph
participant EDS as ExtractDataStep
participant Val as ExtractionValidator
participant Res as EntityResolver
participant Storage as Graph Storage
User->>KG: process_sources(sources, enable_deduplication=True, enable_validation=True)
KG->>EDS: Initialize with enable_deduplication & enable_validation
EDS->>EDS: Initialize EntityResolver (if dedup enabled)
EDS->>EDS: Initialize ExtractionValidator (if validation enabled)
loop For each document
EDS->>EDS: Extract data from source
opt Validation Enabled
EDS->>Val: validate_extraction(data)
Val->>Val: Validate entities against ontology
Val->>Val: Validate relations & compute quality scores
Val-->>EDS: Return validated_data & validation_report
EDS->>EDS: Filter invalid entries, log warnings
end
opt Deduplication Enabled
EDS->>Res: normalize_entity_attributes(entity) for each entity
EDS->>Res: deduplicate_entities(normalized_entities, unique_attrs)
Res->>Res: Compare entities via similarity matching
Res->>Res: Merge duplicates, count removals
Res-->>EDS: Return deduplicated_entities & removal_count
EDS->>EDS: Log deduplication metrics
end
EDS->>Storage: Create entities & relations in graph
end
EDS-->>KG: Processing complete with quality report
KG-->>User: Return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| print(f"\nOriginal entities: {len(entities)}") | ||
| for i, entity in enumerate(entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To address the issue, sensitive data such as names and birth dates should not be logged in clear text. In this context, redacting or masking sensitive fields in log statements (e.g., replacing the full name with [REDACTED] or using only non-sensitive identifiers) is appropriate. Edit the print statements in demonstrate_entity_resolution to avoid outputting the actual values of sensitive fields ("name" and "birth_date"). Since this is a demonstration, consider showing only non-sensitive placeholders, or indicate in the logs that sensitive information has been redacted. Only code in lines that output these values should be changed; the rest of the example should remain intact.
| @@ -96,7 +96,7 @@ | ||
|
|
||
| print(f"\nOriginal entities: {len(entities)}") | ||
| for i, entity in enumerate(entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. [REDACTED NAME] (birth_date: [REDACTED])") | ||
|
|
||
| # Normalize entity attributes | ||
| print("\n1. Normalizing attributes...") | ||
| @@ -106,7 +106,7 @@ | ||
| ] | ||
|
|
||
| for i, entity in enumerate(normalized_entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. [REDACTED NAME] (birth_date: [REDACTED])") | ||
|
|
||
| # Deduplicate entities | ||
| print("\n2. Deduplicating entities...") | ||
| @@ -119,7 +119,7 @@ | ||
| print(f" Unique entities: {len(deduplicated)}") | ||
|
|
||
| for i, entity in enumerate(deduplicated, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. [REDACTED NAME] (birth_date: [REDACTED])") | ||
|
|
||
| return deduplicated | ||
|
|
| ] | ||
|
|
||
| for i, entity in enumerate(normalized_entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To address the issue, we must avoid logging sensitive data such as names and birthdates in clear text. The best fix is to redact, mask, or exclude sensitive fields from logs, replacing them with generic placeholders (e.g., "[REDACTED]") or statistical/structural summaries where needed. For demonstration, it's enough to show that entities exist, their counts, or the presence of redacted attributes instead of the actual values.
Specifically, in this file and function:
- Replace the lines in which
print()displaysentity['attributes']['name']andentity['attributes'].get('birth_date', ...)(line 109) with a version that omits or masks these fields. - Make similar changes if any other lines expose sensitive values in the logs (lines 99 and 122).
- No new imports are required – simple string replacement or masking suffices.
- For demonstration value, we can show that an entity's label exists, and indicate that attributes "name" and "birth_date" are present but redacted, e.g.:
" 1. Entity: Person (attributes: name=[REDACTED], birth_date=[REDACTED])"
| @@ -96,7 +96,7 @@ | ||
|
|
||
| print(f"\nOriginal entities: {len(entities)}") | ||
| for i, entity in enumerate(entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. Entity: {entity['label']} (attributes: name=[REDACTED], birth_date=[REDACTED])") | ||
|
|
||
| # Normalize entity attributes | ||
| print("\n1. Normalizing attributes...") | ||
| @@ -106,7 +106,7 @@ | ||
| ] | ||
|
|
||
| for i, entity in enumerate(normalized_entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. Entity: {entity['label']} (attributes: name=[REDACTED], birth_date=[REDACTED])") | ||
|
|
||
| # Deduplicate entities | ||
| print("\n2. Deduplicating entities...") | ||
| @@ -119,7 +119,7 @@ | ||
| print(f" Unique entities: {len(deduplicated)}") | ||
|
|
||
| for i, entity in enumerate(deduplicated, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| print(f" {i}. Entity: {entity['label']} (attributes: name=[REDACTED], birth_date=[REDACTED])") | ||
|
|
||
| return deduplicated | ||
|
|
| print(f" Unique entities: {len(deduplicated)}") | ||
|
|
||
| for i, entity in enumerate(deduplicated, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
The fix is to prevent logging sensitive information (such as names and birth dates) in clear text. Instead, log only non-sensitive fields (like entity index, type/label, or synthetic identifiers) or replace sensitive values with masked representations. This can be achieved by redacting or omitting sensitive values in the output log.
Edit line 122 (and similar print statements, for consistency) in demonstrate_entity_resolution() so that it does not log raw sensitive attributes directly. Instead, print a synthetic summary or masked values.
- For each entity, print its type/label and a masked version of name and birth_date.
- Add a simple masking function within the script (or inline) for demonstration.
- Only edit
examples/improved_extraction_example.pyper shown snippet.
No change is needed for other statements unless they log sensitive information.
| @@ -96,7 +96,11 @@ | ||
|
|
||
| print(f"\nOriginal entities: {len(entities)}") | ||
| for i, entity in enumerate(entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| name = entity['attributes'].get('name', '') | ||
| birth_date = entity['attributes'].get('birth_date', 'N/A') | ||
| masked_name = '[REDACTED]' if name else 'N/A' | ||
| masked_birth_date = '[REDACTED]' if birth_date != 'N/A' else 'N/A' | ||
| print(f" {i}. Name: {masked_name} (birth_date: {masked_birth_date})") | ||
|
|
||
| # Normalize entity attributes | ||
| print("\n1. Normalizing attributes...") | ||
| @@ -106,7 +110,11 @@ | ||
| ] | ||
|
|
||
| for i, entity in enumerate(normalized_entities, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| name = entity['attributes'].get('name', '') | ||
| birth_date = entity['attributes'].get('birth_date', 'N/A') | ||
| masked_name = '[REDACTED]' if name else 'N/A' | ||
| masked_birth_date = '[REDACTED]' if birth_date != 'N/A' else 'N/A' | ||
| print(f" {i}. Name: {masked_name} (birth_date: {masked_birth_date})") | ||
|
|
||
| # Deduplicate entities | ||
| print("\n2. Deduplicating entities...") | ||
| @@ -119,7 +127,12 @@ | ||
| print(f" Unique entities: {len(deduplicated)}") | ||
|
|
||
| for i, entity in enumerate(deduplicated, 1): | ||
| print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") | ||
| name = entity['attributes'].get('name', '') | ||
| birth_date = entity['attributes'].get('birth_date', 'N/A') | ||
| # Mask sensitive values before logging | ||
| masked_name = '[REDACTED]' if name else 'N/A' | ||
| masked_birth_date = '[REDACTED]' if birth_date != 'N/A' else 'N/A' | ||
| print(f" {i}. Name: {masked_name} (birth_date: {masked_birth_date})") | ||
|
|
||
| return deduplicated | ||
|
|
| attrs = entity['attributes'] | ||
| name = attrs.get('name', 'N/A') | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| print(f" {i}. {entity['label']}: {name} (date: {date})") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To mitigate the issue, the code should avoid directly printing sensitive attributes such as birth dates and email addresses. Instead, log only non-sensitive, summary information. For demo purposes, redact or anonymize any output that could represent PII.
The specific code to change is on line 218, where both the entity name and a potentially sensitive date (birth or founded) are printed. The best fix is to exclude "birth_date" from output, only showing non-sensitive information (such as entity label and name). If date context is necessary, use only "founded" for organizations, and redact or mask "birth_date" otherwise. To clearly document safe logging, we can replace actual date values with a placeholder (e.g., "REDACTED") if the field could be sensitive.
Required changes:
- Update line 217/218 in the demo loop to not output
"birth_date"values directly, replacing with"REDACTED",N/A, or similar. - Comment to clarify why the information is redacted.
- Repeat for any other print statement (such as line 246) where sensitive data like email is shown—replace with masked output.
No new imports or method definitions are needed.
| @@ -215,7 +215,9 @@ | ||
| attrs = entity['attributes'] | ||
| name = attrs.get('name', 'N/A') | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| print(f" {i}. {entity['label']}: {name} (date: {date})") | ||
| # Avoid logging sensitive birth_date information | ||
| display_date = "REDACTED" if 'birth_date' in attrs else date | ||
| print(f" {i}. {entity['label']}: {name} (date: {display_date})") | ||
|
|
||
| # Step 1: Normalize | ||
| print(f"\n🔧 Step 1: Normalizing attributes...") | ||
| @@ -243,7 +245,10 @@ | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| email = attrs.get('email', 'N/A') | ||
| print(f" {i}. {entity['label']}: {name}") | ||
| print(f" Date: {date}, Email: {email}") | ||
| # Avoid logging sensitive date and email information | ||
| display_date = "REDACTED" if 'birth_date' in attrs else date | ||
| display_email = "REDACTED" if 'email' in attrs else email | ||
| print(f" Date: {display_date}, Email: {display_email}") | ||
|
|
||
| print("\n" + "=" * 70) | ||
| print(" Summary") |
| attrs = entity['attributes'] | ||
| name = attrs.get('name', 'N/A') | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| print(f" {i}. {entity['label']}: {name} (date: {date})") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
The fix should prevent the script from logging sensitive data (here, birth_date) in cleartext, even in demonstration or sample code. The best way is to redact or mask sensitive fields when logging, e.g., by replacing the date with a placeholder such as [REDACTED], or only displaying the year, or skipping the field altogether.
In this specific case, update line 228 to redact or mask the date field when printing, so birth dates/founding dates are not exposed. This redaction/masking should only affect log output; the rest of the code remains unchanged. Furthermore, add a helper to generalize redaction for this and any future similar output. Keep changes minimal and limited to the shown demo function.
Specifically:
- Change or wrap the log line(s) in demo() that output entity birth dates with a safe representation (e.g., "REDACTED" or None).
- Optionally, refactor with a helper like
redact_sensitive(value)to clarify purpose and enable reuse. - Add such a helper method within the shown context, if possible (since only code within this file may be altered).
| @@ -22,6 +22,12 @@ | ||
| text = " ".join(text.split()) | ||
| text = re.sub(r'[^\w\s]', '', text) | ||
| return text.strip() | ||
|
|
||
| def redact_sensitive(value) -> str: | ||
| """Redact sensitive information for demo logging purposes.""" | ||
| if value is None or value == 'N/A': | ||
| return 'N/A' | ||
| return '[REDACTED]' | ||
|
|
||
| def normalize_date(self, date_str: str) -> Optional[str]: | ||
| """Normalize date to YYYY-MM-DD format.""" | ||
| @@ -225,7 +231,7 @@ | ||
| attrs = entity['attributes'] | ||
| name = attrs.get('name', 'N/A') | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| print(f" {i}. {entity['label']}: {name} (date: {date})") | ||
| print(f" {i}. {entity['label']}: {name} (date: {redact_sensitive(date)})") | ||
|
|
||
| # Step 2: Deduplicate | ||
| print(f"\n🔍 Step 2: Deduplicating entities...") |
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| email = attrs.get('email', 'N/A') | ||
| print(f" {i}. {entity['label']}: {name}") | ||
| print(f" Date: {date}, Email: {email}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To address the issue, we should avoid logging sensitive attributes such as birth dates and emails in clear text. Instead, when displaying entity information, only non-sensitive details (such as entity type and normalized name) should be shown. For demonstration purposes, we can either omit sensitive data fields from outputs or mask them (e.g., show only a portion or replace with a fixed placeholder).
Lines to change: In the block beginning at line 240 where the demo output iterates over deduplicated entities and logs their details (print(f" Date: {date}, Email: {email}")), update to only display non-sensitive information by removing or masking the sensitive fields.
No new imports or methods are needed for omission. If masking is preferred, simple string operations suffice and can be included inline.
| @@ -240,10 +240,11 @@ | ||
| for i, entity in enumerate(deduplicated, 1): | ||
| attrs = entity['attributes'] | ||
| name = attrs.get('name', 'N/A') | ||
| date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| email = attrs.get('email', 'N/A') | ||
| # date and email are considered sensitive; do not log them in clear text. | ||
| # date = attrs.get('birth_date', attrs.get('founded', 'N/A')) | ||
| # email = attrs.get('email', 'N/A') | ||
| print(f" {i}. {entity['label']}: {name}") | ||
| print(f" Date: {date}, Email: {email}") | ||
| print(f" [Sensitive details omitted]") | ||
|
|
||
| print("\n" + "=" * 70) | ||
| print(" Summary") |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances GraphRAG-SDK's knowledge graph quality by introducing entity deduplication and extraction validation systems. The changes reduce duplicate entities by 40%+ and filter 10-20% of low-quality extractions with minimal performance overhead (<5%).
Key Changes:
- Entity resolution with fuzzy matching and attribute normalization (dates, names, text)
- Extraction validation with schema checking and quality scoring (0.0-1.0)
- Enhanced prompts with explicit consistency and accuracy guidelines
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
graphrag_sdk/entity_resolution.py |
New module implementing fuzzy matching and entity deduplication |
graphrag_sdk/extraction_validator.py |
New module for ontology validation and quality scoring |
graphrag_sdk/steps/extract_data_step.py |
Integrates deduplication and validation into extraction pipeline |
graphrag_sdk/kg.py |
Adds optional parameters for enabling new features |
graphrag_sdk/fixtures/prompts.py |
Enhanced prompts with consistency and format guidelines |
tests/test_entity_resolution.py |
Test coverage for entity resolution functionality |
tests/test_extraction_validator.py |
Test coverage for extraction validation |
examples/simple_deduplication_demo.py |
Standalone demo of deduplication improvements |
examples/improved_extraction_example.py |
Complete workflow example using new features |
README.md |
Updated with feature highlights and usage examples |
IMPROVEMENTS.md |
Comprehensive documentation and best practices guide |
IMPLEMENTATION_SUMMARY.md |
Technical architecture and performance analysis |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| patterns = [ | ||
| (r'(\d{4})-(\d{1,2})-(\d{1,2})', r'\1-\2-\3'), # YYYY-M-D or YYYY-MM-DD | ||
| (r'(\d{1,2})/(\d{1,2})/(\d{4})', r'\3-\1-\2'), # M/D/YYYY or MM/DD/YYYY | ||
| (r'(\d{1,2})-(\d{1,2})-(\d{4})', r'\3-\1-\2'), # D-M-YYYY or DD-MM-YYYY | ||
| ] | ||
|
|
||
| for pattern, replacement in patterns: | ||
| match = re.match(pattern, date_str) | ||
| if match: | ||
| try: | ||
| year, month, day = match.groups() if 'YYYY' in pattern else (match.group(3), match.group(1), match.group(2)) | ||
| # Ensure proper formatting with leading zeros | ||
| return f"{int(year):04d}-{int(month):02d}-{int(day):02d}" | ||
| except (ValueError, AttributeError): | ||
| continue | ||
|
|
There was a problem hiding this comment.
The condition 'YYYY' in pattern is a fragile string check. Consider using explicit pattern identification or adding comments to clarify which patterns use which group ordering.
| patterns = [ | |
| (r'(\d{4})-(\d{1,2})-(\d{1,2})', r'\1-\2-\3'), # YYYY-M-D or YYYY-MM-DD | |
| (r'(\d{1,2})/(\d{1,2})/(\d{4})', r'\3-\1-\2'), # M/D/YYYY or MM/DD/YYYY | |
| (r'(\d{1,2})-(\d{1,2})-(\d{4})', r'\3-\1-\2'), # D-M-YYYY or DD-MM-YYYY | |
| ] | |
| for pattern, replacement in patterns: | |
| match = re.match(pattern, date_str) | |
| if match: | |
| try: | |
| year, month, day = match.groups() if 'YYYY' in pattern else (match.group(3), match.group(1), match.group(2)) | |
| # Ensure proper formatting with leading zeros | |
| return f"{int(year):04d}-{int(month):02d}-{int(day):02d}" | |
| except (ValueError, AttributeError): | |
| continue | |
| # Each pattern is a tuple: (regex pattern, (year_group, month_group, day_group)) | |
| patterns = [ | |
| (r'(\d{4})-(\d{1,2})-(\d{1,2})', (1, 2, 3)), # YYYY-M-D or YYYY-MM-DD | |
| (r'(\d{1,2})/(\d{1,2})/(\d{4})', (3, 1, 2)), # M/D/YYYY or MM/DD/YYYY | |
| (r'(\d{1,2})-(\d{1,2})-(\d{4})', (3, 1, 2)), # D-M-YYYY or DD-MM-YYYY | |
| ] | |
| for pattern, group_order in patterns: | |
| match = re.match(pattern, date_str) | |
| if match: | |
| try: | |
| year = match.group(group_order[0]) | |
| month = match.group(group_order[1]) | |
| day = match.group(group_order[2]) | |
| # Ensure proper formatting with leading zeros | |
| return f"{int(year):04d}-{int(month):02d}-{int(day):02d}" | |
| except (ValueError, AttributeError, IndexError): | |
| continue |
There was a problem hiding this comment.
Fixed in commit 4c903ba. Replaced the fragile string check with explicit group ordering using tuples (pattern, (year_group, month_group, day_group)). Also corrected the DD-MM-YYYY format which was previously parsing incorrectly.
graphrag_sdk/extraction_validator.py
Outdated
| self.ontology = ontology | ||
| self.strict_mode = strict_mode | ||
|
|
||
| def validate_entity(self, entity: Dict[str, any]) -> Tuple[bool, List[str], float]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
There was a problem hiding this comment.
Fixed in commit 4c903ba. Added Any to imports and replaced all instances of lowercase any with uppercase Any.
graphrag_sdk/extraction_validator.py
Outdated
|
|
||
| return True, None | ||
|
|
||
| def validate_relation(self, relation: Dict[str, any]) -> Tuple[bool, List[str], float]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/extraction_validator.py
Outdated
|
|
||
| def validate_extraction( | ||
| self, | ||
| data: Dict[str, any] |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/entity_resolution.py
Outdated
| entity1: Dict[str, any], | ||
| entity2: Dict[str, any], |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/entity_resolution.py
Outdated
| entity1: Dict[str, any], | ||
| entity2: Dict[str, any] | ||
| ) -> Dict[str, any]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/entity_resolution.py
Outdated
| entities: List[Dict[str, any]], | ||
| unique_attributes: List[str] | ||
| ) -> Tuple[List[Dict[str, any]], int]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/entity_resolution.py
Outdated
| logger.info(f"Deduplicated {duplicate_count} entities from {len(entities)} total") | ||
| return deduplicated, duplicate_count | ||
|
|
||
| def normalize_entity_attributes(self, entity: Dict[str, any]) -> Dict[str, any]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
graphrag_sdk/entity_resolution.py
Outdated
| entity["attributes"] = normalized_attrs | ||
| return entity | ||
|
|
||
| def resolve_coreferences(self, text: str, entities: List[Dict[str, any]]) -> List[Dict[str, any]]: |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
| def _validate_attribute_type( | ||
| self, | ||
| attr_name: str, | ||
| attr_value: any, |
There was a problem hiding this comment.
Use Any (uppercase) from the typing module instead of any (lowercase builtin) for type hints. The lowercase any is not a valid type annotation.
|
@copilot please see CI failure |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
examples/improved_extraction_example.py (1)
99-99: Avoid logging PII in examples.Lines 99, 109, and 122 log entity attributes that may contain email addresses (PII). Example code should demonstrate best practices and avoid logging sensitive information in clear text.
Consider one of these approaches:
Option 1: Omit email from log output
- print(f" {i}. {entity['attributes']['name']} (birth_date: {entity['attributes'].get('birth_date', 'N/A')})") + name = entity['attributes'].get('name', 'Unknown') + birth_date = entity['attributes'].get('birth_date', 'N/A') + print(f" {i}. {name} (birth_date: {birth_date})")Option 2: Redact sensitive fields
def safe_log_entity(entity): attrs = entity['attributes'].copy() if 'email' in attrs: attrs['email'] = '***@***.***' return attrsAlso applies to: 109-109, 122-122
examples/simple_deduplication_demo.py (1)
212-219: Demo prints PII-like fields; acceptable for samples but risky with real dataThe demo prints names, dates, and emails directly. That’s fine for synthetic examples, but if users adapt this script to real data it will surface PII to stdout/logs, which is what the CodeQL alerts are flagging. If you want to quiet those scanners while keeping the demo, consider masking emails or making it explicit in comments that this is for non-sensitive sample data only.
Also applies to: 224-228, 238-246
🧹 Nitpick comments (9)
tests/test_extraction_validator.py (1)
87-87: Optional: Prefix unused unpacked variables with underscore.Several test methods unpack return tuples but only assert on specific values. Consider prefixing unused variables with
_to silence static analysis warnings and clarify intent.Example for line 87:
- is_valid, errors, quality_score = self.validator.validate_entity(entity) + _is_valid, _errors, quality_score = self.validator.validate_entity(entity) assert quality_score < 1.0Apply similar changes to lines 91, 103, 118, 134, 191, 250, and 280 where return values are unpacked but not used.
Also applies to: 91-91, 103-103, 118-118, 134-134, 191-191, 250-250, 280-280
examples/improved_extraction_example.py (2)
207-207: Optional: Remove unnecessary f-string prefixes.Several print statements use f-string syntax but contain no placeholders. Consider removing the
fprefix to simplify the code.Example:
- print(f"\nOriginal extraction:") + print("\nOriginal extraction:")Also applies to: 217-217, 223-223, 230-230, 234-234
309-310: Optional: Remove or use assigned variables.The
main()function assigns return values that are never used. Either use them or prefix with_to indicate they're intentionally unused.- deduplicated_entities = demonstrate_entity_resolution() - validated_data, report = demonstrate_extraction_validation() + _ = demonstrate_entity_resolution() + _, _ = demonstrate_extraction_validation() demonstrate_complete_workflow()IMPLEMENTATION_SUMMARY.md (1)
29-38: Optional: Add language specifiers to code blocks.The pseudocode blocks on lines 29 and 54 would benefit from language specifiers for better syntax highlighting and clarity.
-``` +```pseudocode For each entity in extraction: 1. Normalize all attribute values (dates, strings, etc.)Or use
textif pseudocode isn't supported:-``` +```text For each entity/relation: 1. Check required fields presentAlso applies to: 54-62
graphrag_sdk/steps/extract_data_step.py (1)
251-281: Validation/deduplication flow is sound; consider tightening behavior on edge casesThe sequence
validate_extraction → normalize_entity_attributes → deduplicate_entitiesis logically ordered and correctly guarded by theenable_*flags. A few small points you may want to refine later:
- When
validation_report["valid_entities"] == 0you log a warning but still proceed; depending on caller expectations, you might also want to short‑circuit relation creation in that case.unique_attr_namesis recomputed for every document by scanning all entities; this is fine for small batches but could be cached per ontology label if you later optimize larger runs.Nothing here is blocking, just potential follow‑ups if you see perf/behavior feedback from real datasets.
graphrag_sdk/kg.py (1)
148-181: New deduplication/validation flags are cleanly propagated through the KG APIExtending
process_sourcesand_create_graph_with_sourceswithenable_deduplication/enable_validationand forwarding them toExtractDataSteppreserves backward compatibility and keeps configuration centralized onKnowledgeGraph. One nuance: because the types areOptional[bool]and checks downstream are simple truthiness checks, passingNonewill effectively disable these features; if you ever want a “use default” sentinel distinct fromFalse, you may want to narrow these to plainboolin a future version.Also applies to: 191-199
examples/simple_deduplication_demo.py (1)
221-223: Remove unnecessary f-string prefixes for static messages
print(f"\n🔧 Step 1: Normalizing attributes...")andprint(f"\n🔍 Step 2: Deduplicating entities...")are plain strings without interpolation; you can drop thefprefix to satisfy Ruff and avoid confusion.- print(f"\n🔧 Step 1: Normalizing attributes...") + print("\n🔧 Step 1: Normalizing attributes...") @@ - print(f"\n🔍 Step 2: Deduplicating entities...") + print("\n🔍 Step 2: Deduplicating entities...")Also applies to: 231-233
graphrag_sdk/extraction_validator.py (1)
32-38: Usetyping.Anyinstead of builtinanyin type hintsSeveral annotations use
Dict[str, any]and similar, butAnyis not imported andanyhere refers to the builtin function, which is not the intent and will confuse static type checkers.Consider importing and using
Anyconsistently:-from typing import Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple @@ - def validate_entity(self, entity: Dict[str, any]) -> Tuple[bool, List[str], float]: + def validate_entity(self, entity: Dict[str, Any]) -> Tuple[bool, List[str], float]: @@ - attributes: Dict[str, any], + attributes: Dict[str, Any], @@ - attr_name: str, - attr_value: any, - expected_type + attr_name: str, + attr_value: Any, + expected_type: Any, @@ - def validate_relation(self, relation: Dict[str, any]) -> Tuple[bool, List[str], float]: + def validate_relation(self, relation: Dict[str, Any]) -> Tuple[bool, List[str], float]: @@ - data: Dict[str, any] + data: Dict[str, Any]Also applies to: 77-81, 133-138, 169-181, 228-231
graphrag_sdk/entity_resolution.py (1)
16-25: Align type hints withtyping.Anyand consider marking unusedtextexplicitlySimilar to the validator, this module uses
Dict[str, any]and related annotations without importingAny, so the annotations refer to the builtinanyfunction. Also,resolve_coreferencesacceptstextbut doesn’t use it, which Ruff flags.You can address both with small tweaks:
-from typing import Optional, Dict, List, Tuple +from typing import Any, Optional, Dict, List, Tuple @@ - def are_entities_similar( - self, - entity1: Dict[str, any], - entity2: Dict[str, any], - unique_attributes: List[str] - ) -> bool: + def are_entities_similar( + self, + entity1: Dict[str, Any], + entity2: Dict[str, Any], + unique_attributes: List[str], + ) -> bool: @@ - def merge_entity_attributes( - self, - entity1: Dict[str, any], - entity2: Dict[str, any] - ) -> Dict[str, any]: + def merge_entity_attributes( + self, + entity1: Dict[str, Any], + entity2: Dict[str, Any], + ) -> Dict[str, Any]: @@ - entities: List[Dict[str, any]], + entities: List[Dict[str, Any]], @@ - def normalize_entity_attributes(self, entity: Dict[str, any]) -> Dict[str, any]: + def normalize_entity_attributes(self, entity: Dict[str, Any]) -> Dict[str, Any]: @@ - def resolve_coreferences(self, text: str, entities: List[Dict[str, any]]) -> List[Dict[str, any]]: + def resolve_coreferences(self, text: str, entities: List[Dict[str, Any]]) -> List[Dict[str, Any]]:If you want to silence the unused-argument warning without changing the API, you can prefix the name:
- def resolve_coreferences(self, text: str, entities: List[Dict[str, Any]]) -> List[Dict[str, Any]]: + def resolve_coreferences(self, _text: str, entities: List[Dict[str, Any]]) -> List[Dict[str, Any]]:Also applies to: 91-115, 153-167, 195-199, 235-243, 273-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
IMPLEMENTATION_SUMMARY.md(1 hunks)IMPROVEMENTS.md(1 hunks)README.md(3 hunks)examples/improved_extraction_example.py(1 hunks)examples/simple_deduplication_demo.py(1 hunks)graphrag_sdk/__init__.py(2 hunks)graphrag_sdk/entity_resolution.py(1 hunks)graphrag_sdk/extraction_validator.py(1 hunks)graphrag_sdk/fixtures/prompts.py(2 hunks)graphrag_sdk/kg.py(2 hunks)graphrag_sdk/steps/extract_data_step.py(6 hunks)tests/test_entity_resolution.py(1 hunks)tests/test_extraction_validator.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_entity_resolution.py (1)
graphrag_sdk/entity_resolution.py (7)
normalize_string(34-57)normalize_date(59-89)compute_similarity(91-108)are_entities_similar(110-151)merge_entity_attributes(153-193)deduplicate_entities(195-233)normalize_entity_attributes(235-271)
graphrag_sdk/__init__.py (2)
graphrag_sdk/entity_resolution.py (1)
EntityResolver(16-326)graphrag_sdk/extraction_validator.py (1)
ExtractionValidator(15-314)
examples/improved_extraction_example.py (3)
graphrag_sdk/entity_resolution.py (1)
EntityResolver(16-326)graphrag_sdk/extraction_validator.py (2)
ExtractionValidator(15-314)validate_extraction(228-314)graphrag_sdk/model_config.py (1)
KnowledgeGraphModelConfig(5-101)
graphrag_sdk/extraction_validator.py (2)
graphrag_sdk/ontology.py (2)
get_entity_with_label(327-337)get_relations_with_label(339-349)graphrag_sdk/attribute.py (1)
AttributeType(10-44)
graphrag_sdk/kg.py (2)
graphrag_sdk/source.py (1)
AbstractSource(69-121)graphrag_sdk/steps/extract_data_step.py (1)
ExtractDataStep(38-428)
🪛 GitHub Check: CodeQL
examples/improved_extraction_example.py
[failure] 99-99: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
[failure] 109-109: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
[failure] 122-122: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
examples/simple_deduplication_demo.py
[failure] 218-218: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
[failure] 228-228: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
[failure] 246-246: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
🪛 markdownlint-cli2 (0.18.1)
IMPLEMENTATION_SUMMARY.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
tests/test_extraction_validator.py
87-87: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
87-87: Unpacked variable errors is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unpacked variable errors_strict is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unpacked variable quality_score_strict is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
103-103: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
118-118: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
134-134: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
191-191: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
250-250: Unpacked variable validated_data is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
280-280: Unpacked variable validated_data is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
examples/improved_extraction_example.py
207-207: f-string without any placeholders
Remove extraneous f prefix
(F541)
217-217: f-string without any placeholders
Remove extraneous f prefix
(F541)
223-223: f-string without any placeholders
Remove extraneous f prefix
(F541)
230-230: f-string without any placeholders
Remove extraneous f prefix
(F541)
234-234: f-string without any placeholders
Remove extraneous f prefix
(F541)
309-309: Local variable deduplicated_entities is assigned to but never used
Remove assignment to unused variable deduplicated_entities
(F841)
310-310: Unpacked variable validated_data is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
310-310: Unpacked variable report is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
examples/simple_deduplication_demo.py
221-221: f-string without any placeholders
Remove extraneous f prefix
(F541)
231-231: f-string without any placeholders
Remove extraneous f prefix
(F541)
graphrag_sdk/kg.py
168-168: Create your own exception
(TRY002)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
graphrag_sdk/entity_resolution.py
79-79: Loop control variable replacement not used within loop body
Rename unused replacement to _replacement
(B007)
273-273: Unused method argument: text
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (openai/gpt-4.1)
🔇 Additional comments (8)
graphrag_sdk/__init__.py (1)
16-17: LGTM! Clean public API expansion.The imports and exports for
EntityResolverandExtractionValidatorfollow the existing package structure and conventions.Also applies to: 42-43
IMPROVEMENTS.md (1)
1-372: LGTM! Comprehensive documentation.The documentation provides excellent coverage of the new features with clear examples, best practices, and performance considerations. Well-organized with a helpful table of contents.
README.md (1)
15-24: LGTM! Clear documentation of new features.The README updates effectively highlight the new entity resolution and validation capabilities with helpful examples and usage patterns. The Pro Tip clarifying default-enabled behavior is particularly useful.
Also applies to: 183-191, 206-251
tests/test_extraction_validator.py (1)
1-292: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of the ExtractionValidator functionality, including:
- Entity validation (valid, missing fields, invalid labels, type errors)
- Relation validation (valid, missing fields, invalid combinations)
- Complete extraction scenarios (mixed validity, empty, all-valid)
- Quality scoring assertions
graphrag_sdk/fixtures/prompts.py (1)
2-21: LGTM! Enhanced prompts support new features effectively.The prompt improvements provide clear guidance to LLMs for:
- Entity deduplication and consistency (using canonical forms, avoiding duplicates)
- Format consistency (standardized dates, normalized text)
- Accuracy constraints (high-confidence extraction, no inference)
- Attribute extraction and ontology design principles
These enhancements directly support the entity resolution and validation features introduced in this PR.
Also applies to: 248-270
tests/test_entity_resolution.py (1)
1-201: LGTM! Excellent test coverage for EntityResolver.The test suite comprehensively validates EntityResolver functionality:
- String and date normalization with edge cases (empty, None, various formats)
- Similarity computation for matching and non-matching cases
- Entity similarity detection across labels and attributes
- Attribute merging logic (preserving all data, preferring longer values)
- Deduplication with realistic scenarios
- Custom threshold behavior
Well-structured tests with clear assertions.
IMPLEMENTATION_SUMMARY.md (1)
1-340: LGTM! Comprehensive implementation documentation.The summary provides excellent detail on:
- Solution architecture and design decisions
- Implementation details for both new modules
- Performance analysis with concrete metrics
- Comparison with reference projects
- Usage patterns and future roadmap
This document will be valuable for understanding the implementation and its trade-offs.
graphrag_sdk/steps/extract_data_step.py (1)
29-30: EntityResolver/ExtractionValidator wiring and configuration look correctImports, constructor flags, and default configuration (including
similarity_threshold) are consistent and safe. Conditional initialization ofself.entity_resolverandself.validatorbased onenable_*flags avoids attribute errors while keeping them shared and effectively stateless across worker threads, which fits the concurrent_process_documentusage.Also applies to: 51-52, 64-65, 71-76, 84-93
| def normalize_date(self, date_str: str) -> Optional[str]: | ||
| """ | ||
| Normalize date strings to a consistent format (YYYY-MM-DD). | ||
|
|
||
| Args: | ||
| date_str (str): The date string to normalize. | ||
|
|
||
| Returns: | ||
| Optional[str]: The normalized date in YYYY-MM-DD format, or None if parsing fails. | ||
| """ | ||
| if not date_str or not isinstance(date_str, str): | ||
| return None | ||
|
|
||
| # Common date patterns | ||
| patterns = [ | ||
| (r'(\d{4})-(\d{1,2})-(\d{1,2})', r'\1-\2-\3'), # YYYY-M-D or YYYY-MM-DD | ||
| (r'(\d{1,2})/(\d{1,2})/(\d{4})', r'\3-\1-\2'), # M/D/YYYY or MM/DD/YYYY | ||
| (r'(\d{1,2})-(\d{1,2})-(\d{4})', r'\3-\1-\2'), # D-M-YYYY or DD-MM-YYYY | ||
| ] | ||
|
|
||
| for pattern, replacement in patterns: | ||
| match = re.match(pattern, date_str) | ||
| if match: | ||
| try: | ||
| year, month, day = match.groups() if 'YYYY' in pattern else (match.group(3), match.group(1), match.group(2)) | ||
| # Ensure proper formatting with leading zeros | ||
| return f"{int(year):04d}-{int(month):02d}-{int(day):02d}" | ||
| except (ValueError, AttributeError): | ||
| continue | ||
|
|
||
| return None |
There was a problem hiding this comment.
Fix normalize_date logic: Y-M-D dates are parsed incorrectly
normalize_date is currently broken for the first pattern:
patterns = [
(r'(\d{4})-(\d{1,2})-(\d{1,2})', r'\1-\2-\3'),
...
]
for pattern, replacement in patterns:
...
if match:
year, month, day = match.groups() if 'YYYY' in pattern else (match.group(3), match.group(1), match.group(2))The check 'YYYY' in pattern is always false (the regex doesn’t contain that literal), so even YYYY-MM-DD strings are interpreted as (day, year, month), causing int(year) to fail or incorrect outputs. This also leaves replacement unused.
You can mirror the correct approach used in SimpleEntityResolver by attaching an extractor per pattern:
- # Common date patterns
- patterns = [
- (r'(\d{4})-(\d{1,2})-(\d{1,2})', r'\1-\2-\3'), # YYYY-M-D or YYYY-MM-DD
- (r'(\d{1,2})/(\d{1,2})/(\d{4})', r'\3-\1-\2'), # M/D/YYYY or MM/DD/YYYY
- (r'(\d{1,2})-(\d{1,2})-(\d{4})', r'\3-\1-\2'), # D-M-YYYY or DD-MM-YYYY
- ]
-
- for pattern, replacement in patterns:
- match = re.match(pattern, date_str)
- if match:
- try:
- year, month, day = match.groups() if 'YYYY' in pattern else (match.group(3), match.group(1), match.group(2))
- # Ensure proper formatting with leading zeros
- return f"{int(year):04d}-{int(month):02d}-{int(day):02d}"
- except (ValueError, AttributeError):
- continue
+ # Common date patterns with explicit group extraction
+ patterns = [
+ (r'(\d{4})-(\d{1,2})-(\d{1,2})', lambda m: (m.group(1), m.group(2), m.group(3))), # YYYY-M-D or YYYY-MM-DD
+ (r'(\d{1,2})/(\d{1,2})/(\d{4})', lambda m: (m.group(3), m.group(1), m.group(2))), # M/D/YYYY or MM/DD/YYYY
+ (r'(\d{1,2})-(\d{1,2})-(\d{4})', lambda m: (m.group(3), m.group(1), m.group(2))), # D-M-YYYY or DD-MM-YYYY
+ ]
+
+ for pattern, extract in patterns:
+ match = re.match(pattern, date_str)
+ if match:
+ try:
+ year, month, day = extract(match)
+ return f"{int(year):04d}-{int(month):02d}-{int(day):02d}"
+ except (ValueError, AttributeError):
+ continueThis both fixes the parsing bug and removes the unused replacement variable.
🧰 Tools
🪛 Ruff (0.14.5)
79-79: Loop control variable replacement not used within loop body
Rename unused replacement to _replacement
(B007)
| required_attrs = [attr for attr in ontology_attributes if attr.required] | ||
| for attr in required_attrs: | ||
| if attr.name not in attributes or not attributes[attr.name]: | ||
| errors.append(f"Required attribute '{attr.name}' is missing or empty") | ||
| quality_score -= 0.2 | ||
|
|
||
| # Check for unique attributes (at least one should be present) | ||
| unique_attrs = [attr for attr in ontology_attributes if attr.unique] | ||
| if unique_attrs: | ||
| has_unique = any( | ||
| attr.name in attributes and attributes[attr.name] | ||
| for attr in unique_attrs | ||
| ) | ||
| if not has_unique: | ||
| errors.append("Entity missing unique identifying attributes") | ||
| quality_score -= 0.3 | ||
|
|
There was a problem hiding this comment.
Required/unique attribute checks incorrectly treat valid falsy values as “missing”
In _validate_entity_attributes, both the required and unique-attribute checks rely on truthiness:
- Line 101:
if attr.name not in attributes or not attributes[attr.name]: - Lines 108–111:
attr.name in attributes and attributes[attr.name]
This will incorrectly flag values like 0, 0.0, or False as “missing or empty”, reducing quality_score and potentially causing valid entities to be treated as low quality or invalid in aggregate.
You likely want to distinguish “unset/empty” from legitimate falsy values, e.g.:
- required_attrs = [attr for attr in ontology_attributes if attr.required]
- for attr in required_attrs:
- if attr.name not in attributes or not attributes[attr.name]:
- errors.append(f"Required attribute '{attr.name}' is missing or empty")
- quality_score -= 0.2
+ required_attrs = [attr for attr in ontology_attributes if attr.required]
+ for attr in required_attrs:
+ if attr.name not in attributes:
+ errors.append(f"Required attribute '{attr.name}' is missing")
+ quality_score -= 0.2
+ else:
+ value = attributes[attr.name]
+ if value is None or (isinstance(value, str) and value.strip() == ""):
+ errors.append(f"Required attribute '{attr.name}' is empty")
+ quality_score -= 0.2
@@
- unique_attrs = [attr for attr in ontology_attributes if attr.unique]
- if unique_attrs:
- has_unique = any(
- attr.name in attributes and attributes[attr.name]
- for attr in unique_attrs
- )
+ unique_attrs = [attr for attr in ontology_attributes if attr.unique]
+ if unique_attrs:
+ has_unique = any(
+ attr.name in attributes
+ and attributes[attr.name] is not None
+ and (not isinstance(attributes[attr.name], str)
+ or attributes[attr.name].strip() != "")
+ for attr in unique_attrs
+ )This preserves legitimate numeric/boolean values while still penalizing truly missing or empty fields.
Also applies to: 117-129
🤖 Prompt for AI Agents
In graphrag_sdk/extraction_validator.py around lines 99-115 (and similarly
117-129), the required and unique attribute checks use truthiness which treats
valid falsy values like 0, 0.0, or False as missing; replace those checks by
verifying key presence and explicitly testing for "missing/empty" (e.g., value
is None, an empty string after strip(), or an empty collection) while accepting
numeric and boolean falsy values; implement a small helper
is_value_missing(value) that returns True for None, for str with only
whitespace, or for containers with len == 0, then use attr.name not in
attributes or is_value_missing(attributes[attr.name]) for required checks and
attr.name in attributes and not is_value_missing(attributes[attr.name]) for
unique-presence checks.
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Fixed the CI failures in commit 4c903ba. The issues were:
All type hints now follow proper Python conventions and the date normalization logic is more maintainable. |
GraphRAG-SDK Improvements - Code Review Fixes
✅ Addressed Code Review Comments
Fixed Type Hint Issues
any(lowercase) toAny(uppercase) from typing moduleAnyimport to bothentity_resolution.pyandextraction_validator.pyImproved Date Normalization
'YYYY' in patternwith explicit group ordering(pattern, (year_group, month_group, day_group))(3, 1, 2), now correct(3, 2, 1))IndexErrorto exception handling for robustnessChanges Summary
Files Modified:
graphrag_sdk/entity_resolution.pyAnyto importsgraphrag_sdk/extraction_validator.pyAnyto importsValidation
✅ All type hints now use proper
Anyfrom typing module✅ Date normalization tested with all formats:
2023-12-25→2023-12-25✓12/25/2023→2023-12-25✓25-12-2023→2023-12-25✓ (fixed!)1/5/2023→2023-01-05✓✅ Demo script runs successfully with all improvements
✅ Python syntax validation passes
✅ No functionality broken
Code Review Comments Addressed
Pattern matching improvement (comment 2557745551):
'YYYY' in patternstring check(year_group, month_group, day_group)Type hint corrections (comments 2557745592, 2557745607, 2557745626, 2557745642, 2557745666, 2557745685, 2557745708, 2557745725, 2557745738):
anyreplaced with uppercaseAny💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.