Skip to content

UCP: Add API to query datatype attributes#8120

Merged
yosefe merged 1 commit intoopenucx:masterfrom
rdietric:ucp_datatype/query_size_api
May 26, 2022
Merged

UCP: Add API to query datatype attributes#8120
yosefe merged 1 commit intoopenucx:masterfrom
rdietric:ucp_datatype/query_size_api

Conversation

@rdietric
Copy link
Copy Markdown

@rdietric rdietric commented Apr 13, 2022

What

This PR adds a new public API function to query attributes for a UCP datatype. Currently, the packed size is the only attribute that can be queried.

Why ?

The size of a message or data transfer is a performance-relevant information. Performance tools should be able to query this information. MPI provides MPI_Type_size to query the size of any MPI datatype.

How ?

A follow-up PR will implement ucp_dt_query in src/ucp/dt/dt.c. Unfortunately, the size of a UCP generic datatype is not available before it has been used in a UCP communication operation without more significant API changes.

@shamisp
Copy link
Copy Markdown
Contributor

shamisp commented Apr 13, 2022

Hi @rdietric. Thanks for sharing your API proposal. Do you have signed CLA with the project ? Corporate and private CLAs can be found here: https://openucx.org/license/

Guidance to contributors is here: https://github.com/openucx/ucx/wiki/Guidance-for-contributors

@yosefe
Copy link
Copy Markdown
Contributor

yosefe commented Apr 13, 2022

@shamisp , @rdietric is with NVIDIA.

@yosefe yosefe added the API label Apr 13, 2022
@shamisp
Copy link
Copy Markdown
Contributor

shamisp commented Apr 13, 2022

@shamisp , @rdietric is with NVIDIA.

I just noticed the email signature. Typically I check the GitHub account affiliation first, which is not set in this case.

@rdietric
Copy link
Copy Markdown
Author

rdietric commented Apr 13, 2022

I used my existing Github account and just added NVIDIA to my profile. Sorry for the inconveniences!

@shamisp
Copy link
Copy Markdown
Contributor

shamisp commented Apr 13, 2022

I used my existing Github account and just added NVIDIA to my profile. Sorry for the inconveniences!

@rdietric not a problem, you did better than many others ;)

@rdietric rdietric force-pushed the ucp_datatype/query_size_api branch from aef2c05 to 33ea8ac Compare April 13, 2022 14:21
brminich
brminich reviewed Apr 13, 2022
View reviewed changes
Comment thread src/ucp/api/ucp.h Outdated
Comment thread src/ucp/api/ucp.h Outdated
brminich
brminich reviewed Apr 20, 2022
View reviewed changes
Comment thread src/ucp/api/ucp.h
@brminich
Copy link
Copy Markdown
Contributor

brminich commented Apr 20, 2022

@yosefe, @shamisp can you please review?

shamisp
shamisp requested changes Apr 20, 2022
View reviewed changes
Comment thread src/ucp/api/ucp.h
@shamisp
Copy link
Copy Markdown
Contributor

shamisp commented Apr 20, 2022

@tonycurtis can you please review documentation.

tonycurtis
tonycurtis previously approved these changes Apr 20, 2022
View reviewed changes
@shamisp
Copy link
Copy Markdown
Contributor

shamisp commented Apr 21, 2022

Looks good

shamisp
shamisp previously approved these changes Apr 21, 2022
View reviewed changes
@rdietric rdietric mentioned this pull request Apr 22, 2022
Merged
@rdietric
Copy link
Copy Markdown
Author

rdietric commented Apr 22, 2022

I added PR #8150 with the implementation.

@rdietric
Copy link
Copy Markdown
Author

rdietric commented May 4, 2022

Is there anything else I can do to get the PR merged?

yosefe
yosefe reviewed May 9, 2022
View reviewed changes
Comment thread src/ucp/api/ucp.h Outdated
Comment thread src/ucp/api/ucp.h
Comment thread src/ucp/api/ucp.h
yosefe
yosefe reviewed May 9, 2022
View reviewed changes
Comment thread src/ucp/api/ucp.h Outdated
yosefe
yosefe previously approved these changes May 9, 2022
View reviewed changes
@yosefe
Copy link
Copy Markdown
Contributor

yosefe commented May 9, 2022

@shamisp @tonycurtis can you pls review as well?

@yosefe
Copy link
Copy Markdown
Contributor

yosefe commented May 9, 2022

@rdietric pls check the build failures

@rdietric
Copy link
Copy Markdown
Author

rdietric commented May 10, 2022

@rdietric pls check the build failures

I fixed the build failures, which were caused by a wrong @ref in a field comment.

@shamisp shamisp self-requested a review May 10, 2022 15:24
shamisp
shamisp previously approved these changes May 10, 2022
View reviewed changes
yosefe
yosefe previously approved these changes May 13, 2022
View reviewed changes
@yosefe
Copy link
Copy Markdown
Contributor

yosefe commented May 13, 2022

@tonycurtis can you pls take a look?

tonycurtis
tonycurtis previously approved these changes May 24, 2022
View reviewed changes
@yosefe
Copy link
Copy Markdown
Contributor

yosefe commented May 24, 2022

@rdietric pls squash

@rdietric rdietric dismissed stale reviews from tonycurtis, yosefe, and shamisp via 0386df2 May 24, 2022 22:36
@rdietric rdietric force-pushed the ucp_datatype/query_size_api branch from e868ac6 to 0386df2 Compare May 24, 2022 22:36
@rdietric
Copy link
Copy Markdown
Author

rdietric commented May 25, 2022

I squashed the commits. The failing tests seem unrelated to changes in this PR.

@yosefe yosefe merged commit 0b8b49e into openucx:master May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brminich brminich brminich left review comments

@shamisp shamisp shamisp left review comments

@yosefe yosefe yosefe left review comments

+1 more reviewer

@tonycurtis tonycurtis tonycurtis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rdietric @shamisp @yosefe @brminich @tonycurtis @rdietrich-nvidia