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

test_conv_embed_db (ctest -> gtest) #2168

Merged
merged 36 commits into from
Jul 16, 2023
Merged

test_conv_embed_db (ctest -> gtest) #2168

merged 36 commits into from
Jul 16, 2023

Conversation

alexandraBara
Copy link
Contributor

Moving part of the conv_2d test to gtest.
Test is automatically picked up by gtest.

test/gtest/conv_2d_implicit_gemm.cpp Outdated Show resolved Hide resolved
test/gtest/conv_2d.hpp Outdated Show resolved Hide resolved
test/gtest/conv_2d_implicit_gemm.cpp Outdated Show resolved Hide resolved
test/gtest/conv_2d_implicit_gemm.cpp Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
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.

@alexandraBara Sorry if I am too soon but I hopt you'll find this useful.

test/gtest/conv_embed_db.cpp Outdated Show resolved Hide resolved
test/gtest/conv_embed_db.cpp Outdated Show resolved Hide resolved
test/gtest/conv_embed_db.cpp Outdated Show resolved Hide resolved
test/gtest/conv_embed_db.cpp Outdated Show resolved Hide resolved
test/gtest/conv_embed_db.cpp Outdated Show resolved Hide resolved
@atamazov

This comment was marked as outdated.

@JehandadKhan JehandadKhan marked this pull request as ready for review June 1, 2023 17:49
@atamazov
Copy link
Contributor

atamazov commented Jun 7, 2023

@JehandadKhan Hmm... Do we really want to move test_conv_embed_db (which should be run only when we embed databases) under the gtest's jurisdiction?

@atamazov
Copy link
Contributor

atamazov commented Jun 7, 2023

@alexandraBara Please rename this PR. It moves test_conv_embed_db.

@atamazov
Copy link
Contributor

@xinlipn

CI is still running "Performance Tests - gfx90a", which should be a residual issue and has supposedly been disabled.

Maybe. We need a kind of approval from @junliume that this PR has passed CI tests.

@atamazov
Copy link
Contributor

@xinlipn BTW testing is not covered in the PR description. I means testing of the test itself, does it perform as expected.

Have you run this test with MIOPEN_LOG_LEVEL=5 and checked if the log matches your expectations?

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.

Some final review comments.

test/gtest/conv_embed_db.cpp Show resolved Hide resolved

