[UR][Tests] Add CTS options to set platforms and devices. #1085

szadam · 2023-11-16T09:23:22Z

Add CTS options to set test platform name or test platforms count and test device name or test devices count.
Create CMake options for running CTS with limited devices count and platforms count.

pbalcer · 2023-11-16T11:21:36Z

test/conformance/source/environment.cpp

+ // Get the argument (test_devices_count) to limit test devices count.
+ u_long count_set = 0;
+ if (argc >= 3) {
+ count_set = std::strtoul(argv[2], nullptr, 10);


is this always the second argument?

It depends on cts_exe.py scripts. In this case, it will always be the second argument.

Right, but the gtest binary may be executed manually, the script can change etc. You shouldn't be hard-coding such assumptions.

I've changed it. What about that?

jandres742 · 2023-11-16T23:12:13Z

test/conformance/README.md

+
+To limit how many devices you want to run the CTS on,
+use CMake option UR_TEST_DEVICES_COUNT. If you want to run


could we make it a runtime option instead of a CMAKE one? imagine the case where users have the binaries for the CTS, and have different types of systems (single CPU, 4xGPU, CPU + 2xGPU, etc). They would have to build the binaries for each configuration.

I think we could change it in the next step.

thanks @szadam . could you share why it couldn't be done as part of this PR? Main reason I'm asking is because I have already run into this problem, where I was running the tests in a multi-device system, and it took me a while to figure out the issue was that the tests were built only for 1 device, so I would expect more people to run into the same problem, so addressing sooner would be great.

if you run the tests directly through their binary, you can use the --test_devices_count 1 cli option that @szadam is adding. If you are using ctest, this patch will automatically reduce the number of devices it will run with to 1. You don't need to set the cmake option.

There's another patch that @szadam is working on, #992, that will solve the broader problem of devices/platforms having the same names.

I think what @jandres742 is proposing is implemented for OpenCL patch (here: https://github.com/oneapi-src/unified-runtime/pull/993/files#diff-d1f50b1ed696f085259b2a9c7d274ae3414700a1cea60f9945677cdae39e25cfR186) and it looks neat as well. The CMake option, though, has, I think, the added value (as Piotr mentioned) that it sets the default value of 1 and still can be overridden with the CLI param.

BTW, Adam has a new version (still on a private branch; to be polished): szadam@ef74282#diff-d1f50b1ed696f085259b2a9c7d274ae3414700a1cea60f9945677cdae39e25cf

thanks @pbalcer and @lukaszstolarczuk for the details.

Sorry for the late response. I am preparing a new solution/version as Lukas said. I will push it today.

pbalcer · 2023-11-21T10:23:15Z

test/conformance/source/environment.cpp

@@ -199,6 +201,22 @@ DevicesEnvironment::DevicesEnvironment(int argc, char **argv)
 error = "Could not find any devices associated with the platform";
 return;
 }
+ // Get the argument (test_devices_count) to limit test devices count.
+ u_long count_set = 0;
+ for (int i = 1; i < argc; ++i) {


Suggested change

for (int i = 1; i < argc; ++i) {

for (int i = 1; i < argc - 1; ++i) {

and drop the && i + 1 < argc in the if below?

You're right. It will work too.

jandres742 · 2023-11-26T05:53:27Z

+1

test/conformance/source/environment.cpp

lukaszstolarczuk · 2023-11-30T11:56:13Z

test/conformance/CMakeLists.txt

@@ -4,6 +4,8 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

 set(UR_CONFORMANCE_TEST_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+option(UR_TEST_DEVICES_COUNT "Count of devices on which CTS will be run" 1)
+option(UR_TEST_PLATFORMS_COUNT "Count of platform on which CTS will be run" 1)


platforms (plural)

lukaszstolarczuk · 2023-11-30T11:59:13Z

test/conformance/README.md

+all available devices/platforms, set 0. The default value is 1.
+If you run binaries for the tests, you can use the parameter
+`--platforms_count=COUNT/--devices_count=COUNT`.


perhaps split this code into two, like:

...you can use the parameter --platforms... or --devices....

lukaszstolarczuk · 2023-11-30T11:59:32Z

test/conformance/README.md

+`--platforms_count=COUNT/--devices_count=COUNT`.
+To set test device/platform name you want to run the CTS on, use
+parameter `--platform=NAME/--device=NAME`.


same here - split the code block into two

lukaszstolarczuk · 2023-11-30T12:00:48Z

test/conformance/README.md

+`--platforms_count=COUNT/--devices_count=COUNT`.
+To set test device/platform name you want to run the CTS on, use
+parameter `--platform=NAME/--device=NAME`.


as discussed, what if two platforms have the same name...? (e.g. "CUDA BACKEND" as the name of the platform 😉 ). Perhaps it's not something we want to solve in this PR, though.

For now, the tests are not adapted to be run on multiple platforms. They would require major modifications, which are not part of this PR. In the case where we have two platforms or devices with the same name, we should simply set their counter to 1, just set their counter to 1 for the tests to pass without any problem.

lukaszstolarczuk · 2023-11-30T12:01:15Z

test/conformance/cts_exe.py

@@ -20,9 +20,13 @@

 parser = ArgumentParser()
 parser.add_argument("--test_command", help="Ctest test case")
+ parser.add_argument("--test_devices_count", type=str, help="Number of devices on which tests will be run")
+ parser.add_argument("--test_platforms_count", type=str, help="Number of devices on which tests will be run")


copy-pasted description, pls adjust

fabiomestre · 2023-12-05T16:49:33Z

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

and test platforms count in CTS.

name and test platforms count or test platform name

szadam requested a review from a team as a code owner November 16, 2023 09:23

szadam assigned pbalcer Nov 16, 2023

pbalcer reviewed Nov 16, 2023

View reviewed changes

jandres742 reviewed Nov 16, 2023

View reviewed changes

szadam force-pushed the limit_devices_count branch 2 times, most recently from c85bf47 to 1db5146 Compare November 21, 2023 09:19

pbalcer approved these changes Nov 21, 2023

View reviewed changes

szadam force-pushed the limit_devices_count branch from 1db5146 to ad21ba8 Compare November 21, 2023 14:01

lukaszstolarczuk marked this pull request as draft November 21, 2023 15:55

szadam force-pushed the limit_devices_count branch from ad21ba8 to eaed4fb Compare November 28, 2023 20:45

szadam marked this pull request as ready for review November 28, 2023 20:45

szadam requested review from jandres742, pbalcer and lukaszstolarczuk November 28, 2023 20:46

szadam force-pushed the limit_devices_count branch 2 times, most recently from 7874c1c to 6a0f1d9 Compare November 29, 2023 00:01

szadam changed the title ~~[UR][Tests] Add a CMake option to limit the test devices count in CTS~~ [UR][Tests] Add CTS options to set platforms and devices. Nov 29, 2023

szadam force-pushed the limit_devices_count branch from 6a0f1d9 to 36d6fde Compare November 29, 2023 08:18

PatKamin reviewed Nov 29, 2023

View reviewed changes

test/conformance/source/environment.cpp Show resolved Hide resolved

pbalcer approved these changes Nov 29, 2023

View reviewed changes

lukaszstolarczuk reviewed Nov 30, 2023

View reviewed changes

szadam force-pushed the limit_devices_count branch from 36d6fde to b6d31fd Compare November 30, 2023 15:20

szadam requested review from lukaszstolarczuk, PatKamin and pbalcer December 4, 2023 12:03

pbalcer approved these changes Dec 4, 2023

View reviewed changes

PatKamin approved these changes Dec 4, 2023

View reviewed changes

fabiomestre changed the base branch from adapters to main December 5, 2023 16:49

szadam and others added 2 commits December 6, 2023 10:33

[UR][Tests] Add a CMake option to limit the test devices count

584314d

and test platforms count in CTS.

[UR][Tests] Add options in CTS to set test devices count or test device

371bd3c

name and test platforms count or test platform name

szadam force-pushed the limit_devices_count branch from b6d31fd to 371bd3c Compare December 6, 2023 09:34

pbalcer merged commit 861d95d into oneapi-src:main Dec 7, 2023
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UR][Tests] Add CTS options to set platforms and devices. #1085

[UR][Tests] Add CTS options to set platforms and devices. #1085

szadam commented Nov 16, 2023 •

edited

Loading

pbalcer Nov 16, 2023

szadam Nov 16, 2023

pbalcer Nov 16, 2023

szadam Nov 21, 2023

jandres742 Nov 16, 2023

szadam Nov 21, 2023

jandres742 Nov 21, 2023

pbalcer Nov 22, 2023

lukaszstolarczuk Nov 23, 2023

jandres742 Nov 26, 2023

szadam Nov 27, 2023

pbalcer Nov 21, 2023

szadam Nov 21, 2023

jandres742 commented Nov 26, 2023

lukaszstolarczuk Nov 30, 2023

szadam Nov 30, 2023

lukaszstolarczuk Nov 30, 2023

szadam Nov 30, 2023

lukaszstolarczuk Nov 30, 2023

szadam Nov 30, 2023

lukaszstolarczuk Nov 30, 2023

szadam Nov 30, 2023

lukaszstolarczuk Nov 30, 2023

szadam Nov 30, 2023

fabiomestre commented Dec 5, 2023


		To limit how many devices you want to run the CTS on,
		use CMake option UR_TEST_DEVICES_COUNT. If you want to run

	for (int i = 1; i < argc; ++i) {
	for (int i = 1; i < argc - 1; ++i) {

[UR][Tests] Add CTS options to set platforms and devices. #1085

[UR][Tests] Add CTS options to set platforms and devices. #1085

Conversation

szadam commented Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandres742 commented Nov 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiomestre commented Dec 5, 2023

szadam commented Nov 16, 2023 •

edited

Loading