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

[Testing] Convert MIOpenDriver tests into shell script #2878

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

xinlipn
Copy link
Contributor

@xinlipn xinlipn commented Apr 8, 2024

The shell script has dependency on the binary, MIOpenDriver

@xinlipn xinlipn self-assigned this Apr 8, 2024
@xinlipn xinlipn requested a review from CAHEK7 April 8, 2024 07:53
@xinlipn xinlipn marked this pull request as draft April 8, 2024 07:53
@xinlipn
Copy link
Contributor Author

xinlipn commented Apr 11, 2024

  1. Create a shell script for MIOpenDriver tests starting from the line below, which will be packed along with single gTest binary, miopen_gtest.

    if(${MIOPEN_TEST_WITH_MIOPENDRIVER})

  2. Shell script depends on the binary, /MIOpen/build/bin/MIOpenDriver. Feel free to advise if hardcoding the path is not preferred

    MIOpenDriver_path="/MIOpen/build/bin/MIOpenDriver" # Change this to the actual path

  3. These two env vars, MIOPEN_TEST_WITH_MIOPENDRIVER and MIOPEN_TEST_ALL, are used to control miopen tests

  4. Also feel free to advise if there's a more appropriate directory to put the shell script instead of /MIOpen/

  5. Test logs attached. Tested on 10.7.47.119 (MI200), with rocm/miopen:ci_fd8d66
    miopendriver_test.log

@xinlipn xinlipn marked this pull request as ready for review April 11, 2024 07:42
@xinlipn xinlipn changed the title [DRAFT] Convert MIOpenDriver tests into shell script [Testing] Convert MIOpenDriver tests into shell script Apr 11, 2024
@iq136boy iq136boy self-requested a review April 11, 2024 17:06
@junliume
Copy link
Collaborator

Adding @atamazov , this looks like adding a functionality which will eventually replace/convert an existing one, I would suggest moving forward if no objections.

Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

Unfortunately does not comply #1843 (2) and other things. I recommend implementing these tests just like we do other tests (but set normal environment and then exec MIOpenDriver with necessary command-line arguments instead of instantiating and using test classes from the ./test directory).

@xinlipn
Copy link
Contributor Author

xinlipn commented Apr 15, 2024

Unfortunately does not comply #1843 (2) and other things. I recommend implementing these tests just like we do other tests (but set normal environment and then exec MIOpenDriver with necessary command-line arguments instead of instantiating and using test classes from the ./test directory).

Hi @atamazov, thanks for the input. Unlike converting other cTests to gTest by implementing test classes and pack them into a single binary as this PR below, what the script does is to call MIOpenDriver binary with arguments like what you commented.

#2806

The shell script has dependency on MIOpenDriver binary and also needs to have environment variables set. The reason is

  1. Previous MIOpenDriver test didn't use the test drivers, e.g. conv2d_driver, a shell script seems to be a feasible solution at this stage after discussion. This script along with the single gTest binary miopen_gtest will be delivered and pack to other internal teams.

  2. Although this shells script isn't part of cTest but it meant to have the same coverage as cTest. From this perspective, it's not meant to be standalone, so I kept the same logic and environment variable i.e. MIOPEN_TEST_ALL

Thanks

@atamazov
Copy link
Contributor

@xinlipn

Unlike converting other cTests to gTest by implementing test classes and pack them into a single binary as this PR below, what the script does is to call MIOpenDriver binary with arguments like what you commented.

What I am proposing is implementing the minimum test classes and calling MIOpenDriver from within the test, just like the script does. The advantage is that the test can work standalone -- it can check the GPU type, XNACK feature etc. But of course is will require the driver to exist, i.e. there is a dependence.

@xinlipn
Copy link
Contributor Author

xinlipn commented Apr 16, 2024

@xinlipn

Unlike converting other cTests to gTest by implementing test classes and pack them into a single binary as this PR below, what the script does is to call MIOpenDriver binary with arguments like what you commented.

What I am proposing is implementing the minimum test classes and calling MIOpenDriver from within the test, just like the script does. The advantage is that the test can work standalone -- it can check the GPU type, XNACK feature etc. But of course is will require the driver to exist, i.e. there is a dependence.

Thanks for the comment. This script can indeed detect/check GPU type at runtime as below we can check other features too if they are relevant to MIOpenDriver tests.

function gpu_detection()

This design offers a quick solution and hopefully same logic/coveratge as current MIOpenDriver tests to be part of the test package along with the stand alone gTest binary, miopen_gtest. We can do it with a class as proposed in another implementation/PR.

FYI, this script does run by itself (given the presence of MIOpenDriver) if this makes sense. Thanks

@xinlipn
Copy link
Contributor Author

xinlipn commented Apr 24, 2024

@xinlipn

Unlike converting other cTests to gTest by implementing test classes and pack them into a single binary as this PR below, what the script does is to call MIOpenDriver binary with arguments like what you commented.

What I am proposing is implementing the minimum test classes and calling MIOpenDriver from within the test, just like the script does. The advantage is that the test can work standalone -- it can check the GPU type, XNACK feature etc. But of course is will require the driver to exist, i.e. there is a dependence.

Creating a gTest to run MIOpenDriver is probably a better way than shell script. Will close this PR and create a new one for new implementation.

@CAHEK7
Copy link
Contributor

CAHEK7 commented Jun 10, 2024

I guess it can be closed, since #2984 has been merged.

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

Successfully merging this pull request may close these issues.

4 participants