Skip to content

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Jul 15, 2021

Follow up to #5057

Closes #5056

I don't really understand why I wrote the test so complicated yesterday. The issue is very easily reproducible this way. For some reason this was not successful, yesterday 🤷‍♂️

nthreads=[("127.0.0.1", 1) for _ in range(10)],
# typical runtime just 2-3s but on CI this may increase significantly
timeout=60,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

dacorator can now fit on 1 line

Copy link
Member Author

Choose a reason for hiding this comment

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

well, keeping an additional line reduces diffs if the decorator is ever changed again. I personally also consider this to be easier readable.
The point of having an auto formatter like black running is to not have to argue about formatting. Since black is fine with this, I'll keep it as is

while not s.tasks:
await asyncio.sleep(0.001)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant with the following sleep

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the sleep after the cancel seems unnecessary; why don't you simply start bombarding the workers with heartbeat requests straight away?

    while s.tasks:
        await asyncio.sleep(0.001)

Copy link
Collaborator

Choose a reason for hiding this comment

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

while sum(w.executing_count for w in workers) < len(workers) / 2:

Why don't you wait for all workers fullstop? (remove the / 2). slowinc should reliably fill the whole cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@fjetter fjetter force-pushed the simple_test_worker_heartbeat_after_cancel branch from 9107e8e to 48e4577 Compare July 15, 2021 10:15
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

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

approved conditionally to getting green tests

@crusaderky
Copy link
Collaborator

github actions seems to be down?

@fjetter
Copy link
Member Author

fjetter commented Jul 15, 2021

github actions seems to be down?

I believe this just looks funny because there are still tasks of a previous commit to be cancelled

It's coming in while typing...

@fjetter
Copy link
Member Author

fjetter commented Jul 15, 2021

Looking at all actions, this workflow was marked with a clock opposed to a yellow / red / green dot. The checks summary above doesn't seem to pick this up

@fjetter
Copy link
Member Author

fjetter commented Jul 15, 2021

This is incredibly satisfying ❤️

image

@fjetter fjetter merged commit 4734833 into dask:main Jul 15, 2021
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.

test_worker_heartbeat_after_cancel hangs on MacOS 3.7/3.8

2 participants