Skip to content

Conversation

kylediaz
Copy link
Contributor

@kylediaz kylediaz commented Aug 30, 2025

closes #5047

@kylediaz kylediaz marked this pull request as ready for review August 30, 2025 09:06
Copy link
Contributor Author

kylediaz commented Aug 30, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor

propel-code-bot bot commented Aug 30, 2025

Improve Embedding Validation Error Messages

This PR updates the error handling in the embedding normalization logic, providing clearer and more consistent ValueError messages when embeddings validation fails. It ensures empty lists and empty nested lists within embeddings are specifically checked and produce informative, user-friendly feedback.

Key Changes

• Changed the ValueError message for empty embeddings to use embeddings (lowercase) for consistency.
• Added a validation check for empty nested lists within embeddings and a corresponding error message.
• Raised a ValueError when a list of embeddings contains an empty inner list, clarifying that embeddings should be at least 1-dimensional.

Affected Areas

• chromadb/api/types.py (normalize_embeddings function)

This summary was automatically generated by @propel-code-bot

Comment on lines +155 to +162
if len(target[0]) == 0:
raise ValueError("Expected embeddings to be at least 1-dimensional")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BestPractice]

The error message contains an inconsistency with the existing codebase. Based on the related code from the database, this function should raise errors about empty embeddings, but the new validation check for empty nested lists uses different wording than the existing pattern.

The existing validation functions use more specific error messages like:

  • "Expected each embedding in the embeddings to be a 1-dimensional numpy array with at least 1 int/float value. Got a 1-dimensional numpy array with no values at pos {i}"

Consider making the error message more specific and consistent:

Suggested change
if len(target[0]) == 0:
raise ValueError("Expected embeddings to be at least 1-dimensional")
raise ValueError(f"Expected each embedding to be non-empty, got empty embedding at position 0")

Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

@kylediaz kylediaz force-pushed the kylediaz/_enh_embedding_validation_error_message branch from 54b888f to adf844d Compare September 4, 2025 21:25
@kylediaz kylediaz mentioned this pull request Sep 4, 2025
1 task
Copy link
Contributor Author

kylediaz commented Sep 4, 2025

2 Jobs Failed:

PR checks / all-required-pr-checks-passed

Step "Decide whether the needed jobs succeeded or failed" from job "all-required-pr-checks-passed" is failing. The last 20 log lines are:

[...]
}
EOM
)"
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  GITHUB_REPO_NAME: chroma-core/chroma
  PYTHONPATH: /home/runner/_work/_actions/re-actors/alls-green/release/v1/src
# ❌ Some of the required to succeed jobs failed 😢😢😢

📝 Job statuses:
📝 python-tests → ✓ success [required to succeed or be skipped]
📝 python-vulnerability-scan → ✓ success [required to succeed or be skipped]
📝 javascript-client-tests → ✓ success [required to succeed or be skipped]
📝 rust-tests → ✓ success [required to succeed or be skipped]
📝 go-tests → ❌ failure [required to succeed or be skipped]
📝 lint → ✓ success [required to succeed]
📝 check-helm-version-bump → ⬜ skipped [required to succeed or be skipped]
📝 delete-helm-comment → ✓ success [required to succeed or be skipped]
Error: Process completed with exit code 1.
PR checks / Go tests / cluster-test

Step "Run bin/cluster-test.sh bash -c 'cd go && make test'" from job "Go tests / cluster-test" is failing. The last 20 log lines are:

[...]
	github.com/chroma-core/chroma/go/pkg/log/sysdb		coverage: 0.0% of statements
ok  	github.com/chroma-core/chroma/go/pkg/memberlist_manager	4.249s	coverage: 75.6% of statements
	github.com/chroma-core/chroma/go/pkg/proto/coordinatorpb		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/proto/logservicepb		coverage: 0.0% of statements
ok  	github.com/chroma-core/chroma/go/pkg/sysdb/coordinator	4.638s	coverage: 65.5% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/coordinator/model		coverage: 0.0% of statements
ok  	github.com/chroma-core/chroma/go/pkg/sysdb/grpc	4.393s	coverage: 39.6% of statements
ok  	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/db/dao	5.264s	coverage: 50.8% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/db/dao/daotest		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/db/dbcore		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/db/dbmodel		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/db/dbmodel/mocks		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/sysdb/metastore/s3		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/pkg/types		coverage: 0.0% of statements
ok  	github.com/chroma-core/chroma/go/pkg/utils	1.055s	coverage: 31.2% of statements
	github.com/chroma-core/chroma/go/shared/libs		coverage: 0.0% of statements
	github.com/chroma-core/chroma/go/shared/otel		coverage: 0.0% of statements
FAIL
make: *** [Makefile:35: test] Error 1
Error: Process completed with exit code 2.

Summary: 1 successful workflow, 1 failed workflow

Last updated: 2025-09-04 21:50:09 UTC

@kylediaz kylediaz closed this Sep 17, 2025
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.

[Feature Request]: Better error message for empty embeddings
1 participant