Merged
Conversation
Member
|
Thanks @pentschev . I'll merge after passing |
Member
|
Is there anything we can put into a test here? |
Member
|
You could put in an invalid UCX config option then check debug output. |
Member
Author
|
Not really, because we don't let users specify those manually and the current code will never encounter a condition where it can fail. We could do a mock test, but I don't think it's worth the time to be honest, we might just wipe that check out instead as in its current state it will never fail (unless there's a change in UCX). |
jakirkham
approved these changes
May 25, 2021
Member
|
I'm not familiar with the UCX code and trust your judgment. If this is fine without a test, go ahead. |
Member
|
Thank you for the comments and review @fjetter |
Member
Author
|
Thanks all for the reviews! |
douglasdavis
pushed a commit
to douglasdavis/distributed
that referenced
this pull request
May 28, 2021
pentschev
added a commit
to pentschev/dask-cuda
that referenced
this pull request
Sep 24, 2021
Support for setting UCX global options was dropped in dask/distributed#4850, as the conflict of Dask configs and UCX configs can be dangerous since both used to live in the same namespace. Setting global UCX options can still be done via environment variables, such as `UCX_*`, and is the preferred method now.
rapids-bot bot
pushed a commit
to rapidsai/dask-cuda
that referenced
this pull request
Sep 24, 2021
Support for setting UCX global options was dropped in dask/distributed#4850, as the conflict of Dask configs and UCX configs can be dangerous since both used to live in the same namespace. Setting global UCX options can still be done via environment variables, such as `UCX_*`, and is the preferred method now. Fixes #627 Authors: - Peter Andreas Entschev (https://github.com/pentschev) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #738
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I believe this check was originally intended to work as a map from
DASK_UCX__*to the actualUCX_*configurations. Since the original implementation the UCX configurations in Dask have evolved to aliases, so the mapping doesn't match. However, theoptionsvariable is what is passed to UCX, so we should verify that we're not setting variables that don't match those that UCX exposes.