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

Adding base class for gtests #3321

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Adding base class for gtests #3321

wants to merge 5 commits into from

Conversation

reidkwja
Copy link
Collaborator

Addressing issue #3135 with a random seed generator + environmental variables saving.

@reidkwja reidkwja requested a review from CAHEK7 October 17, 2024 20:22
@reidkwja reidkwja marked this pull request as ready for review October 21, 2024 20:13
@reidkwja reidkwja requested review from iq136boy and cderb and removed request for JehandadKhan October 21, 2024 20:43
@Vsevolod1983
Copy link
Contributor

Btw, we are going to get rid of using environment variables #3329

Copy link
Contributor

@CAHEK7 CAHEK7 left a comment

Choose a reason for hiding this comment

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

Could you provide an example of how it can be used?
For example for test/gtest/smoke_solver_ConvBinWinograd3x3U.cpp or test/gtest/hipblaslt_gemm.cpp.

Copy link
Contributor

@CAHEK7 CAHEK7 left a comment

Choose a reason for hiding this comment

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

It seems it does not build. Probably something went wrong with proper gtest base class.
Also it would be nice to have less destructive changes, ideally with minimal changes in the tests.

// Set environment variables dynamically
for(const auto& [var, val] : GPU_Conv2dDefault_FP32::get_env_values())
{
miopen::env::setEnvironmentVariable(var, val);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid it doesn't work for cached env variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I have been trying different ways to integrate it without changing gtest_common.hpp - which will require changing large numbers of tests.

I am still looking into ways to do so with seamless integration into gtests...

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