Add langchain_text_splitters - #1947
Conversation
- Remove the folder dev_utils and its content - Remove .gitignore inside the test-data folder - Merge macros_for_testing.xml into macros.xml - Remove macro for setting the TIKTOKEN_CACHE_DIR env var - Add "AI4Social+" to the creators - Addresss flake8 warnings
|
@bgruening thanks for the quick first review :) Here are the changes:
|
|
Thank you @IvoLeist, nice tool! A few issues to fix before merge, found by AI review (verified by running the script with the pinned deps). Must fix (silent wrong output)
Should fix (crashes)
Nice to fix
Tests to addEmpty input, whitespace-only input, non-UTF-8 input, CRLF/CR input, character+token mode with a disallowed special token, and a TSV round-trip check (the current |
|
I fixed the points from my review locally and ran the tests. 16/16 passed before, 20/20 after. Two more real bugs came up while testing. Correction: do not use
|
Input handling: - Decode the input as UTF-8 instead of Path.read_text(), which silently rewrote CRLF/CR line endings and shifted the reported start indices. - Report invalid UTF-8 input with a clear message instead of a traceback. - Reject input that is empty or whitespace only instead of writing empty outputs. TSV output: - Escape backslash, tab, carriage return and newline so that the chunk text stays on one row and the escaping can be reversed. The previous escaping could not be undone, and csv.writer additionally quoted any chunk containing a double quote. Errors and metadata: - Report a disallowed special token with a clear message on the character splitter as well, not only on the token splitter. - Report an invalid start index as null instead of replacing the number with a string, so the JSON field keeps a single type. The warning on stdout is unchanged. - Use a single "length" key per chunk together with the existing "length_unit" instead of character_count/token_count. - Report strip_whitespace as false in token mode, where the splitter never applies it. - Write all outputs as UTF-8 instead of the system locale encoding. Command line and separators: - Pass a custom separator as --separator=VALUE so that a separator starting with a dash is not read as an option. - Reject unsupported and out-of-range custom separator escapes with a clear message instead of crashing or passing them through silently. - Make --encoding-name and --model-name mutually exclusive. Tests and docs: - Add tests for whitespace-only input, a disallowed special token on the character splitter, a dash-prefixed custom separator, and a predefined separator inside the repeat. - Expect null start indices in the two overlapping token tests, which hit the same upstream bug as the dedicated repro test. - Document the tiktoken encoding cache, the TSV escaping and the JSON fields in the tool help. - Deduplicate the two tiktoken command line blocks into a macro.
Very interesting finding 🤓 I went through some langchain issues/stopped PRs regarding it (see e.g. langchain-ai/langchain#29884) and apparently this is not easy to fix. So 🤞 that they succeed at one point ! Since this start_index information is nice to have (from the users perspective) but in my opinion not critical I would (for now) comment out the failing asserts including a reference to the upstream bug issue. So we/potential future maintainers could reactivate these assert easily ;D |
Contributions from Arash :)
- Add NLTKTextSplitter and SpacyTextSplitter - Add Arash as creator *Note this is so far still a proof of concept. There are no spaCy or NLTK tests yet. So not merge ready.
|
Latest update:
*Note this is so far only a proof of concept. There are no spaCy or NLTK tests yet. So not merge ready. |
05f283c to
d80e5c7
Compare
Tests 11, 12 and 14 could not be fixed by adjusting the assertions, each had a different cause. Test 12 asserted "\nASecond sentence ..." with a stray A, and its third element missed the trailing newline. planemo stops at the first failing element, so only one of the two showed up. Test 11 is unreachable at chunk size 23. The sentences are 10, 10, 13 and 12 characters long and langchain drops the carried over sentence until the incoming one fits, so the third chunk only keeps the overlap at a chunk size of 25 or more. Retuned to 25 / 13. Test 14 ended in a fourth chunk holding a single newline. Galaxy's upload appends a trailing newline to the input, and the sentencizer reports it as a sentence of its own. Since the sentence splitters keep the whitespace between the sentences and never strip a chunk, that newline survived into the outputs. Chunks that hold nothing but whitespace are now left out and reported in a warning, so the test is back to three chunks. Fixes found while testing the new sentence splitters: - The LookupError handler for the missing NLTK Punkt data was unreachable. langchain loads the tokenizer in the constructor, so the error is raised in build_splitter() and not in create_documents(). KeyError and IndexError are excluded so that an unrelated lookup failure is not reported as missing Punkt data. - --spacy-max-length was ignored for the sentencizer, which langchain builds from English() without forwarding max_length. Any input above one million characters failed with a raw spaCy traceback. - The maximum input length was unbounded. en_core_web_sm keeps the dependency parser and needs roughly 3.5 kB per input character, so a large value could exhaust the memory of a shared compute node. - The input length is now checked before the tiktoken encoding is loaded, so a job that cannot run does not pay for it first. - The NLTK splitter drops the text after the last detected sentence, so its chunks do not add up to the input. This is now reported in a warning and documented. The spaCy splitter keeps everything. - The help section still held a TODO instead of the sentence splitter documentation, and .shed.yml did not mention sentence splitting. Note that punkt_tab is still not covered by a requirement. The conda-forge nltk_data package ships punkt and not punkt_tab, so the NLTK splitter keeps failing where the resource is not provided by the instance. The error message now says so instead of showing a traceback.
…fixes Arash fixed the failing sentence splitter tests :)
…he separator input
Chunk diagnostics:
- Detect a start index that is invalid without being negative. Once the
splitter returns one negative index, the positions it derives from it stay
positive but point at an earlier part of the input, which was reported as if
it were correct. The position is now checked for order and for content.
- Report text that the splitter altered separately from a wrong position.
Splitting between tokens can cut a character that is encoded in several
bytes, which replaces it with the Unicode replacement character. Those chunks
used to be reported as an invalid start index, pointing the user at the wrong
cause.
Custom separator:
- Widen the sanitizer so that characters such as | ; ~ % & $ @ < > " [ ] { }
reach the script instead of being silently replaced by an X, which split the
text at the wrong places.
- Reject a separator that still contains a character Galaxy cannot pass on
unchanged, with a message pointing at the escape syntax.
Other:
- Write the JSON output as UTF-8 as well, instead of the system locale
encoding.
- Let special_token_error() return the error to raise, so the special token
handling has a single contract instead of an unreachable re-raise at both
call sites.
Tests:
- Add a test for a separator that the sanitizer used to mangle.
Arash separated the chunk diagnostics and hardened the separator input
c7b7c6c to
f97dd61
Compare
|
Note, before Arash's PR conda-forge/nltk_data-feedstock#10 is not merged the NLTK TextSplitter can only be tested locally: First you would need to build the Then you could build the package with: cd nltk_data-feedstock
python build-locally.pyNote You need some patience building it can take a while.... You can run the tests as follows: planemo test \
--conda_dependency_resolution \
--conda_auto_install \
--conda_ensure_channels \
"file:///home/<user>/<path_to_feedstock>/build_artifacts,conda-forge,bioconda" It should return:
Or test it in your local galaxy instance by setting the following in your config/galaxy.yml conda_ensure_channels: file:///home/<user>/<path_to_feedstock>/build_artifacts,conda-forge,bioconda
conda_auto_install: truebefore running it with: |
|
To keep everyone in the loop: |
- Add strip_whitespace option for SpacyTextSpliiter; refer to it in the tool help and change the params of one test case to strip_whitespace=True - Wrap strip_whitespace in macro - Add explanation why strip_whitespace is fixed to false for the NLTK text splitter - Populate the NLTK language options with all the in punkt_tab available 19 languages - Set the chunk_overlap default to "0" - Add a TODO related to galaxy adding by default a "\n" to each upload when there is no terminal new line and made one text fail on purpose so we remember to address this before the merge
|
Latest changes:
Regarding the still open TODO this is what I learned so far. if last_block and last_block[-1] != NEWLINE_BYTE:
converted_newlines = True
i += 1
fp.write(b"\n")https://github.com/galaxyproject/galaxy/blob/release_26.1/lib/galaxy/datatypes/sniff.py#L166-L169 A simple: However, just not removing the "\n" potentially messes with the splitting as indicated by our
To reproduce use sentence_nltk_english.txt of our test-data, with target chunk size in characters: 30 |
|
Does your test data contain windows line breaks? |
No, I just double checked: cd test-data/
file *
|
…itters Two bugs silently changed the user's text. spaCy with "strip whitespace" deleted the space between sentences. langchain strips every sentence before joining them and we join with an empty separator, so "mat. The" became "mat.The". The chunk then no longer occurred in the input, so its start index was reported as null. spaCy is now built with strip_whitespace=False and the stripping is done on the finished chunk, which is what the option promises and keeps the chunk a slice of the input. Whitespace-only chunks were dropped for every splitter. A run of blank lines is content for the token and character splitters, and dropping it renumbered the chunks that followed, so 64 characters of a chapter break disappeared. They are now only dropped when stripping was asked for and nothing is left. Also: - the spaCy input limit is set per pipeline; one limit of 10 million characters allowed about 44 GB with the English model - the "text does not occur in the input" warning no longer claims token splitting is the only cause - the NLTK dropped-text check uses the chunk's start index instead of rfind(), which found the last occurrence and so reported no loss when the dropped text repeated the final chunk - strip_whitespace is reported once from the argument instead of being hardcoded per splitter - the punkt_tab notes match the nltk_data requirement - test 9 pins the NLTK warning instead of failing on purpose - new test on space separated prose; every other sentence fixture separates with a line break, which is why this was never caught
…e-fixes Fix whitespace handling in the sentence splitters
- add disclaimer that strip_whitespace is not carried by langchain but by the main function in our python wrapper
…two misleading warnings - fail early when the settings would produce more chunks than the instance's max_discovered_files, instead of splitting everything and then letting Galaxy fail the job on the file count - name the cause that fits the run in the altered-text warning instead of always blaming multi-byte tokens - stop reporting a null start index for overlapping chunks that legitimately begin at the same offset - set strip_whitespace once for every splitter instead of per branch - correct the help on whitespace at a chunk boundary, and let three tests assert the warning instead of pinning the upstream -1
Another round of contributions from Arash
The function picks between two causes for the same symptom, but nothing tested that it picks the right one. - multibyte_persian.txt with the token splitter: gpt2 splits Persian into very small tokens, so a cut lands inside a multi-byte character and the chunk comes back with the replacement character. - repeated_separator.txt with a discarded comma: the run of separators is rebuilt as a single one, so the chunk text no longer occurs in the input. Each test asserts the cause it expects and that the other one is absent, so swapping the two would fail.
Add test cases for altered_text_cause
…chain's strip whitespace option but our own logic
|
really cool, thanks so much @IvoLeist @arash77 @anuprulez and @bgruening ! |
5 participants
This PR starts the addition of langchain-text-splitters, a script collection for breaking large text into smaller chunks for downstream LLM tools. In the GalaxyEU context these could be e.g. LLM Hub or RAG Retriever.
Leveraged Methods of
langchain-text-splittersfor this initial PRNLTKTextSplitter: Uses NLTK's Punkt sentence tokenizer, which is trained per language and therefore handles language-specific abbreviations such asDr.in English orz.B.in German. Select the language of the input.SpacyTextSplitter: Uses spaCy. The rule-based sentencizer is fast and needs no model, but it applies English tokenization rules. Theen_core_web_smmodel is more accurate on English text, and is roughly five times slower and needs about ten times more memory.Library for Token Counting
OpenAI's fast Byte-Pair Encoding (BPE) tokenizer tiktoken library is used to determine the number of tokens in a chunk for this proof of concept. *
langchain-text-splittersis also allowing Hugging Face tokenizer to count length, consequently this is on the roadmap, but not (yet) part of this PR.FOR CONTRIBUTOR:
There are two labels that allow to ignore specific (false positive) tool linter errors:
skip-version-check: Use it if only a subset of the tools has been updated in a suite.skip-url-check: Use it if github CI sees 403 errors, but the URLs work.