Skip to content

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Oct 23, 2020

This fixes a regression introduced in #4107

I believe this scenario can only be encountered in some race condition scenarios which is why I had a hard time constructing a test case which is less close to the implementation.

@gforsyth
Copy link
Contributor

This fix looks correct to me -- I hadn't hit this particular edge case while working on #4107 but the key in self.tasks check is prevalent throughout. The Travis failure is spurious -- if someone can restart the windows 3.6 build, I'm curious to see if that is a legitimate failure or just a bit of flakiness.

@jrbourbeau
Copy link
Member

if someone can restart the windows 3.6 build, I'm curious to see if that is a legitimate failure or just a bit of flakiness

I pushed an empty commit so we can still see the initial CI failure (hope that's okay @fjetter)

Copy link
Contributor

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Great. Looks like the windows failure was a fluke -- once this passes on Travis should be good to go in.

@fjetter
Copy link
Member Author

fjetter commented Oct 26, 2020

Builds still fail and run into connection timeout errors. I'm wondering if this connects to #4176 :/

@mrocklin
Copy link
Member

Thank you for identifying and fixing this @fjetter

Builds still fail and run into connection timeout errors. I'm wondering if this connects to #4176 :/

In situations like these I recommend submitting a fresh PR to test CI and see if the issue repeats there. If so, we should raise an issue, but can go ahead and merge this in.

@jrbourbeau
Copy link
Member

There's already an issue for test_broken_worker_during_computation (xref #4173), so I'll go ahead and merge this PR in

@jrbourbeau jrbourbeau merged commit b7a2274 into dask:master Oct 27, 2020
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.

4 participants