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][gtest] Sl/test solver #1877

Merged
merged 9 commits into from
Nov 22, 2022
Merged

[Test][gtest] Sl/test solver #1877

merged 9 commits into from
Nov 22, 2022

Conversation

xinlipn
Copy link
Contributor

@xinlipn xinlipn commented Nov 14, 2022

ConvAsm3x3 solver test. Passed CI after disabling Navi21 which is not supported by this solver.

test/test.hpp Outdated
Comment on lines 96 to 98
// Conflicts with GoogleTest FAIL()
#ifdef FAIL
#undef FAIL
Copy link
Contributor

Choose a reason for hiding this comment

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

How it could be so that test/test.hpp is used in gtest?

Copy link
Contributor Author

@xinlipn xinlipn Nov 15, 2022

Choose a reason for hiding this comment

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

@atamazov test.hpp isn't used in GTest, however, there's a warning from compiler about FAIL() being already defined in GoogleTest. This change probably shouldn't go to develop

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can refactor these defs out into their own header, so that they are only included when they are needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we can refactor these defs out into their own header, so that they are only included when they are needed

@JehandadKhan separated Macros defined in test.hpp to a separate header test_macro.hpp and updated affected code.

@atamazov
Copy link
Contributor

This can't be merged in due to Navi21 issues. I recommend converting this into draft.

@atamazov
Copy link
Contributor

@junliume @JehandadKhan I highly recommend pausing with new GoogleTests until #1843 (comment) is resolved.

@atamazov
Copy link
Contributor

atamazov commented Nov 15, 2022

@xinlipn

ConvAsm3x3 solver test.

Please describe which aspects of the solver's behavior this test is intended to check. Thanks.

by @xinlipn:
@atamazov the intension is to bring test down to solver level for more granularity, i.e. each solver test will have its own gtest. Hope this makes sense.

@xinlipn
Copy link
Contributor Author

xinlipn commented Nov 15, 2022

This can't be merged in due to Navi21 issues. I recommend converting this into draft.

@atamazov ConvAsm3x3 doesn't support Navi21, only supports gfx8* and gfx9*.

@atamazov
Copy link
Contributor

atamazov commented Nov 16, 2022

@xinlipn That is why this test won't pass on Navi21. If this PR is merged in, CI will always fail. Which means that this PR, in its current state, shouldn't be merged.

@atamazov
Copy link
Contributor

@xinlipn

@atamazov the intension is to bring test down to solver level for more granularity, i.e. each solver test will have its own gtest. Hope this makes sense.

Yes, this makes sense. However, this does not answer my question.

@pfultz2
Copy link
Contributor

pfultz2 commented Nov 16, 2022

If we are writing tests for solvers then there should be unit tests for IsApplicable, GetSolution, and IsValidPerformanceConfig to get full branch coverage. For example, there is a lot of checking that is in IsApplicable, I would expect a unit test for each one of these branches. These unit tests should be very fast so it should easy to run through many configs to check all the branches.

if(!solv.IsApplicable(ctx))
{
test_skipped = true;
GTEST_FAIL() << solv.SolverDbId() << "ConvAsm3x3U Not Applicable for this problem"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
GTEST_FAIL() << solv.SolverDbId() << "ConvAsm3x3U Not Applicable for this problem"
GTEST_SKIP() << solv.SolverDbId() << "ConvAsm3x3U Not Applicable for this problem"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JehandadKhan replaced with GTEST_SKIP()

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. This change resolves #1877 (comment).

@JehandadKhan
Copy link
Collaborator

This can't be merged in due to Navi21 issues. I recommend converting this into draft.

That is a false issue, if the solver is not applicable on Navi, then the test should skip instead of failing, since there is nothing to test! Please see my suggestion here

@junliume @JehandadKhan I highly recommend pausing with new GoogleTests until #1843 (comment) is resolved.

I dont agree with that, since that would mean a new test which would enhance test coverage in MIOpen would be withering in our PR section. Since we wish to get to a place where we test each solver individually.

If we are writing tests for solvers then there should be unit tests for IsApplicable, GetSolution, and IsValidPerformanceConfig to get full branch coverage. For example, there is a lot of checking that is in IsApplicable, I would expect a unit test for each one of these branches. These unit tests should be very fast so it should easy to run through many configs to check all the branches.

I agree, however, that is not the intent of this PR. The purpose of this PR to increase numerical accuracy coverage to a new solver, which previously does not exists. The current testing infra uses the API to test numerical accuracy on the API level where individual solvers might get skipped since they were not fast enough. Once we have numerical coverage over all the solvers, we can start the work on unit testing each solver as you propose.

cc @xinlipn


#include "test.hpp"

#define CHECK(...) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is copied from test.hpp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why this is copied from test.hpp?

@atamazov this idea is to separate FAIL() out in a separate header which conflicts with the same definition in GoogleTest. It seems to be strange if just move FAIL() out, that's I put all Macros in the new header file

Copy link
Contributor

Choose a reason for hiding this comment

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

@xinlipn But then why test.hpp (that contains the same CHECK) is included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xinlipn But then why test.hpp (that contains the same CHECK) is included here?

@atamazov separate out only one Macro FAIL() from the rest and just create a header file for it seems to be a temp workaround and looks weird logically, at least to me.

FAIL() isn't used anywhere and I removed it for now to minimize the impact to other code.
Consequently test_macro.hpp is removed and changes in other headers anyway are reverted.

\
} while(false)

#define EXPECT(...) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see comments above

\
} while(false)

#define EXPECT_OP(LEFT, OP, RIGHT) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see comments above

@@ -39,7 +39,7 @@
#include "get_handle.hpp"
#include <vector>
#include <thread>
#include "test.hpp"
#include "test_macro.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to resolve the "FAIL" problem without touching existing tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atamazov another way is only separate FAIL in to a new header file or remove this definition for now if no code is currently using it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR can be simplified, if we just remove the FAIL macro if its not being used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR can be simplified, if we just remove the FAIL macro if its not being used anywhere

@JehandadKhan done
cc @atamazov

miopenDestroyConvolutionDescriptor(conv_desc);
}
ConvTestCase conv_config;
miopenConvolutionDescriptor_t conv_desc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use the MIOpen C++ objects instead, then you would not need to manage the lifetime of these objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use the MIOpen C++ objects instead, then you would not need to manage the lifetime of these objects.

@JehandadKhan , done

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.

No objections!

@junliume junliume changed the title Sl/test solver [Test][gtest] Sl/test solver Nov 22, 2022
@junliume junliume merged commit cbc1665 into develop Nov 22, 2022
@junliume junliume deleted the sl/test_solver branch March 23, 2023 20:41
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.

5 participants