Support non p2p configuration when initializing the comms #4543
Conversation
ChuckHastings
added
improvement
| @@ -52,6 +52,21 @@ def dask_client(): | |||
| stop_dask_client(dask_client, dask_cluster) | |||
|
|
|||
|
|
|||
| # FIXME: Add tests leveraging this fixture | |||
| @pytest.fixture(scope="module") | |||
| def dask_client_non_p2p(): | |||
There was a problem hiding this comment.
Do we know if we'd like to run tests that don't create UCX endpoints by hardcoding a different fixture, or would it be better to pass a CLI option, or something else? I think it's okay to add this fixture now if we're going to use it, but I wonder if we should think of an alternative if something like a CLI option to pytest would be better.
There was a problem hiding this comment.
I think we should eliminate the p2p feature entirely. IIRC, the only reason we required the P2P to be configured was because at the time we initially implemented it was required to properly set up the NCCL communication that the C++ library uses. NCCL has been improved so that the P2P feature is no longer required to make this configuration. Joseph's changes here and his testing have validated that belief.
I'd be inclined to merge this PR and create a new issue to address dropping the P2P configuration option and convert all of the tests to configure with the non-P2P configuration.
|
/merge |
rapids-bot
bot
merged commit c941748
into
rapidsai:branch-24.08
closes #4490