Skip to content

fix(tests): the mock embedding provider must not depend on the hash seed - #223

Merged
acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/mock-embedding-provider-hash-is-per-process
Sep 7, 2026
Merged

acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/mock-embedding-provider-hash-is-per-process

Conversation

@RobertSigmundsson

@RobertSigmundsson RobertSigmundsson commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • MockEmbeddingProvider derives its vector from the built-in hash(), which CPython salts per
    interpreter, so the mock returns different vectors from run to run.
  • With a modulus of 1000, two different texts collide roughly once in a thousand runs, and
    test_different_texts_differ then fails on two byte-identical vectors.
  • blake2b gives the same digest in every process, so a collision between two texts is now a
    property of the texts rather than of the seed the job happened to draw.

Why

Test (Python 3.11) failed on a pull request whose diff touched unified_config.py and a
consolidation test, several directories away from embeddings:

tests/unit/test_embedding_provider.py:134: in test_different_texts_differ
    assert v1 != v2
E   assert [0.4987766778715924, …] != [0.4987766778715924, …]

A mock written for determinism should be deterministic; a failure whose cause is the hash seed
teaches contributors to re-run the job rather than read it, which is a habit worth avoiding.

Changes

  • tests/unit/test_embedding_provider.py: MockEmbeddingProvider.embed derives h from
    hashlib.blake2b(text.encode("utf-8"), digest_size=8) instead of hash(text).
  • The same file gains test_embedding_does_not_depend_on_the_hash_seed, which pins the vectors
    for "text a" and "text b". On a salted hash it fails under every seed but the one that
    produced the constants, so the old behaviour cannot return unnoticed.

Test plan

  • PYTHONHASHSEED=99 pytest tests/unit/test_embedding_provider.py on main1 failed, 82 passed, the failure being test_different_texts_differ with two identical vectors.
    Seed 99 is simply one that collides; most seeds pass, which is the bug.
  • The same command on this branch — 84 passed. Also green under seeds 0, 1, 7, 12345 and
    424242, each 84 passed.
  • With the change reverted and both tests kept, test_embedding_does_not_depend_on_the_hash_seed
    fails under all four seeds tried, and seed 99 additionally fails test_different_texts_differ.
  • ruff check src/ tests/ clean; ruff format --check src/ tests/ reports 739 files already
    formatted.
  • mypy src/ --ignore-missing-imports — success, no issues found in 354 source files.
  • pytest tests/ --timeout=120 -m "not stress" against a live SurrealDB v3.2.0 on a freshly
    created namespace, serial as the Integration (SurrealDB) job runs it — 7286 passed, 48
    skipped, 1 xfailed, which is main's 7285 measured in the same environment plus the one
    test added here.
  • CHANGELOG.md untouched — left to the release entry.

Related issues

Reported in #226.

Verified by @RobertSigmundsson

MockEmbeddingProvider derives its vector from the built-in hash(), which
Python salts per interpreter. Two different texts therefore collide modulo
1000 about once in a thousand runs, and test_different_texts_differ fails
with two identical vectors on a change that never touched embeddings.

blake2b gives the same digest in every process, so the mock is deterministic
and a collision between two texts either exists or never does. A new test
pins the vectors for "text a" and "text b"; on a salted hash it fails under
every seed but the one that produced the constants.

@acidkill acidkill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed the full diff. blake2b with pinned constants is the right fix — deterministic across processes, and the regression test fails under a salted hash() regardless of seed. Green CI.

@acidkill
acidkill merged commit d40434c into acidkill:main Sep 7, 2026
9 checks passed
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