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

GTEST/UCT: Add ability to filter by resource or md name #10205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tvegas1
Copy link
Contributor

@tvegas1 tvegas1 commented Oct 2, 2024

What

Add ability to limit gtest runs by MD and RSC name.

Why ?

Shorten test time on systems with 30+ interfaces.

How ?

Do it from gtest test instantiation, as UCX_NET_DEVICES is inoperent.

GTEST_UCT_MD_NAME=mlx5_0 GTEST_UCT_RSC_NAME=mlx5_0:1  ./test/gtest/gtest --gtest_filter=ib/test_md.reg*

iyastreb
iyastreb previously approved these changes Oct 3, 2024

std::vector<test_md_param> result;
for (std::vector<md_resource>::iterator iter = md_resources.begin();
iter != md_resources.end(); ++iter) {
if (iter->cmpt_attr.name == cmpt_name) {
if ((iter->cmpt_attr.name == cmpt_name) &&
((md_name == "") || (md_name == iter->rsc_desc.md_name))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: more C++ idiomatic way of checking empty string:
md_name.empty()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -461,6 +468,10 @@ std::vector<const resource*> uct_test::enum_resources(const std::string& tl_name
ucs_async_context_destroy(async);
}

if (all_resources.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (all_resources.size() == 0) {
if (all_resources.empty()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the filter only affects UCT tests, shall we filter UCP resources as well / or set UCX_NET_DEVICES?

@@ -461,6 +468,10 @@ std::vector<const resource*> uct_test::enum_resources(const std::string& tl_name
ucs_async_context_destroy(async);
}

if (all_resources.empty()) {
UCS_TEST_ABORT("Could not add any resource");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why needed? do we want to force failure when some resources not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after adding that filtering, I want to make sure we have at least one interface, to in usability as we cannot run test without any resource, and we get crash otherwise.

@tvegas1
Copy link
Contributor Author

tvegas1 commented Oct 7, 2024

Seems the filter only affects UCT tests, shall we filter UCP resources as well / or set UCX_NET_DEVICES?

The focus is for UCT for EFA, on UCP tests would less be instantiated per md/rsc, and we need to see lanes as they will be used. It's always possible to add UCX_NET_DEVICES if needed.

@tvegas1
Copy link
Contributor Author

tvegas1 commented Oct 14, 2024

/azp run UCX PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

4 participants