std::string GetFloatArg()
{
static const auto tmp = miopen::GetEnv("MIOPEN_TEST_FLOAT_ARG");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reuse env.hpp from the library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

test/gtest/conv_embed_db.cpp Show resolved Hide resolved
#if MIOPEN_EMBED_DB

const auto& handle = get_handle();
if((handle.GetDeviceName() == "gfx900" || handle.GetDeviceName() == "gfx906") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Factor out device checks to a function.

Suggested change
if((handle.GetDeviceName() == "gfx900" || handle.GetDeviceName() == "gfx906") &&
if(IsTestSupportedForDevice(handle) &&

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

case miopenInt8x4:
case miopenInt32:
case miopenDouble:
MIOPEN_THROW(miopenStatusBadParm,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use MIOPEN_THROW in tests. This one is for library.

Copy link
Contributor

Choose a reason for hiding this comment

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

just fail the test

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

test/gtest/conv_embed_db.cpp Show resolved Hide resolved
#if MIOPEN_EMBED_DB

const auto& handle = get_handle();
const auto& envVar = miopen::GetEnv("MIOPEN_TEST_FLOAT_ARG");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use miopen::GetStringEnv() as it implements caching. Example:

MIOPEN_DECLARE_ENV_VAR(MIOPEN_TEST_FLOAT_ARG)
...
static std::string GetFloatArg()
{
    const char* p = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{});
    if(p == nullptr)
    {
        // Fail the test (preferred) or return "--float" by default.
    }
    return {p};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This matches the proven pattern that we use in MIOpen when we need to read a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

@atamazov
Copy link
Contributor

@xinlipn
Please note the following comments:

These were hidden as resolved which is not actually so. Due to that I had do duplicate some review comments. Let's hide review comments after they explicitly marked [Resolved], if possible.

Please also provide an answer to #2168 (comment). Thank you!

@xinlipn
Copy link
Contributor

xinlipn commented Jun 24, 2023

@xinlipn BTW testing is not covered in the PR description. I means testing of the test itself, does it perform as expected.

Have you run this test with MIOPEN_LOG_LEVEL=5 and checked if the log matches your expectations?

Will upload

@xinlipn BTW testing is not covered in the PR description. I means testing of the test itself, does it perform as expected.

Have you run this test with MIOPEN_LOG_LEVEL=5 and checked if the log matches your expectations?

@atamazov , CTest and gTest cover the same test cases, after having it reviewed by JD, there are a few issues, e.g. warning message that test_drive was called more than once. Have been working on them...

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.

Some refactoring is highly recommended.

Comment on lines 111 to 112
if(IsTestSupportedForDevice(handle) &&
(p_envVar != nullptr && std::strcmp(p_envVar, "--float") == 0))
Copy link
Contributor

@atamazov atamazov Jun 27, 2023

Choose a reason for hiding this comment

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

[R] This can be factored out to a function, like this:

Suggested change
if(IsTestSupportedForDevice(handle) &&
(p_envVar != nullptr && std::strcmp(p_envVar, "--float") == 0))
if(IsTestSupportedForDevice(handle) && IsTestRunWith("--float"))

And of course you do not need the following definition above

const char* const p_envVar = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{});

Proposed function:

static bool IsTestRunWith(const char* float_arg)
{
    assert(float_arg != nullptr);
    const char* const p = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{})
    return p != nullptr && std::strcmp(p, float_arg) == 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, on a side note, google test Macro ASSERT_NE(float_arg, nullptr) can only be used in a void function, otherwise there will be a compile error

"cannot initialize return object of type 'bool' with an rvalue of type 'void'"

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

const char* const p_envVar = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{});

if(IsTestSupportedForDevice(handle) &&
(p_envVar != nullptr && std::strcmp(p_envVar, "--half") == 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

[R] Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

const char* const p_envVar = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{});

if(IsTestSupportedForDevice(handle) &&
(p_envVar != nullptr && std::strcmp(p_envVar, "--int8") == 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

[R] Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

const char* const p_envVar = miopen::GetStringEnv(MIOPEN_TEST_FLOAT_ARG{});

if(IsTestSupportedForDevice(handle) &&
(p_envVar != nullptr && std::strcmp(p_envVar, "--bfloat16") == 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

[R] Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

[Resolved]

@atamazov
Copy link
Contributor

Almost done. Two things left:

@xinlipn
Copy link
Contributor

xinlipn commented Jul 5, 2023

@xinlipn BTW testing is not covered in the PR description. I means testing of the test itself, does it perform as expected.

Have you run this test with MIOPEN_LOG_LEVEL=5 and checked if the log matches your expectations?

@atamazov , there was some discrepancies in terms of initiated solvers for test coverage between develop branch and this branch, e.g. Forward convolution: ConvAsmImplicitGemmV4R1DynamicFwd_1x1 in Develop vs ConvHipImplicitGemmV4R1Fwd in current branch for
Input tensor: 128, 512, 7, 7
Weights tensor: 2048, 512, 1, 1
Output tensor: 128, 2048, 7, 7

Took some time to remove the discrepancies by updated cmake flags on Develop branch.

  • Log from Develop branch:

devleop_make_test_conv_embed_db_stdout.log

  • Log from current branch

branch_ctest.log

Please note MIOPEN_LOG_LEVEL=5 has been enabled, but it has affected the gtest serial spew. I have been working on this, which may need a separate ticket.

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

Successfully merging this pull request may close these issues.

6 participants