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

Update usage of get_worker() in tests #1141

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

pentschev
Copy link
Member

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.

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
Copy link
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

async def test_worker_force_spill_to_disk():

@wence-
Copy link
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?

Using `client.submit` seems not allowed to resolve `get_worker`, but
`client.run` does.
@pentschev
Copy link
Member Author

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

I guess you mean

worker: Any = get_worker()
which is called from
def _run_coroutine_on_worker(sessionId, coroutine, args):
session_state = worker_state(sessionId)
and submitted from
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.

Copy link
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
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
Member Author

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
Contributor

wence- commented Mar 22, 2023

Ah, ok, thanks for the explanation and digging!

@pentschev
Copy link
Member Author

Thanks everyone for reviews!

@pentschev
Copy link
Member Author

/merge

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

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
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
Development

Successfully merging this pull request may close these issues.

4 participants