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

Partial revert of compute-task message format #6626

Merged

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Jun 24, 2022

This is a hotfix for #6624 by reverting the compute-task message format almost to the original state before #6410

I didn't debug very deeply but it appears that our (de-)serializer cannot handle nested Serialized objects very well or something else is messed up. By formatting the run_spec value as a dict the way #6410 did appears to be a compatible change but the nesting is destroyed as soon as it reaches the worker.

basically

# Scheduler side

dct_scheduler["run_spec"] = {"function" b"foo", "args": b"bar", "kwargs": "baz"}

# becomes on worker side

dct_worker["run_spec"] = deserialize(dct_scheduler["args"])

@fjetter fjetter requested a review from crusaderky June 24, 2022 10:51
@fjetter
Copy link
Member Author

fjetter commented Jun 24, 2022

@jakirkham maybe you have an idea what's going on here?

@fjetter fjetter requested a review from gjoseph92 June 24, 2022 10:52
Comment on lines +3657 to +3658
with pytest.raises(IndexError):
overlapped.compute()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is obviously not ideal but I simply took the reproducer from #6624
I do not hit the serialization error anymore but rather an IndexError ¯_(ツ)_/¯

@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2022

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   10h 8m 29s ⏱️ - 24m 59s
  2 891 tests +1    2 808 ✔️ +4    83 💤 ±0  0  - 2 
21 416 runs  +8  20 450 ✔️ +8  966 💤 +4  0  - 3 

Results for commit 27f9f67. ± Comparison against base commit dc019ed.

♻️ This comment has been updated with latest results.

@fjetter fjetter mentioned this pull request Jun 24, 2022
9 tasks
@fjetter
Copy link
Member Author

fjetter commented Jun 24, 2022

All green 🎉 👀

Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This seems like a nice clean solution to me!

Comment on lines +7324 to +7327
"run_spec": None,
"function": None,
"args": None,
"kwargs": None,
Copy link
Member

Choose a reason for hiding this comment

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

I don't 100% see the point of initializing these all with None but I also don't see the harm in it.

@jsignell
Copy link
Member

Shall I go ahead and merge this @fjetter or do you think it needs another set of eyes?

@jsignell
Copy link
Member

merging

@jsignell jsignell merged commit a156c35 into dask:main Jun 24, 2022
@fjetter fjetter deleted the partial_revert_compute_task_message_format branch June 24, 2022 15:29
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.

map_overlap tasks fail to deserialize on workers - keywords must be strings
2 participants