fix(vector-io): handle missing document_id in insert_chunks #3521
Conversation
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
meta-cla
bot
added
the
CLA Signed
skamenan7
requested review from
ashwinb,
bbrowning,
ehhuang,
hardikjshah,
leseb,
mattf,
raghotham,
reluctantfuturist,
slekkala1,
terrytangyuan and
yanxi0830
as code owners
|
cc: @leseb as per our discussions here is the bug fix. Thanks. |
ehhuang
reviewed
Sep 23, 2025
Fixed KeyError when chunks don't have document_id in metadata or chunk_metadata. Updated logging to safely extract document_id using getattr and RAG memory to handle different document_id locations. Added test for missing document_id scenarios. Fixes issue llamastack#3494 where /v1/vector-io/insert would crash with KeyError.
- Gate debug logging behind isEnabledFor check to avoid unnecessary computation - Add Chunk.document_id property to safely handle metadata/chunk_metadata extraction - Simplify RAG memory code using new property
skamenan7
force-pushed
the
fix/vector-io-document-id-keyerror-3494
branch
from
db366c4 to
2510bd3
Compare
Required by project logging rules to use logging.DEBUG constant
leseb
reviewed
Oct 3, 2025
ehhuang
requested changes
Oct 6, 2025
franciscojavierarceo
previously approved these changes
Oct 7, 2025
franciscojavierarceo
requested changes
Oct 7, 2025
Simplified logging to always execute instead of gating behind isEnabledFor check. Main fix is using safe chunk.document_id property instead of direct metadata access.
Added back the return statement in insert_chunks for consistency with the document_id property pattern where None is explicitly returned.
franciscojavierarceo
approved these changes
Oct 7, 2025
leseb
reviewed
Oct 9, 2025
Address PR feedback to validate document_id type when accessed from metadata dict. Since metadata is dict[str, Any], we now fail fast with a clear TypeError when document_id exists but isn't a string, rather than silently returning None. Also removed redundant isinstance check for chunk_metadata.document_id since Pydantic already guarantees it's str | None. Added test coverage for the new validation behavior.
leseb
approved these changes
Oct 10, 2025
|
@ehhuang your comments have been taken care of, final look? Thanks! |
ehhuang
approved these changes
Oct 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixed KeyError when chunks don't have document_id in metadata or chunk_metadata. Updated logging to safely extract document_id using getattr and RAG memory to handle different document_id locations. Added test for missing document_id scenarios.
Fixes issue #3494 where /v1/vector-io/insert would crash with KeyError.
Fixed KeyError when chunks don't have document_id in metadata or chunk_metadata. Updated logging to safely extract document_id using getattr and RAG memory to handle different document_id locations. Added test for missing document_id scenarios.
What does this PR do?
Fixes a KeyError crash in
/v1/vector-io/insertwhen chunks are missingdocument_idfields. The APIwas failing even though
document_idis optional according to the schema.Closes #3494
Test Plan
Before fix:
/v1/vector-io/insertwith chunks → 500 KeyErrordocument_idwas placedAfter fix:
document_idscenarios