Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ±0 15 suites ±0 3h 20m 23s ⏱️ + 1m 7s Results for commit 400a492. ± Comparison against base commit f51fa77. ♻️ This comment has been updated with latest results. |
bb21148 to
400a492
Compare
| assert tokenize(f4) != tokenize(f1) | ||
|
|
||
| # Reordering the inputs must not prevent equality | ||
| # Reordering the inputs must not prevent equality... |
There was a problem hiding this comment.
I think this is a bad idea, but I'll leave revisiting it as out of scope
|
thx |
|
This change has dramatically degraded performance, to make analyses with coffea (via dask-awkward) untenable, see dask-contrib/dask-awkward#468 . Since there is no PR description or related issue, can I ask what the intent is, what's going on here? Is it possible to revert? (to dask-awkward group: we should have nightly dask-dev CI runs) cc @lgray |
|
@martindurant FWIW this PR is mentioned as part of the overall plan in: #10905 |
|
Just to add some context to this --- the change is costly because we treat |
which, if it isn't clear, is the reason for proposing #10918; str() is the wrong thing to tokenise on for a dask-aware object. |
The intent is to make the run_spec (the values of the dask graph) deterministically tokenizable in 99% of the cases. As SubGraphCallable is ubiquitously used during optimization, it becomes essential for this object to be deterministically hashable too. The PR itself can't be reverted fully without throwing #10905 out of the window. Fixed by either #10919 or adding |
|
Thanks @crusaderky . We will report back when everything is in order. For the moment, coffea has pinned dask<2024.2, so this is important but not urgent. |
No description provided.