Repository navigation
fix: DocumentCleaner remove_regex strips only pages where the regex matched - #13360
davidsbatista merged 4 commits into
Conversation
|
Someone is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @Elias-Sun, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
|
孙意博 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
`_remove_regex` called `.strip()` on every page, so `remove_regex` removed indentation and newlines at page edges even when the regex matched nothing and all whitespace cleaning options were disabled. Closes deepset-ai#13357 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e693fde to
3732f75
Compare
|
Thanks for signing the CLA, @Elias-Sun! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
|
Tested this branch (3732f75) against the repro from #13357 on WSL2 / Python 3.14:
LGTM from a user's perspective. Thanks for the quick fix! |
|
Thanks @Elias-Sun! Dropping the Could you strip only the pages where the regex actually matched, using That fixes #13357 without changing behaviour for anyone else. Please also update the "match" test case and drop the upgrade section from the release note. |
Use re.subn and strip a page only when the regex matched at least once, so pages without a match keep their whitespace while existing outputs for matching pages (e.g. footer removal) and their document IDs stay the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @davidsbatista, done in fb8917b:
🤖 Addressed by Claude Code |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
DocumentCleaner remove_regex strips only pages where the regex matched
|
Thanks @Elias-Sun, looks good, all points addressed. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Related Issues
Proposed Changes:
DocumentCleaner._remove_regexcalled.strip()on every\f-separated page after the substitution. As a result,remove_regexremoved indentation and newlines at the start and end of every page, even on pages where the regex matched nothing. This happened even whenremove_extra_whitespaces,remove_empty_linesandstrip_whitespaceswere all disabled.This PR switches to
re.subnand strips a page only when the regex matched at least once on it:\f) are preserved as before (fix: DocumentCleaner: keep the \f in text #8078).The
remove_regexdocstring now describes this behavior.How did you test it?
test_remove_regex_strips_only_matching_pagescovers three cases: no match (whitespace preserved), match (page stripped as before), and multi-page content (only the matching page is stripped).test_remove_regex_with_strip_whitespacescovers a matching page and a non-matching page in the same document withstrip_whitespaces=True.strip_whitespacescases fail onmain. The match case passes on both, since that behavior is unchanged.hatch run test:unit test/components/preprocessors/test_document_cleaner.py test/components/preprocessors/test_document_preprocessor.py: 37 passed.hatch run fmtandhatch run test:typespass.main.Notes for the reviewer
Following review, this no longer changes behavior on pages where the regex matches, so the release note has only a
fixessection.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.🤖 Generated with Claude Code