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

common/ucx: New UCX common folder to be shared among components #8484

Closed

Conversation

alex--m
Copy link
Contributor

@alex--m alex--m commented Feb 15, 2021

No description provided.

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@yosefe
Copy link
Contributor

yosefe commented Feb 15, 2021

@alex--m according to openucx@43ec4cb, would it be correct to say that the only thing that UCC component uses from common UCX code is opal_common_ucx and its associated init/cleanup functions?

@alex--m
Copy link
Contributor Author

alex--m commented Feb 15, 2021

@alex--m according to openucx@43ec4cb, would it be correct to say that the only thing that UCC component uses from common UCX code is opal_common_ucx and its associated init/cleanup functions?

@yosefe no, I wouldn't say that's accurate. The implementation in openucx@43ec4cb is just a temporary one, and I'd expect a full one would also use the datatype conversion functionality and free-lists in ompi_common_ucx (in addition to the stuff in opal_common_ucx you mentioned). Obviously, like with the UCP-worker reuse issue, using this kind of functionality would also require some UCC API before the UCC component could be finalized.

@yosefe
Copy link
Contributor

yosefe commented Feb 15, 2021

@alex--m according to openucx@43ec4cb, would it be correct to say that the only thing that UCC component uses from common UCX code is opal_common_ucx and its associated init/cleanup functions?

@yosefe no, I wouldn't say that's accurate. The implementation in openucx@43ec4cb is just a temporary one, and I'd expect a full one would also use the datatype conversion functionality and free-lists in ompi_common_ucx (in addition to the stuff in opal_common_ucx you mentioned). Obviously, like with the UCP-worker reuse issue, using this kind of functionality would also require some UCC API before the UCC component could be finalized.

Ok so as we discussed on the confcall, the right way would be to 1st have the underlying UCC APIs that would be used by OpenMPI, and show how the OpenMPI code changes which make use of this API improve performance/memory usage/else.

@alex--m
Copy link
Contributor Author

alex--m commented Feb 15, 2021

@alex--m according to openucx@43ec4cb, would it be correct to say that the only thing that UCC component uses from common UCX code is opal_common_ucx and its associated init/cleanup functions?

@yosefe no, I wouldn't say that's accurate. The implementation in openucx@43ec4cb is just a temporary one, and I'd expect a full one would also use the datatype conversion functionality and free-lists in ompi_common_ucx (in addition to the stuff in opal_common_ucx you mentioned). Obviously, like with the UCP-worker reuse issue, using this kind of functionality would also require some UCC API before the UCC component could be finalized.

Ok so as we discussed on the confcall, the right way would be to 1st have the underlying UCC APIs that would be used by OpenMPI, and show how the OpenMPI code changes which make use of this API improve performance/memory usage/else.

@shamisp is this what you got from the discussion last week? I seem to recall the decision was that Mellanox would review the non-UCC-related changes first, and only then we will proceed with any changes to UCC for OMPI components. If @yosefe 's version is right - we're at a deadlock again, since the UCC WG decided to wait for UCP changes first (see disucssion in this UCC PR ) - which makes sense as UCC relies on UCX and not vice versa.

@alex--m
Copy link
Contributor Author

alex--m commented Feb 15, 2021

My bad - PR was misdirected, replaced with this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants