Skip to content

Update usage of get_worker() in tests#1141

Merged
rapids-bot[bot] merged 2 commits intorapidsai:branch-23.04from
pentschev:update-get_worker-usage
Mar 22, 2023
Merged

Update usage of get_worker() in tests#1141
rapids-bot[bot] merged 2 commits intorapidsai:branch-23.04from
pentschev:update-get_worker-usage

Conversation

@pentschev
Copy link
Copy Markdown
Member

@pentschev pentschev commented Mar 21, 2023

In dask/distributed#7580 get_worker was modified to return the worker of a task, thus it cannot be used by client.run, and we must now use dask_worker as the first argument to client.run to obtain the worker.

@pentschev
Update usage of get_worker() in tests
8d5241f 
In dask/distributed#7580 `get_worker` was
modified to return the worker of a task, thus it cannot be used by
`client.run`, and we must now use `dask_worker` as the first argument to
`client.run` to obtain the worker.
@pentschev pentschev requested a review from a team as a code owner March 21, 2023 21:42
@github-actions github-actions bot added the python python code needed label Mar 21, 2023
@pentschev pentschev added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Mar 21, 2023
jacobtomlinson
jacobtomlinson reviewed Mar 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Looks like this test also needs updating

dask-cuda/dask_cuda/tests/test_proxify_host_file.py

Line 418 in ec2de78

async def test_worker_force_spill_to_disk():

@wence-
Copy link
Copy Markdown
Contributor

wence- commented Mar 22, 2023

@madsbk there are usages of get_worker inside the explicit comms implementation. Are these likely to be safe because they're run in a context where there is a task?

@pentschev
Use client.run for async function
884a595 
Using `client.submit` seems not allowed to resolve `get_worker`, but
`client.run` does.
@pentschev
Copy link
Copy Markdown
Member Author

pentschev commented Mar 22, 2023

Are these likely to be safe because they're run in a context where there is a task?

I guess you mean

dask-cuda/dask_cuda/explicit_comms/comms.py

Line 77 in ec2de78

worker: Any = get_worker()
which is called from

dask-cuda/dask_cuda/explicit_comms/comms.py

Lines 92 to 93 in ec2de78

def _run_coroutine_on_worker(sessionId, coroutine, args):
session_state = worker_state(sessionId)
and submitted from

dask-cuda/dask_cuda/explicit_comms/comms.py

Lines 228 to 235 in ec2de78

ret = self.client.submit(
_run_coroutine_on_worker,
self.sessionId,
coroutine,
args,
workers=[worker],
pure=False,
)

The above should be fine as _run_coroutine_on_worker and is executed by client.submit.

madsbk
madsbk approved these changes Mar 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Peter

@wence-
Copy link
Copy Markdown
Contributor

wence- commented Mar 22, 2023

The above should be fine as _run_coroutine_on_worker and is executed by client.submit.

884a595 says "Using client.submit seems not allowed to resolve get_worker, but client.run does."

So this seems in conflict?

@pentschev
Copy link
Copy Markdown
Member Author

pentschev commented Mar 22, 2023

884a595 says "Using client.submit seems not allowed to resolve get_worker, but client.run does."

So this seems in conflict?

I meant that for async functions only, it does work with non-async. I've also commented about that behavior in dask/distributed#7696 (comment) , but I currently don't know if it's possible to resolve, although it shouldn't be a problem for us currently, as we don't use it in actual code, just in that one test.

@wence-
Copy link
Copy Markdown
Contributor

wence- commented Mar 22, 2023

Ah, ok, thanks for the explanation and digging!

@pentschev
Copy link
Copy Markdown
Member Author

pentschev commented Mar 22, 2023

Thanks everyone for reviews!

@pentschev
Copy link
Copy Markdown
Member Author

pentschev commented Mar 22, 2023

/merge

@rapids-bot rapids-bot bot merged commit 06fb4e2 into rapidsai:branch-23.04 Mar 22, 2023
@pentschev
Copy link
Copy Markdown
Member Author

pentschev commented Mar 22, 2023

For completeness in this thread, regarding the substitution of client.submit by client.run, this is what Distributed devs answered in dask/distributed#7696 (comment):

I looked at 884a595 and argue this "workaround" is how it is supposed to be. client.submit is intended to run user functions (on a thread) while client.run is intended to run a function/coroutine on the worker event loop, in the main thread, outside of our task scheduling. If you are actually inspecting the worker in your test, that's how you should do it

@pentschev pentschev deleted the update-get_worker-usage branch May 15, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobtomlinson jacobtomlinson jacobtomlinson left review comments

@madsbk madsbk madsbk approved these changes

@wence- wence- wence- approved these changes

Assignees

No one assigned

Labels

3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change python python code needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pentschev @wence- @madsbk @jacobtomlinson