Skip to content

Fix the failing sentence splitter tests - #4

Merged
IvoLeist merged 1 commit into
IvoLeist:add-langchain_text_splittersfrom
arash77:langchain-text-splitters-sentence-fixes
Aug 6, 2026
Merged

Fix the failing sentence splitter tests#4
IvoLeist merged 1 commit into
IvoLeist:add-langchain_text_splittersfrom
arash77:langchain-text-splitters-sentence-fixes

Conversation

@arash77

@arash77 arash77 commented Aug 6, 2026

Copy link
Copy Markdown

Fixes tests 11, 12 and 14, plus what came up while testing the new sentence splitters. All 26 tests pass locally.

The three failing tests

Each had a different cause, none of them fixable by changing the assertion.

Test 12 — a typo: \A
ASecond sentence ... has a stray A. chunk_0003 also needed a trailing 
. planemo stops at the first failing element, which is why only one of the two ever showed up.

Test 11 — unreachable at chunk_size=23. The sentences are 10, 10, 13 and 12 characters. langchain drops the carried-over sentence until the incoming one fits, so chunk 3 only keeps its overlap at a chunk size of 25 or more. Retuned to 25 / 13, which gives exactly the three chunks the test describes.

Test 14, the empty 4th chunk — not spaCy's fault. sentence_tokens.txt is committed without a trailing newline, but Galaxy's upload appends one, so the tool sees ...five six.\n and the sentencizer reports that lone \n as a fourth sentence. Running the script on the file directly gives 3 chunks, through Galaxy 4.

The same thing explains the chunk texts: with separator="" and strip_whitespace=False the newline stays at the start of the next chunk.

actual:   'one two.' | '\nthree four.' | '\nfive six.' | '\n'
asserted: 'one two.\n' | 'three four.\n' | 'five six.\n'

Rather than assert the junk chunk, the tool now leaves out chunks that hold nothing but whitespace and says so in a warning. A 1-byte chunk carries nothing for a downstream step, and since the upload always appends that newline, almost every real spaCy run would produce one.

Fixed while testing

  • The LookupError handler for missing NLTK Punkt data was unreachable. langchain loads the tokenizer in NLTKTextSplitter.__init__, so the error comes out of build_splitter() and not create_documents(). Users got a traceback instead of the message. KeyError and IndexError are excluded so an unrelated lookup failure is not mislabelled.
  • --spacy-max-length was ignored for the sentencizer. langchain builds it from English() and only forwards max_length on the spacy.load() branch, so any input above one million characters failed with a raw [E088] traceback mentioning parser and NER models that are not even loaded.
  • 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 a shared compute node. Capped, with the cost stated in the help.
  • The input length is now checked before the tiktoken encoding is loaded.
  • The NLTK splitter drops the text after the last sentence, so its chunks do not add up to the input. Now warned about and documented. The spaCy splitter keeps everything.
  • The help section held a TODO where the sentence splitter documentation belongs, and .shed.yml still described the tool as character and token only.

Still open

punkt_tab is not covered by a requirement and there is no package that provides it — conda-forge nltk_data 2022.05.27 ships tokenizers/punkt, not punkt_tab (that format arrived with nltk 3.8.2). On a clean environment all three NLTK tests fail at splitter construction. They pass locally for me only because I copied the resource into the environment by hand, so CI will probably still fail there. The error message now explains the situation instead of showing a traceback, but the real options are vendoring the corpus, building a package, or dropping NLTK and keeping spaCy. Happy to go either way.

Note this branch does not include #3, which is still open and fixes the separator sanitizer and the chunk diagnostics. Those two touch split_text.py in nearby places, so whichever merges second will need a small manual merge — I would suggest merging that one first.

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.
@IvoLeist
IvoLeist merged commit 9aee392 into IvoLeist:add-langchain_text_splitters Aug 6, 2026
8 of 10 checks passed
@arash77
arash77 deleted the langchain-text-splitters-sentence-fixes branch August 6, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants