Repository navigation
Conversation
…t_length is None OpenAIEmbedder._embed() previously skipped chunk_text_balanced() when self._max_input_length was None (the default across every embedder conf). Any single input longer than cluster_texts()'s hard-coded 75,000-char ceiling then raised an unhandled ValueError, surfacing as HTTP 500 on ingestion of long messages. Replace the conditional guard with an unconditional split that falls back to max_total_input_length_per_request (the same 75,000-char bound cluster_texts() enforces) when max_input_length is None. Every text is now guaranteed to fit before cluster_texts() sees it. Scope is limited to OpenAIEmbedder per architect review: Bedrock and SentenceTransformer embedders do not call cluster_texts() and have no equivalent crash path. Conf defaults remain None by design — no single Unicode code-point value is correct across all supported models. Adds regression test test_embed_oversized_input_with_no_max_input_length covering an 80,000-char input with max_input_length=None. Refs: #1298 Signed-off-by: Steve Scargall <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash path in the server’s OpenAI embedder by ensuring long inputs are always chunked before being passed into the clustering step, preventing unhandled ValueError and resulting HTTP 500s during ingestion of very long messages.
Changes:
- Always chunk inputs in
OpenAIEmbedder._embed()using a fallback bound whenmax_input_lengthisNone. - Add a regression test covering an 80,000-character input with
max_input_length=None.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/server/src/memmachine_server/common/embedder/openai_embedder.py | Ensures inputs are chunked with an effective max length before calling cluster_texts(). |
| packages/server/server_tests/memmachine_server/common/embedder/test_openai_embedder.py | Adds regression test for oversized single input when max_input_length is unset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
edwinyyyu
approved these changes
Apr 15, 2026
Contributor
|
I've tested this PR locally and confirmed the crash described in the issue #1298 is fixed now. I've gone ahead and closed the issue. Thanks. |
edwinyyyu
pushed a commit
to edwinyyyu/MemMachine
that referenced
this pull request
Apr 20, 2026
…t t_length is None (MemMachine#1328) fix(embedder): always chunk text before cluster_texts() when max_input_length is None OpenAIEmbedder._embed() previously skipped chunk_text_balanced() when self._max_input_length was None (the default across every embedder conf). Any single input longer than cluster_texts()'s hard-coded 75,000-char ceiling then raised an unhandled ValueError, surfacing as HTTP 500 on ingestion of long messages. Replace the conditional guard with an unconditional split that falls back to max_total_input_length_per_request (the same 75,000-char bound cluster_texts() enforces) when max_input_length is None. Every text is now guaranteed to fit before cluster_texts() sees it. Scope is limited to OpenAIEmbedder per architect review: Bedrock and SentenceTransformer embedders do not call cluster_texts() and have no equivalent crash path. Conf defaults remain None by design — no single Unicode code-point value is correct across all supported models. Adds regression test test_embed_oversized_input_with_no_max_input_length covering an 80,000-char input with max_input_length=None. Refs: MemMachine#1298 Signed-off-by: Steve Scargall <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the change
OpenAIEmbedder._embed() previously skipped chunk_text_balanced() when self._max_input_length was None (the default across every embedder conf). Any single input longer than cluster_texts()'s hard-coded 75,000-char ceiling then raised an unhandled ValueError, surfacing as HTTP 500 on ingestion of long messages.
Description
Replace the conditional guard with an unconditional split that falls back to max_total_input_length_per_request (the same 75,000-char bound cluster_texts() enforces) when max_input_length is None. Every text is now guaranteed to fit before cluster_texts() sees it.
Scope is limited to OpenAIEmbedder per architect review: Bedrock and SentenceTransformer embedders do not call cluster_texts() and have no equivalent crash path. Conf defaults remain None by design — no single Unicode code-point value is correct across all supported models.
Adds regression test test_embed_oversized_input_with_no_max_input_length covering an 80,000-char input with max_input_length=None.
Fixes/Closes
Fixes #1298
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist
Maintainer Checklist
Screenshots/Gifs
N/A
Further comments
None