Repository navigation
fix(knowledge): chunk CSV/JSON file text instead of the content dict repr - #7612
Lesereingrape wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughCSV and JSON knowledge sources now chunk each file's text separately instead of chunking the string representation of the full content dictionary. Tests cover file text, multiple files, and synchronous and asynchronous storage. ChangesKnowledge Source Chunking
Priority: ➖ Normal Severity of issue fixed: Medium
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the main change: correcting CSV and JSON chunking to use file text instead of the content dictionary representation. |
| Description check | ✅ Passed | The description includes all required sections, links issue #7611, explains the change, documents verification results and known optional-dependency failures, and provides additional context. |
| Linked Issues check | ✅ Passed | The PR satisfies the coding requirements in #7611. CSVKnowledgeSource and JSONKnowledgeSource now chunk each file text independently in both add() and aadd(). The tests cover readable CSV and … |
| Out of Scope Changes check | ✅ Passed | The changes stay within #7611. The PR modifies CSV and JSON chunking and adds focused tests for the required behavior. No unrelated change is identified. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create a new PR
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 help to get the list of available commands.
|
Two contributor-side items I cannot close myself — both need a maintainer with write access. 1. CI has not been allowed to run on this PR. On the current head, seven workflow runs are In the meantime, the equivalent evidence I can produce locally: the exact pytest and ruff commands 2. The Three sibling PRs from the same account were prepared the same way and need both items: #7610, #7612, One deliberate non-request: I have not pushed a merge of |
9e19949 to
70a415a
Compare
Related issue
Fixes #7611
Summary
CSVKnowledgeSource.add()/aadd()andJSONKnowledgeSource.add()/aadd()chunkedstr(self.content)— the Python repr of the wholedict[Path, str]— instead of each file's text, so every chunk handed to the embedder carriedPosixPath('...')wrappers, quotes and\\nliterals, and a multi-file source had all of its files merged into one string.Measured on
3831e8b(absolute paths shortened here):After the change:
BaseFileKnowledgeSourcedeclarescontent: dict[Path, str] = Field(init=False, default_factory=dict)and itsmodel_post_initalways assignsself.content = self.load_content(), so for these two sources theisinstance(self.content, dict)branch is not a corner case — it is the only branch that ever runs (contentcannot be passed to the constructor). That makes theelsearm dead and the repr the permanent behavior.The fix drops the repr and iterates the mapping, which is exactly what the three sibling sources already do —
text_file_knowledge_source.py:26/:33,pdf_knowledge_source.py:46/:53,excel_knowledge_source.py:151/:165— so CSV/JSON now match the per-file chunking the rest of the package has. Same four-line shape in bothadd()andaadd()of each class; no change to any signature, field, or stored format.Verification
Tests added or updated for the changed behavior
Relevant tests and quality checks pass locally
Four tests added to
lib/crewai/tests/knowledge/test_knowledge.py: CSV text, JSON text (_json_to_textoutput reaches the chunks), one chunk group per file for a two-file source, and theaadd()path assertingstorage.asave(chunks). All four fail on3831e8bwith the repr shown above and pass with the fix:4 failed, 18 deselectedon the base →2 failed, 20 passedon this branch.uv run pytest lib/crewai/tests/knowledge -q→ 56 passed, 2 failed. The two failures aretest_docling_sourceandtest_multiple_docling_sources, which need the optionaldoclingextra this environment does not have; they fail identically on3831e8bwith this diff reverted (CI installs--all-extras, where they pass).Blast radius:
grep -rn "CSVKnowledgeSource\|JSONKnowledgeSource" lib/returns onlyknowledge.py,source_helper.py(both just register the classes by extension) and this test file, and no existing test calledadd()/aadd()on either source — which is how the corrupted chunks stayed invisible.uv run ruff checkanduv run ruff format --check→ clean on both changed sources.uv run mypy→ no error in either changed file (the 10 reported errors are the pre-existingno-any-unimportedfindings increw_docling_source.py, from that same missing optional extra).Additional context
Retrieval quality is the reason to take this: the corrupted string is what gets embedded, so path noise and escaped newlines were steering similarity search for every CSV/JSON source, and absolute file paths leaked into the chunks that end up in the model's context. For files longer than
chunk_sizethe window also slid over repr offsets, so chunk boundaries fell mid-wording of the repr rather than in the text.JSONKnowledgeSource._json_to_text()is the clearest tell — it builds readablekey: valuelines with indentation, and the oldadd()threw that formatting away one line later.Follow-up this PR deliberately does not attempt:
CrewDoclingSourceandExcelKnowledgeSourcedo their own chunking, andBaseFileKnowledgeSourcecould hoist the per-file chunk loop so a new source cannot forget it again. Both are refactors rather than a bug fix.Authored with an AI coding assistant.
.github/CONTRIBUTING.mdrequires thellm-generatedlabel for agent-authored contributions and external contributors cannot apply it here — maintainers, please add it.Note
Medium Risk
Changes what gets embedded for CSV/JSON knowledge sources, so existing vector indexes built before re-ingest may still hold bad chunks until sources are re-added; behavior is a correctness fix with limited blast radius to those two source types.
Overview
Fixes a bug where CSV and JSON knowledge sources embedded
str(self.content)(the dict repr with path keys and escaped newlines) instead of each file’s parsed text.add()andaadd()on both sources now iterateself.content.values(), chunk each file’s text separately, and extendchunks—matching PDF and text file sources. Multi-file sources produce distinct chunk groups per file rather than one merged repr string.Four unit tests assert chunk contents for CSV/JSON (including async
aadd()and two-file CSV).Reviewed by Cursor Bugbot for commit e50d40f. Bugbot is set up for automated code reviews on this repo. Configure here.