Skip to content

Encode string before xxhash in sagemaker boto3 mixin (fixes xxhash 4.0 TypeError) - #3462

Merged
pingsutw merged 1 commit into
masterfrom
fix-sagemaker-xxhash-encode
Aug 18, 2026
Merged

Encode string before xxhash in sagemaker boto3 mixin (fixes xxhash 4.0 TypeError)#3462
pingsutw merged 1 commit into
masterfrom
fix-sagemaker-xxhash-encode

Conversation

@pingsutw

Copy link
Copy Markdown
Member

Why are the changes needed?

build-plugins (flytekit-aws-sagemaker) is failing on master:

>  hash = xxhash.xxh64(sorted_dict_str(updated_config)).hexdigest()
E  TypeError: Strings must be encoded before hashing

4 failed, 140 passed

xxhash 4.0 removed the implicit encoding of str inputs, so xxh64() on a
string now raises. The plugin pins xxhash unbounded (plugins/flytekit-aws-sagemaker/setup.py),
so CI picked up 4.x and every code path that computes an idempotence token broke.

Failing tests: test_call_with_idempotence_token,
test_call_with_truncated_idempotence_token,
test_call_with_truncated_idempotence_token_as_input,
test_call_hashes_normalised_integer_values_consistently.

What changes were proposed in this pull request?

Encode the config string as utf-8 before hashing:

hash = xxhash.xxh64(sorted_dict_str(updated_config).encode("utf-8")).hexdigest()

utf-8 is exactly what xxhash <4 did internally, so the digests are unchanged —
existing idempotence tokens (and the tokens asserted in the tests) stay the
same, and this works on both xxhash 3.x and 4.x. Pinning xxhash<4 in setup.py
would also make CI green, but it leaves the latent bug in place, so I fixed the
call instead.

Verified the digest is stable across versions:

xxhash 3.8.1:  xxh64(s).hexdigest()                  -> 8d9d5330fea0133b
xxhash 3.8.1:  xxh64(s.encode("utf-8")).hexdigest()  -> 8d9d5330fea0133b

How was this patch tested?

No new tests — this restores 4 existing tests that fail on master. Ran the
suite locally against xxhash 4.0.1:

# without the fix
4 failed, 6 passed in 0.72s

# with the fix
10 passed in 1.52s

The token assertions (2de338ec95ee0da6, 1a25e4bb988697a7, ce735d6a183643f1)
pass unchanged, confirming the fix does not alter existing hashes.

Check all the applicable boxes

  • I updated the documentation accordingly. (n/a)
  • All new and existing tests passed.
  • All commits are signed-off.

xxhash 4.0 dropped implicit str encoding, so xxh64() on a str now
raises TypeError: Strings must be encoded before hashing. Encode the
config string as utf-8, which is what xxhash <4 did internally, so the
idempotence tokens are unchanged.

Signed-off-by: Kevin Su <pingsutw@apache.org>
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.00%. Comparing base (5f18ae7) to head (49cba7c).

❗ There is a different number of reports uploaded between BASE (5f18ae7) and HEAD (49cba7c). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (5f18ae7) HEAD (49cba7c)
16 13
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3462       +/-   ##
===========================================
- Coverage   76.27%   52.00%   -24.27%     
===========================================
  Files         217      329      +112     
  Lines       22943    29585     +6642     
  Branches     3036     3036               
===========================================
- Hits        17499    15387     -2112     
- Misses       4556    14069     +9513     
+ Partials      888      129      -759     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pingsutw
pingsutw merged commit fdac8d9 into master Aug 18, 2026
56 of 59 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