Repository navigation
fix(anomaly): RSHash slot assignment depends on PYTHONHASHSEED - #449
Draft
nikolas-sapa wants to merge 1 commit into
Draft
nikolas-sapa wants to merge 1 commit into
nikolas-sapa wants to merge 1 commit into
Conversation
RSHashCountMinSketch._indices mapped a discretised vector to a slot with hash((key, payload)). CPython salts hash() for bytes and tuple inputs from PYTHONHASHSEED, which is randomized per process, so a seeded RSHash produced different scores in different processes. Replace the salted hash with zlib.crc32, seeded from the table key. The key still steers its own table, so the detector seed still reaches slot assignment, but the result no longer depends on the interpreter's hash salt. Measured on TinyBlobs, RSHash AUC with seed=42 is 0.781305 before and after, so the 0.78 pin in tests/test_anomaly_detectors.py does not move. Individual scores do change (6 of 1100 on TinyBlobs), so this is a behaviour change for existing users. Add tests/test_rshash_reproducibility.py, which asserts reproducibility across processes with different PYTHONHASHSEED values, that the sketch keys keep the tables independent, and that the detector still consumes its seed. Assisted-by: opencode:space-bunny-free
This branch has not been deployed
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.
Problem
RSHashis reproducible in one process but not across processes. Two runs of the same seeded detector on the same stream produce different scores.Cause:
RSHashCountMinSketch._indicesmaps a discretised vector to a slot withhash((key, payload)), wherepayloadisbytes. CPython saltshash()forbytesandtupleinputs fromPYTHONHASHSEED, which is randomized per process by default.Same code, same seed, only the env var differs (
m=300, s=64, w=4, p=10000, 300 instances of TinyBlobs, score trace digested with sha256):The existing test suite cannot catch this. Within one process the salt is constant, so the trace is self-consistent either way. The only observable effect is that two vectors sometimes land in the same slot under one salt and not another, which changes a sparse set of scores. The 0.78 AUC pin in
tests/test_anomaly_detectors.pyis unaffected.Fix
One line, using
zlib.crc32as its initial register:hash()took the key as part of the tuple it hashed.crc32takes the key as its initial register instead, so the same property holds: each of thewkeys steers its own table, and the detector seed still reaches slot assignment throughhash_keys.key ^ (key >> 32)folds the 63-bit key down to the 32 bitscrc32accepts.This changes scores for existing users
Slot assignment changes for every vector, so individual scores move. On TinyBlobs (1100 instances,
m=300, s=256, w=4, p=10000, seed=42), 6 of 1100 scores change.The AUC pin does not move:
So
tests/test_anomaly_detectors.pystays at 0.78. The default-constructed detector (s=1000, window longer than the stream) also stays at 0.504595 on both sides.Anyone relying on exact score values, or on a stored ranking, will see different numbers. Anyone relying on AUC or on reproducibility gets strictly better behaviour.
Cost
_indicesdominates this detector: over 1100 instances atm=300, s=1000, w=4, p=10000it is called 390,000 times, computing 1,560,000 individual slot hashes.Slot assignment only, 80k single-key index computations, min of 7:
End to end,
RSHash(m=300, s=1000, w=4, p=10000)over 3000 instances:Regression test
tests/test_rshash_reproducibility.py, four tests:test_rshash_is_reproducible_across_processes[1,12345]runs a seeded detector in a subprocess under an explicitPYTHONHASHSEEDand compares score-trace digests across processes. Verified in both directions: fails on the current code (2 failed), passes after the fix.test_table_keys_keep_the_tables_independentasserts thewkeys still steer distinct slots and that distinct payloads do not collapse. This one is load bearing: a seed-blindzlib.crc32(payload, 0)passes the other three tests and only fails here.test_rshash_still_consumes_its_seedasserts seed 42 and seed 1 still differ, so cross-process stability is not bought by dropping the seed.Suite results:
Relation to #432
This was offered on #432 as a follow-up while working through the seeding fixes there, and #432's own open question to the maintainer is still pending. This PR does not fix, depend on, or supersede #432: it only touches
_indicesand is independent of therandom_seedwiring. The 0.78 AUC pin is unchanged, so it does not conflict with that work.Draft on purpose
This is a behaviour change that alters results for existing users, and it costs about 1.1x end to end. It should not land without maintainer sign-off, so it is opened as a draft.
zlib.crc32was the recommended option in the original offer; a keyedblake2bwas the alternative at roughly 3x thecrc32slot cost.Assisted-by: opencode:space-bunny-free