-
Notifications
You must be signed in to change notification settings - Fork 17
doc processor: Markdown skip only header chunks #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
doc processor: Markdown skip only header chunks #48
Conversation
When processing documents using the markdown chunker we may end up with
chunks that don't really have any useful information, as in having only
headers.
For example, the following text:
# Configuration
## cinder.conf
Blah, blah
Will result in 2 chunks:
# Configuration
and
## cinder.conf
Blah, blah
With the first chunk being useless, moreover considering the second
chunk has this in its metadata (that is used for the embedding):
header_path: /Configuration
In this patch we change how we process markdown doc types and remove
chunks that have no real content because they only have headers or only
have space related characters.
WalkthroughThe PR refactors text validation in the document processor by introducing two new validation methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lightspeed_rag_content/document_processor.py (1)
102-111: LGTM - Validation correctly updated to use new logic.The conversion to an instance method and delegation to
_valid_text_nodeis correct. The generic log message is appropriate given that validation now includes multiple conditions (whitespace and header checks).Optional: Consider logging more details about why a node is invalid (e.g., "no whitespace", "only headers") to aid debugging, but this is a minor enhancement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lightspeed_rag_content/document_processor.py(1 hunks)tests/test_document_processor_llama_index.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_document_processor_llama_index.py (1)
src/lightspeed_rag_content/document_processor.py (3)
_valid_text_node(96-100)_got_non_headers(88-94)_filter_out_invalid_nodes(102-111)
⏰ 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). (3)
- GitHub Check: Pylinter
- GitHub Check: build-and-push-dev
- GitHub Check: Konflux kflux-prd-rh02 / rag-content-on-pull-request
🔇 Additional comments (7)
src/lightspeed_rag_content/document_processor.py (3)
87-94: LGTM - Clean header detection logic.The implementation correctly identifies non-header content in markdown by checking if any non-empty line doesn't start with "#". The early return optimization is good.
Note: Code blocks containing lines starting with "#" will be treated as headers, but this is acceptable given the goal of filtering header-only chunks.
96-100: LGTM - Correct validation logic for markdown chunks.The validation correctly enforces that markdown chunks must contain non-header content in addition to whitespace. The early return when markdown has only headers is efficient.
This implementation aligns with the PR objective to filter header-only and whitespace-only chunks specifically for markdown documents.
113-116: LGTM - Method correctly converted to instance method.The conversion from classmethod to instance method is necessary and correctly implemented. All call sites (lines 155 and 357) are properly updated to invoke this as an instance method.
tests/test_document_processor_llama_index.py (4)
22-22: LGTM - Import correctly updated for test requirements.The addition of
Nodeimport is necessary to properly spec the non-TextNode mock intest__filter_out_invalid_nodes, allowing the test to verify that only TextNode instances are validated.
75-120: LGTM - Comprehensive test coverage for validation logic.These tests thoroughly cover the
_valid_text_nodemethod behavior across different document types and content scenarios:
- Plain doc type correctly checks only whitespace
- Markdown doc type correctly checks both non-headers and whitespace
- Early return for header-only markdown is properly tested
The mock assertions ensure the correct methods are called in the right sequence.
122-152: LGTM - Excellent parametrized test coverage with edge cases.These parametrized tests provide comprehensive coverage of the
_got_non_headersmethod:
- Content detection tests verify various patterns of headers mixed with content
- Header-only tests cover important edge cases: leading/trailing whitespace, whitespace-only text, multiple headers, and headers without spaces
The edge case coverage (lines 143-147) is particularly thorough, including whitespace-only text and headers with various spacing patterns.
154-175: LGTM - Test correctly verifies updated filtering behavior.The test properly validates that:
- Only
TextNodeinstances are checked for validity via_valid_text_node- Non-TextNode instances are filtered out without validation
- The validation results correctly determine which nodes are kept
The use of
side_effectto return different validation results for different nodes is an effective testing approach.
Description
When processing documents using the markdown chunker we may end up with chunks that don't really have any useful information, as in having only headers.
For example, the following text:
Will result in 2 chunks:
and
With the first chunk being useless, moreover considering the second chunk has this in its metadata (that is used for the embedding):
In this patch we change how we process markdown doc types and remove chunks that have no real content because they only have headers or only have space related characters.
Type of change
Checklist before requesting a review
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.