Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tokenize: Don't call str() on dict values #10919

Merged
merged 1 commit into from Feb 14, 2024
Merged

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Feb 12, 2024

@crusaderky crusaderky marked this pull request as draft February 12, 2024 16:01
@crusaderky crusaderky changed the title tokenize: Don't use str() to sort dicts tokenize: Don't call str() on dict values Feb 12, 2024
@lgray
Copy link
Contributor

lgray commented Feb 12, 2024

I think we can drop handling of key=str in set, at least in my case it will not happen, but it does leave a possibility for future unintended problems like this.

@crusaderky crusaderky marked this pull request as ready for review February 12, 2024 16:24
@crusaderky
Copy link
Collaborator Author

I think we can drop handling of key=str in set, at least in my case it will not happen, but it does leave a possibility for future unintended problems like this.

dropping key=str would cause a failure for tokenize({1, (2, 3)}).
Ultimately I would much rather work on the assumption that str() is cheap for all objects.

@lgray
Copy link
Contributor

lgray commented Feb 12, 2024

Sure - I think that's fine and reasonable for set. It may still be useful to stick in the documentation a louder warning/callout/suggestion that __dask_tokenize__ is very much the preferred option. Sorry - confusing the two underlying issues / causes. I blame 9 hours of jetlag.

Copy link
Contributor

github-actions bot commented Feb 12, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

     15 files  ± 0       15 suites  ±0   3h 21m 27s ⏱️ + 5m 9s
 13 024 tests + 3   12 094 ✅ + 3     930 💤 ±0  0 ❌ ±0 
161 047 runs  +45  144 556 ✅ +44  16 491 💤 +1  0 ❌ ±0 

Results for commit bbd0c87. ± Comparison against base commit 07099e5.

♻️ This comment has been updated with latest results.

@crusaderky crusaderky self-assigned this Feb 12, 2024
@phofl phofl merged commit ba5f3bc into dask:main Feb 14, 2024
28 checks passed
@phofl
Copy link
Collaborator

phofl commented Feb 14, 2024

thx @crusaderky

@crusaderky crusaderky deleted the normalize_dict branch February 14, 2024 12:17
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.

None yet

3 participants