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

[tests] Register backend-parameterized tests inside lib/Backends #3476

Closed
wants to merge 1 commit into from

Conversation

@bertmaher
Copy link
Contributor

commented Aug 30, 2019

Summary: This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code. Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,

INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);

Obviously the provided test NewTest is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:

  • There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
  • It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file. I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
  • I don't love making the include path start with "tests/", maybe there's some way around this.

Test Plan: eventually, run all the tests.

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@bertmaher

It seems to me that value-parameterized abstract tests could be a better solution. I think this way the definitions of actual tests can still be in *.cc files inside a common tests library and each backend would just need to use INSTANTIATE_TEST_SUITE_P for instantiating them under lib/Backends/*. WDYT?

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@bertmaher

It seems to me that value-parameterized abstract tests could be a better solution. I think this way the definitions of actual tests can still be in *.cc files inside a common tests library and each backend would just need to use INSTANTIATE_TEST_SUITE_P for instantiating them under lib/Backends/*. WDYT?

I don't see how that actually works here. The problem is that INSTANTIATE_TEST_SUITE_P instantiates an entire suite, not an individual test, and we want to be able to instantiate them on a per-test granularity. This PR effectively makes each test its own suite using the token-pasting macro magic in GLOW_BACKEND_TEST, but this requires each test definition (not just the fixture) to be available in lib/Backends.

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

instantiates an entire suite, not an individual test, and we want to be able to instantiate them on a per-test granularity.

Oh, yes. You are right! We want to have a per-test granularity.

@bertmaher bertmaher requested review from opti-mix and nickgg Aug 30, 2019

@jackm321

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Test Plan: eventually, run all the tests.

If someone creates a new test immediately after we run all the tests, do they need to go instantiate the test for all backends? How will the good people at MyStealthModeStartup know that a new test has been added and they should instantiate the it in their backend also?

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Test Plan: eventually, run all the tests.

If someone creates a new test immediately after we run all the tests, do they need to go instantiate the test for all backends? How will the good people at MyStealthModeStartup know that a new test has been added and they should instantiate the it in their backend also?

Yeah, every time a new test is added, it will need to be added to all backends. That's not totally different from today, in which someone needs to go update their local copy of OperatorTest to add to the new tests' ENABLED_BACKENDS string, though.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

And, one should always be able to diff the InterpreterOperatorTest against the StealthyOperatorTest to get an at-a-glance list of the supported tests gap.

@jfix71

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

So this seems to work pretty nicely! But just to suggest another idea/alternative which I think is a bit closer to what we currently have -- inside the test you can query for the name of the test/suite, e.g.:

auto *testInfo = ::testing::UnitTest::GetInstance()->current_test_info();
auto testName = testInfo->name();  // e.g. "replaceNaN_Float/0"
auto testName = testInfo->test_case_name();  // e.g. "OperatorTest/OperatorTest"

We could pass these strings to a function in the backend (or if we don't want it inside the backend we could come up w/ something else 🙂) which returns whether that test is supported, and if not then bail from the test early like we currently do with ENABLED_BACKENDS();.

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@bertmaher I like your solution but I'm not happy about the fact that the whole test needs to be defined in the include file. I still think that using abstract parameterized tests, defining only the fixture class in the header and the actual tests in the cpp file is a better solution.

The problem is that INSTANTIATE_TEST_SUITE_P instantiates an entire suite, not an individual test, and we want to be able to instantiate them on a per-test granularity.

That's true. BTW, I actually meant INSTANTIATE_TEST_CASE_P rather than INSTANTIATE_TEST_SUITE_P.

Also, it turns out that gtest allows for enabling/disabling any specific tests programmatically! For example:

// Run a specific test only
testing::GTEST_FLAG(filter) = "MyLibrary.TestReading"; // I'm testing a new feature, run something quickly.

// Exclude a specific test
testing::GTEST_FLAG(filter) = "-MyLibrary.TestWriting"; // The writing test is broken, so skip it.

It seems like this way it is pretty easy to control which specific tests from a suite should or should not run. Would it help to solve the issue?

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

@bertmaher I actually tried out locally this solution for our quantization tests. Moved all the fixture classes into a header and then included this header into QuantizationTest.cpp. I also removed all test instantiations from QuantizationTest.cpp. Then I actually created two other cpp files. Each of them included QuantizationTest.h and then just defined different instantiations:

namespace glow {
INSTANTIATE_TEST_CASE_P(CPU, Quantization, ::testing::Values("CPU"));
}

and

namespace glow {

INSTANTIATE_TEST_CASE_P(
    InterpAndCPUProfAndQuant, InterpAndCPU,
    ::testing::Combine(::testing::Values("Interpreter", "CPU"),
                       ::testing::Values("Interpreter", "CPU")));
}

Once linked with the implementation QuantizationTest.cpp file, everything seems to work just fine and run the expected quantization tests.

So, seems like this way we can easily define on a per test basis what we need to run with a given backend.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

I don't understand, are you saying the abstract value parameterized test approach works after all? I... don't really believe that, unless you've a-priori separated your tests into different fixtures that you can instantiate.
(The whole test-case test-suite nomenclature makes this discussion really annoying, too, btw -- old googletest, which we use, calls groups of tests "cases". They finally fixed this naming scheme to call groups "suites". Hence the ambiguity between INSTANTIATE_TEST_CASE_P and INSTANTIATE_TEST_SUITE_P. They're the same thing for different gtest versions :-( ).

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

I'll have to think about the GFLAG solution and/or the test name solution. Those are both interesting possibilities for this...

One more thing on the abstract value approach. I'm actually thinking defining the tests in a header could be preferable to defining them in a .cpp file -- it's one less file to hop through while trying to make sense of things. I guess there's a pretty concrete downside though, which is you now have to compile the blob of tests N times. Depends how quickly our tests compile :) (And maybe do extra linking depending on if you're using shared libs.)

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

One more thing that may be useful: gtest supports GTEST_SKIP which can be used like this:

#include "gtest/gtest.h"

using ::testing::Test;

TEST(SkipTest, DoesSkip) {
  GTEST_SKIP();
  EXPECT_EQ(0, 1);
}

class Fixture : public Test {
 protected:
  void SetUp() override {
    GTEST_SKIP() << "skipping all tests for this fixture";
  }
};

TEST_F(Fixture, SkipsOneTest) {
  EXPECT_EQ(5, 7);
}

TEST_F(Fixture, SkipsAnotherTest) {
  EXPECT_EQ(99, 100);
}

Maybe it could be used to conditionally skip certain tests based on backend-specific conditions.

@jfix71

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@opti-mix I believe GTEST_SKIP() isn't available in an official gtest release yet -- we discussed using it with ENABLED_BACKENDS but didn't for this reason, and so we just early exit and report the test passed. So I think the only benefit of using it is that it reports SKIPPED when you run it instead of PASSED.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

I ported OperatorTest to use this method but I quickly realized a likely fatal problem - since tests have to be explicitly enabled for each backend it would be way too easy to create a new test and forget to enable it anywhere, and nothing would help you discover that fact.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

I do have an idea though - I think it’s pretty similar to what @jfix suggested, which is to instantiate the whole suite in the backend, but call a static method that blacklists tests by name if necessary.

@bertmaher bertmaher force-pushed the bertmaher:tests branch from 55965ea to a36d1cd Sep 5, 2019

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Okay! Here's something I'm a bit happier with. It combines some of the ideas from @opti-mix (value-parameterized abstract tests) and @jfix71 (backend test selection based on names).

The idea is that OperatorTest will create a library of parameterized tests, then FooOperatorTest will instantiate those tests with INSTANTIATE_TEST_CASE_P for backend Foo. Each test will (by convention) first call a function to check if the test is supported on that backend; that function can do arbitrary things, but in practice you'd provide a blacklist or whitelist of test names.

One really cool thing about this, you can run the whole test suite by just making that function return true;, which gives a quick check of how compliant your backend is.

There's a wee bit of magic required to support this approach, which is that the isTestEnabled function is actually a function pointer that's filled in by a static initializer invoked by the appropriate backend-specific test. The reason for this sleight of hand is to allow the enablement check to be "hidden" in the backend-specific code, while still building OperatorTest as a library independent from the backend-specific code.

@jfix71
Copy link
Contributor

left a comment

Looks pretty good!

As far as allowing for listing either a blacklist or whitelist inside IS_TEST_ENABLED() -- this flexibility seems nice, but it may also make things a bit error prone, because when you add a new test you'd need to consider each backend and if it needs to be blacklisted vs. whitelisted. Would it be a good idea to force one way or the other, maybe by changing IS_TEST_ENABLED() to be something like GET_BLACKLIST() to return a reference to a blacklist set to check against?

using namespace glow;

IS_TEST_ENABLED(std::string name) {
std::set<std::string> blacklist = {

This comment has been minimized.

Copy link
@jfix71

jfix71 Sep 5, 2019

Contributor

nit: static

lib/Backends/CPU/tests/CPUOperatorTest.cpp Outdated Show resolved Hide resolved
glow::isTestEnabled = &Enabler::isTestEnabled; \
return true; \
} \
static bool isTestEnabled(std::string name); \

This comment has been minimized.

Copy link
@jfix71

jfix71 Sep 5, 2019

Contributor

nit: StringRef or const ref.

@opti-mix
Copy link
Contributor

left a comment

@bertmaher Your current solution looks much better! But I'm pretty sure it could be improved even further. See my comments below.

IS_TEST_ENABLED(std::string name) {
std::set<std::string> blacklist = {
"less_float16Cases/0",
"less_int64Cases/0",

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

Why do we need to keep this list as code? May be it should be in some kind of a config file?

Formulating this as code has some advantages, because this way it is closer to the actual code that uses it. But I'm not quite convinced if it is really so important. On the negative side, you have to touch the code and rebuild tests if you want to change something. Using a config file would also allow for using the same file for defining whitelists/blacklists for multiple test suites at once, which could be convenient.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 5, 2019

Author Contributor

I don't think there are enough advantages to a config file to make it worthwhile.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 6, 2019

Author Contributor

Actually, I might relent on the config file front. I'm starting to worry a bit about the amount of "ceremony" involved in creating all of these backend-specialized tests, and I think it might be good to drive things from a config instead of a bunch of code. I think I might be able to address your point that it's easy for backends to "overlook" a test suite, too, using a bit of build system sorcery.

I'll try it out tonight and see if it pans out...

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

I think I might be able to address your point that it's easy for backends to "overlook" a test suite, too, using a bit of build system sorcery.

Hopefully this will work with both CMake and buck.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 6, 2019

Author Contributor

Yeah, CMake is the big concern for me, I'm pretty confident with Buck (although there are some definite gotchas there too).

"less_float16Cases/0",
"less_int64Cases/0",
"ResizeNearest_Float/0",
"ResizeNearest_Float16/0",

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

It would be nice to use gtest's filter syntax:

  1. Users of gtest know it already
  2. This syntax allows for usage of (a limited form of) regular expressions, which may make the lists like this much shorter. Or do you intentionally want to use fully specified test names here?

And BTW, if we'd use gtest's filter syntax, may be we could just use the testing::GTEST_FLAG(filter) = backend_tests_filter_string; trick? We would just need to form the backend_tests_filter_string properly. This could solve multiple problems:

  • You could use gtest filter syntax
  • gtest would do the parsing of those regular expressions in the filters (even if we don't go with using the filter flag, it would be still nice to reuse gtest's parsing functionality for filters if possible)
  • gtest would skip execution of corresponding tests
  • gtest would report skipped tests as skipped, not as passed
  • we could have a flag to ignore the backend-specific filters and try to execute all tests to see if there are new tests that are failing/passing now.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 5, 2019

Author Contributor

I'm not crazy about using GTEST_FLAG(filter) internally to the test. In particular I don't want to figure out how to merge a user-specified --gtest_filter with the internal filter. It's probably not so bad, but I'm not convinced any of the advantages are worth the added complexity. I don't see any exposed API for filter parsing, either.

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

In particular I don't want to figure out how to merge a user-specified --gtest_filter with the internal filter.

Yeah, I see your point. But I'm not sure they have to interact at all. Typically, a user uses --gtest_filter to run e.g. a specific test, which means the user knows what she does. I'd say user-provided --gtest_filter should just completely override everything else, i.e. GTEST_FLAG(filter) should just be ignored in this case.

BTW, Chromium seems to do some crazy things in this direction ;-) They have their own way to parse those filters and eventually merge them with user-provided ones... I don't think we need to go that far.

return blacklist.count(name) == 0;
}

INSTANTIATE_TEST_CASE_P(CPU, OperatorStatelessTest, ::testing::Values("CPU"));

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

This assumes that a backend needs to be aware of all the test suites (not specific tests) so that it can instantiate them. What happens if a new test suite is added in glow/master? This backend wouldn't know, right?

  • May be some kind of a dynamic discovery mechanism could be used to find all test suites and instantiate them dynamically?
  • Or e.g. the library containing those test suites should somehow require for each test suite certain symbols to be defined at the linking stage. This way each backend will have to define them (e.g. using INSTANTIATE_TEST_CASE_P), because otherwise the linker would complain and make them aware that they miss a new test suite now.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 5, 2019

Author Contributor

Yeah, this is a valid complaint, but as far as I can tell, the ability to dynamically/programmatically register tests is not available in the version of gtest we're using.

Honestly, the number of test suites is not that huge and we already have this problem in that you can forget to add YourBackend to the appropriate INSTANTIATE_TEST_CASE_P.

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

Do you think the static approach (second bullet) would work? E.g. a test suite X defined in a library of tests requires TestSuiteX::static_something to be defined somewhere outside of the library. This way each backend who links against it would need to define it, thus it will be aware of X's existence and can add INSTANTIATE_TEST_CASE_P, etc.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 5, 2019

Author Contributor

I like the principle of the static symbol approach, but I don't quite see how it prevents someone from just not creating a test suite for their backend (since the instantiation of the test suite happens in the backend subdirectory, if they just never create it there's nothing to throw an error).

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

I like the principle of the static symbol approach, but I don't quite see how it prevents someone from just not creating a test suite for their backend (since the instantiation of the test suite happens in the backend subdirectory, if they just never create it there's nothing to throw an error).

I was thinking about something like this:

  • Let's say we use a distinct fixture class for each test suite. And such a fixture class defined in the backend-independent library declares a static member of this fixture, but does not define it in any cpp files. Inside the cpp file with tests, we make sure that this static member is referenced so that the linker would always search for it when this library is used (e.g. reference it from a global variable initialization or something like this)

  • Now any backend inside the backends subdirectory that links against this library, but does not define this static member, would get a linking error, because reference to this static member inside the library cannot be resolved. Once this error pops up (e.g. when a new fixture was added in the library), the backend developer immediately becomes aware of this newly introduced fixture (i.e. about the newly introduced test suite inside the library). Now the required static member can be defined in the backend specific cpp file and INSTANTIATE_TEST_CASE_P for this fixture can be added.

Seems like it should work and help backend developers to detect new test suites.

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

I think I might be able to address your point that it's easy for backends to "overlook" a test suite, too, using a bit of build system sorcery.

Here is a a simpler idea than any build system or linker tricks.

  • Let's say we use something like this header file to get a possibility to use the MAP macro over a list of arguments.
  • In the test library header file, e.g. MyTestsLibraryHeader.h, where the test suites/fixtures are defined, we just need to define a macro that lists ALL test suites in this library, e.g. like this
/// List of all test suites defined in the library.
#define TESTSUITE_NAMES TestSuite1, TestSuite2, TestSuite3, TestSuite4
  • We also define in a common utility header file, e.g. InstantiateTestSuites.h, the following macros:
/// A macro to instantiate a test-suite with a given name using a specific backend.
#define INSTANTIATE_TESTSUITE(NAME) INSTANTIATE_TEST_CASE_P(BACKEND_NAME, NAME, ::testing::Values(STR(BACKEND_NAME)));
/// Stringify the provided macro operand.
#define STR(X) #X

/// Instantiate all test suites from the TESTSUITE_NAMES list using the INSTANTIATE_TESTSUITE macro.
MAP(INSTANTIATE_TESTSUITE, TESTSUITE_NAMES)
  • Now in the backend-specific cpp file in the backends directory we just need to include the headers mentioned above and then do:
#define BACKEND_NAME MyCustomBackend
#include "MyTestsLibraryHeader.h"
#include "InstantiateTestSuites.h"
  • Now we have all test suites instantiated in the backend-specific cpp gtest file! No build system or linker tricks are necessary. Just a bit of a preprocessor magic! :-)

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 6, 2019

Author Contributor

Idk man, the build system trick is about 7 lines of CMake :-). But this is pretty clever, let's see how it compares to what I've got now...

@@ -100,7 +89,7 @@ lessHelper(glow::PlaceholderBindings &bindings, glow::Module &mod,
}

TEST_P(OperatorTest, less_int8) {
ENABLED_BACKENDS(Interpreter, CPU);
CHECK_IF_ENABLED();

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 5, 2019

Contributor

Your solution uses CHECK_IF_ENABLED at the beginning of each test, which is OK, but a bit intrusive. But may be it is possible to use gtest event listeners for this purpose? IMHO, it could be a less intrusive and cleaner solution.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 5, 2019

Author Contributor

I don't see a way to use an event listener for this purpose -- maybe it would work if SKIP were available (but it's not), but as it is, there's no way for the test setup to force the test to pass, only to fail.

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

Yeah, it is a pity one cannot do SKIP in listeners.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Would it be a good idea to force one way or the other, maybe by changing IS_TEST_ENABLED() to be something like GET_BLACKLIST()

I'm open to being convinced on this; one of my motivations for leaving it up to the code is that for bringup a whitelist would be really useful (like, in the early days of Habana we had, like, 4 operators working :) ).

@facebook-github-bot
Copy link

left a comment

@bertmaher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I'll be honest, the amount of cleverness that is going into this feels totally disproportionate to the goal, which seems relatively straightforward :-/. I don't know if that's a sign that something's badly off the rails, or what.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Okay, I'm incrementally happier with this version. There's a little CMake magic to build a list of enabled backends and instantiate an OperatorTest variant for each, but there's pretty much no "other" magic here.

What I like about this version:

  • You don't need a whole bunch of supporting files (extra headers for each test, CMakeLists in each backend/tests dir, etc.). You just add one .cpp file to each backend containing a blacklist.
  • Tests are automatically created for each enabled backend without any thought
@opti-mix
Copy link
Contributor

left a comment

Your latest version that builds test for all backends looks really great! It definitely solves the issue of building all test suites for all backends and does it in a pretty straight-forward way.

The only remaining things are:

TestMain)
add_glow_test(OperatorTest ${GLOW_BINARY_DIR}/tests/OperatorTest --gtest_output=xml:OperatorTest.xml)
LIST(APPEND UNOPT_TESTS ./tests/OperatorTest -optimize-ir=false &&)
foreach(backend ${GLOW_BACKENDS})

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

This should probably become a CMake macro BUILD_TEST_FOR_ALL_BACKENDS that can be used for different tests.


/// Helper macro to check the current test against the blacklist.
#define CHECK_IF_ENABLED() \
if (backendTestBlacklist.count( \

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

Should we have whitelist and blacklist? Depending on the number of elements in each, it could be easier to define one or the other. The check would look like this:

/// Bail if there is a non-empty white list and the current test is not in it.
if (!backendTestWhitelist.empty() && !backendTestWhitelist.count(...)) {
  return;
}
/// Bail if the current test is in the blacklist.
if (backendTestBlacklist.count(...)) {
   return;
}

BTW, we have a similar logic in the partitioner.

This comment has been minimized.

Copy link
@jfix71

jfix71 Sep 6, 2019

Contributor

I wasn't sure about this -- if backends define their support differently then it makes things a bit more error prone. E.g. if I add a new test that I want to enable on the CPU and Habana backends, and I initially test it just on the CPU backend (which may use a blacklist and so it's enabled by default), and then once it's working I forget that I need to explicitly enable it on Habana which use a whitelist.

I'm also wondering about the potential for mistakes if we e.g. misspell names in the whitelist. Then it would appear the test passes even if it's not enabled, as we don't have GTEST_SKIP() yet.

@@ -0,0 +1,11 @@
set(GLOW_BACKENDS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/lib/Backends")

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 6, 2019

Contributor

We have GlowExternalBackends.cmake already. Those ones should be taken into account as well.

I feel like we need to build GLOW_BACKENDS variable that includes both in-tree and external backends. May be this logic should even live in the same cmake file.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 6, 2019

Author Contributor

Hm... I kind of agree, but externalbackends/ has its own layout and logic. I think this work is probably orthogonal (although perhaps if everything works out we can deprecate externalbackends and just use lib/Backends)

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Is there a big advantage to using config files instead of string set in code? There are a few non-trivial downsides:

  • complicates the build and test system; buck and cmake both know natively how to compile and link in a cpp file, they need to be taught that config files exist, export them as targets, and how/where to read them from in the test.
  • it adds code to the test to read and parse the yaml (or whatever) into a set that we can check, when we could just have written the set.

The main advantage I can see is avoiding a need to recompile to change the test set, but changing that set is probably a development-time thing anyways.

I guess I feel a bit similar to regex matching. I built the existing blacklist with a bash one-liner:

for i in $(./tests/OpenCLOperatorTest --gtest_list_tests  | cut -d' ' -f3); do if ! ./tests/OpenCLOperatorTest --gtest_filter="*.$i" >&/dev/null; then echo $i; fi; done | tee /tmp/blacklist

Building regexes for this would actually be harder :-). I'd rather not add code to do pattern matching unless people are going to really use it (or be really impeded by not having it).

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@bertmaher I kind of agree with your reasoning on config files, regexes, whitelists and external backends.

Let's start with the current simple solution first and see how backend developers react. We can always add any of the mentioned features if there is a real demand for them.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@opti-mix Sounds great to me, definitely happy to expand the feature set if people start to demand them :-)

@facebook-github-bot
Copy link

left a comment

@bertmaher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@bertmaher bertmaher changed the title [RFC][Do not commit] Register backend-parameterized tests inside lib/Backends [tests] Register backend-parameterized tests inside lib/Backends Sep 7, 2019

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

Now that the approach seems more-or-less settled I've ported everything but quantization tests (those are a bit weird since they can pick two backends to compare). Sorry the diff is a bit unwieldy; most of the changes are mechanical, and all of the interesting stuff is in unittests/CMakeLists and BackendTestUtils.

@jfix71
jfix71 approved these changes Sep 10, 2019
Copy link
Contributor

left a comment

LGTM, want to unblock here to avoid conflicts.

Couple last questions. First, I believe there were/still are some tests with out an ENABLED_BACKENDS / CHECK_IF_ENABLED. If they don't have them then it will run for all backends. Do we want to enforce that it should be added to all tests? No strong opinion here.

Second, I also noticed one test which (I believe) was accidentally blacklisted when it used to run for all -- curious how you determined the blacklists, was it programmatically? I didn't go through and check that they're all the same from before 🙂

"RecSys_Partitioned_RWQuantizedFP16AccumFP16_SLWS/0",
"RecSys_Partitioned_RWQuantizedFP16_SLWS_FP16/0",
"RecSys_Partitioned_RWQuantizedFP16AccumFP16_SLWS_FP16/0",
"RecSys_SLS_Only/0",

This comment has been minimized.

Copy link
@jfix71

jfix71 Sep 10, 2019

Contributor

I think this one was enabled before, no?

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 10, 2019

Author Contributor

Let me check -- I generated most of the blacklists programmatically using a bash script basically like

for test in $(XyzTest --gtest_list_tests); do
  if ! XyzTest --gtest_filter="$test"; then
    echo $test
  fi
done

But for RecSys I did some manual editing because it's so long running, and I probably just made a mistake.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 10, 2019

Author Contributor

I wouldn't mind adding CHECK_IF_ENABLED on everything, either, but I don't have a strong opinion. I've added it in a lot of places but probably not everywhere (like, OperatorTest I think misses some cases that just happened to pass on all backends).

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 10, 2019

Author Contributor

I was also contemplating if CHECK_IF_ENABLED could be rolled into a special macro, like TEST_GLOW_BACKEND(suite, test), and I think it can be done but since this diff is already a bit thorny I didn't want to push on it too much.

bertmaher added a commit to bertmaher/glow that referenced this pull request Sep 10, 2019
Register backend-parameterized tests inside lib/Backends (pytorch#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: pytorch#3476

Test Plan: eventually, run all the tests.

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: c63a67ddc27ff0840e4a661ec26e2a325734db2a

@bertmaher bertmaher force-pushed the bertmaher:tests branch from 17de9a0 to c95c210 Sep 10, 2019

bertmaher added a commit to bertmaher/glow that referenced this pull request Sep 10, 2019
Register backend-parameterized tests inside lib/Backends (pytorch#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: pytorch#3476

Test Plan: eventually, run all the tests.

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: a41120d9bceb7ec998f5d32519a1279f9ea0d028

@bertmaher bertmaher force-pushed the bertmaher:tests branch from c95c210 to bf3c4ff Sep 10, 2019

@opti-mix
Copy link
Contributor

left a comment

LGTM, modulo some minor comments.

And you should add to the docs a description of what needs to be done by backends to enable/disable certain tests. We should describe what the test authors need to do (e.g. how they define new tests or test suites, how they package them into a library, how they define them in CMake, etc) and what backends need to do (what they need to include, what they need to do in their CMake files and how they can easily build blacklists using some shell-commands you mentioned in this PR thread). Basically, the docs should describe the new way to define and use tests.

/// Blacklist of tests for the current backend under test.
extern std::set<std::string> backendTestBlacklist;

#define STR(X) #X

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

Could you add doxygen comments to these macros?

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

BTW, it feels like the name STR could potentially conflict with something from other libraries. Maybe we should give it a less common name.

extern std::set<std::string> backendTestBlacklist;

#define STR(X) #X
#define IBT(B, T) INSTANTIATE_TEST_CASE_P(B, T, ::testing::Values(STR(B)));

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

IBT is not a very descriptive name ;-)

TestMain)
add_glow_test(OperatorTest ${GLOW_BINARY_DIR}/tests/OperatorTest --gtest_output=xml:OperatorTest.xml)
LIST(APPEND UNOPT_TESTS ./tests/OperatorTest -optimize-ir=false &&)
function(add_backend_test)

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

Some comments explaining the overall logic of this function would be useful.

@@ -0,0 +1,11 @@
set(GLOW_BACKENDS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/lib/Backends")
file(GLOB subdirs RELATIVE "${GLOW_BACKENDS_DIR}" "${GLOW_BACKENDS_DIR}/*")
foreach(object ${subdirs})

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

Some comments explaining the overall logic of this loop would be useful.

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Good call on the docs @opti-mix, I've added a blurb to our Backends document. I've also made some small changes to lower the friction: specifically, if a backend doesn't have a tests/$Backend$Test.cpp file, then we won't create a $Test target for that backend. This way people can only define the tests they care about, without getting a big pile of mysterious linker errors right off the bat.

bertmaher added a commit to bertmaher/glow that referenced this pull request Sep 10, 2019
Register backend-parameterized tests inside lib/Backends (pytorch#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: pytorch#3476

Test Plan: eventually, run all the tests.

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: 869f95a0a76edb7fcb96262980340e521fc360a5

@bertmaher bertmaher force-pushed the bertmaher:tests branch from bf3c4ff to b5dee6d Sep 10, 2019

std::set<std::string> glow::backendTestBlacklist = {};
```

This blacklist can be used to exclude any unsupported tests while a backend is

This comment has been minimized.

Copy link
@opti-mix

opti-mix Sep 10, 2019

Contributor

Nice! It would be still useful to give an example of a shell-command to quickly blacklist all/some tests.

This comment has been minimized.

Copy link
@bertmaher

bertmaher Sep 10, 2019

Author Contributor

Oops, added that to the internal diff and forgot to push to GH. Should be here in a minute :).

bertmaher added a commit to bertmaher/glow that referenced this pull request Sep 10, 2019
Register backend-parameterized tests inside lib/Backends (pytorch#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: pytorch#3476

Test Plan: eventually, run all the tests.

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: 1e4761ddc8639aa444da96b6c0d88f3499cea28c

@bertmaher bertmaher force-pushed the bertmaher:tests branch from b5dee6d to c8d8148 Sep 10, 2019

Register backend-parameterized tests inside lib/Backends (#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: #3476

Test Plan: eventually, run all the tests.

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: d492676e67edbfd37ce72eae0914f9d53f39916b

@bertmaher bertmaher force-pushed the bertmaher:tests branch from c8d8148 to 87e5ebc Sep 10, 2019

bertmaher added a commit to bertmaher/glow that referenced this pull request Sep 11, 2019
Register backend-parameterized tests inside lib/Backends (pytorch#3476)
Summary:
This diff shows a possible approach to moving our backend parameterization of tests into lib/Backends.

The basic problem we're trying to solve is that vendors maintaining private backends may want to use, e.g., the OperatorTest suite to verify conformance, but currently the "ENABLED_BACKENDS" mechanism requires us to list each supported backend, for each test, in the source code.  Obviously, MyStealthModeStartup is not going to want that name appearing in the open-source operator test, so they've got to maintain a private, forked version of OperatorTest that enables all the tests relevant to their architecture.

By moving the parameterization into lib/Backends/*, we just have a test consisting of a simple source file listing the instantiated test cases, e.g.,
```
INSTANTIATE_GLOW_BACKEND_TEST(MyStealthModeStartup, OperatorTest, Conv2D);
```

Obviously the provided test `NewTest` is just a sample that wouldn't land; I ripped one of the cases out of OperatorTest for ease of editing.

I'll say:
- There's a bit more ceremony involved than I'd strictly like (mostly because the CMake lib list is pretty unwieldy).
- It feels odd to define all the tests in a header file, and put just the instantiates in a .cpp file.  I couldn't see a good way around this, though, since gtest requires you to instantiate entire test suites, rather than individual tests, and we need finer-grain control over test enablement.
- I don't love making the include path start with "tests/", maybe there's some way around this.
Pull Request resolved: pytorch#3476

Test Plan: eventually, run all the tests.

Reviewed By: rdzhabarov

Differential Revision: D17215560

Pulled By: bertmaher

fbshipit-source-id: 1eff172b8c7367e66988d4481a8d0a4404a638e1
@facebook-github-bot

This comment has been minimized.

Copy link

commented Sep 11, 2019

@bertmaher merged this pull request in d12d6d0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.