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

Revert "Fix co-assignment for binary operations" #6985

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

gjoseph92
Copy link
Collaborator

This reverts commit 7ebd1d9.

Re-opens #6597. See coiled/benchmarks#295 for motivation.

  • Tests added / passed
  • Passes pre-commit run --all-files

tg.last_worker = (
ws if tg.states["released"] + tg.states["waiting"] > 1 else None
)
tg.last_worker_tasks_left -= 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, what it looked like before #6985:

ws = tg.last_worker
if not (ws and tg.last_worker_tasks_left and ws.address in self.workers):
# Last-used worker is full or unknown; pick a new worker for the next few tasks
ws = min(
(self.idle or self.workers).values(),
key=partial(self.worker_objective, ts),
)
assert ws
tg.last_worker_tasks_left = math.floor(
(len(tg) / self.total_nthreads) * ws.nthreads
)
# Record `last_worker`, or clear it on the final task
tg.last_worker = (
ws if tg.states["released"] + tg.states["waiting"] > 1 else None
)
tg.last_worker_tasks_left -= 1
return ws

Note there's still a slight change here—previously we'd schedule onto any worker if none were idle, now we only schedule onto running workers.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @gjoseph92! Have we had a chance to confirm this fixes the observed regression in coiled/benchmarks#295 as expected? Also cc @ian-r-rose in case he's looked into it

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Unit Test Results

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

       14 files   -        1         14 suites   - 1   6h 9m 1s ⏱️ - 23m 51s
  3 082 tests  -        4    2 997 ✔️  -        5    84 💤 ±  0  1 +1 
21 569 runs   - 1 265  20 572 ✔️  - 1 277  996 💤 +11  1 +1 

For more details on these failures, see this check.

Results for commit 3f215fd. ± Comparison against base commit bfc5cfe.

@gjoseph92
Copy link
Collaborator Author

Have we had a chance to confirm this fixes the observed regression

I have not, but will try now.

@gjoseph92
Copy link
Collaborator Author

I've run it manually and confirmed it fixes the regression. I can't figure out how to get it to render with the dashboard script, but here are memory profiles. Notice the difference in peak values:

Before this PR:
after-steal

After this PR:
a5813cfa

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

Thanks @gjoseph92

@fjetter fjetter merged commit acf6078 into dask:main Sep 2, 2022
@jrbourbeau
Copy link
Member

Yeah, thanks for handling this @gjoseph92

@gjoseph92 gjoseph92 deleted the revert-coassignment-fix branch September 6, 2022 18:06
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants