Tc check #460

thetheodor · 2018-05-29T15:37:05Z

closes #351

skimo-openhub · 2018-05-29T15:54:37Z

On Tue, May 29, 2018 at 08:37:19AM -0700, Theodoros Theodoridis wrote: closes #351

I haven't looked at it in detail yet, but this looks great. skimo

skimo-openhub

It would be nice to have some documentation in 72f22d3.
Otherwise looks good.

skimo-openhub · 2018-05-30T12:11:08Z

tc/core/check.cc

+Checker tc_check(bool condition, const char* filename, uint64_t lineno) {
+  if (condition) {
+    return Checker(condition, {}, {});
+  }


Why this special case? Is it to avoid calling makeLocation?
Why not simply store filename and lineno?
Also, why does only this variant have such a special case?

If the condition holds then the strings are not used, it's an "optimization".

It was present in the rest of tc_check_* but I removed it because it didn't feel very useful. I forgot to remove this one.

skimo-openhub · 2018-05-30T12:12:58Z

tc/core/check.h

+  bool condition_;
+  std::string location_;
+  std::string baseErrorMsg_;
+  std::string additionalMsg_;


Why do you need to make a distinction between these two types of messages?

There's no need. I'll merge them.

On a second thought there is one use case:

TC_CHECK_X(...) << "A" << "B";

What I initially implemented will print the following:

Check failed [file:lineno] "what failed": AB

The "what failed" depends on the type of check, e.g. for TC_CHECK_EQ(1,2) it would be 1 not equal to 2. This part of the output is always present.

Extra information (passed with <<) is optional. The reason I keep track of it in a separate string is to figure out if and where the separator (:) and the space before it should be printed.

There are two options:

The optional/extra information is maintained on a separate string.

The space and separator is appended on the first call to operator<< and there is extra state that keeps track of it (e.g., a boolean).

skimo-openhub · 2018-05-30T12:41:06Z

test/test_check.cc

+
+TEST(CHECK, Vector) {
+  std::stringstream expected;
+  expected << "Check failed [" << __FILE__ << ':' << __LINE__ + 3 << ']'


This looks fragile and probably compiler dependent.
Why can you assume that the reported line number is the last line of
the macro invocation? Better make sure ASSERT_THROW_WHAT call
is within a single line in these tests.

Do we still have format checking enabled on CI? If yes, then it will complain.

skimo-openhub · 2018-05-30T12:43:12Z

test/test_check.cc

+  {
+    std::stringstream expected;
+    expected << "Check failed [" << __FILE__ << ':' << __LINE__ + 2
+             << "] 1 not less than " << 1;


Why is the second "1" outside the string (and not the first one)?
Similarly below.

No reason. I'll fix it.

skimo-openhub

This should obviously be reformatted.
tc/lang/gfg/parser.cc should probably be reformatted separately first.

skimo-openhub · 2018-05-30T12:46:13Z

tc/autotuner/genetic_search.cc

      << "the mutation rate (" << mutationRate       \
      << ") should be in the [0,100] interval";      \
-  CHECK(crossOverRate >= 0 and crossOverRate <= 100) \
+  TC_CHECK(crossOverRate >= 0 and crossOverRate <= 100) \
      << "the crossover (" << crossOverRate          \
      << ") rate should be in the [0,100] interval";


skimo-openhub · 2018-05-30T12:46:35Z

tc/autotuner/msg

@@ -0,0 +1 @@
+#259 switched from TC function names to using whole TCs (canonicalized) as part of compilation cache keys. However, the final step of the autotuner, that is retrieving the best found options, was not updated and it used just the function name. As a result, an empty optional<CudaMappingOptions> was always returned.


what's this?

I must have accidentally committed this.

skimo-openhub · 2018-05-30T12:48:27Z

tc/core/cuda/cuda_rtc.cc

@@ -20,6 +20,7 @@
 #include <cuda_runtime.h>
 #include <nvrtc.h>

+#include "tc/core/check.h"


Why only here (and a couple of other places)? Shouldn't every file that uses these calls have this include?

skimo-openhub · 2018-05-30T12:50:33Z

tc/core/tensor-inl.h

@@ -18,9 +18,10 @@
 #include <algorithm>
 #include <sstream>

-#include <glog/logging.h>
+//#include <glog/logging.h>


remove instead of commenting out

skimo-openhub · 2018-05-30T13:24:30Z

On Wed, May 30, 2018 at 06:18:15AM -0700, Theodoros Theodoridis wrote: ttheodor commented on this pull request. > + std::stringstream expected; + expected << "Check failed [" << __FILE__ << ':' << __LINE__ + 1 << ']'; + ASSERT_THROW_WHAT(TC_CHECK(false), expected.str()); + } + + { + std::stringstream expected; + expected << "Check failed [" << __FILE__ << ':' << __LINE__ + 2 << ']' + << ": 1+1=3"; + ASSERT_THROW_WHAT(TC_CHECK(false) << "1+1=3", expected.str()); + } +} + +TEST(CHECK, Vector) { + std::stringstream expected; + expected << "Check failed [" << __FILE__ << ':' << __LINE__ + 3 << ']' Do we still have format checking enabled on CI? If yes, then it will complain.

Yes. Complain about what? skimo

thetheodor · 2018-05-30T13:26:20Z

Υes. Complain about what?

This:

This looks fragile and probably compiler dependent.
Why can you assume that the reported line number is the last line of
the macro invocation? Better make sure ASSERT_THROW_WHAT call
is within a single line in these tests.

skimo-openhub · 2018-05-30T13:33:12Z

On Wed, May 30, 2018 at 06:26:23AM -0700, Theodoros Theodoridis wrote: > Υes. Complain about what? This: > This looks fragile and probably compiler dependent. > Why can you assume that the reported line number is the last line of > the macro invocation? Better make sure ASSERT_THROW_WHAT call > is within a single line in these tests.

Why would it complain if you arrange for all ASSERT_THROW_WHAT calls to stay within a line? skimo

skimo-openhub · 2018-05-31T13:05:30Z

On Thu, May 31, 2018 at 05:51:53AM -0700, Theodoros Theodoridis wrote: Extra information (passed with `<<`) is optional. The reason I keep track of it in a separate string is to figure out if and where the separator (`:`) and the space before it should be printed.

OK. skimo

thetheodor · 2018-05-31T13:30:55Z

All comments were addressed.

skimo-openhub

It seems you dragged in some other commits. Can we have just the top two on top of master?
Also CI is complaining.

nicolasvasilache

Great @ttheodor this is a welcome cleanup.

As a follow up do you intend to have a way to pass the type of exception to TC_CHECK_XXX like (I think) we had discussed at some point or do you think the vast majority of cases are handled just by CHECK + explicit throw?

thetheodor · 2018-05-31T18:57:36Z

Being able to specify the exception type makes a lot of sense. How about adding "overloads" with an extra exception type parameter? For example:

CHECK_LT(i,size, std::out_of_range);

nicolasvasilache · 2018-05-31T20:10:54Z

@ttheodor sounds good to me but let's do it as a followup PR? No point in delaying landing this nice improvement.

skimo-openhub

Thanks for the documentation. I fixed the CI issue (hopefully).

thetheodor · 2018-06-01T14:01:44Z

@nicolasvasilache since this was still unmerged I went ahead and implemented the checks with custom exception types.

nicolasvasilache · 2018-06-04T01:15:59Z

@ttheodor you should be able to rebase and land this now

thetheodor · 2018-06-04T11:33:48Z

Rebased

nicolasvasilache · 2018-06-08T04:25:55Z

rebase again I guess

For some reason they only triggered for me when trying to rebase tc_check. This is still relevant as an independent fix.

The glog CHECK macros terminate on failure. Instead of termination the TC_CHECK macros throw exceptions. The available macros are: TC_CHECK(condition) (boolean argument) TC_CHECK_EQ(x, y) (checks if x == y) TC_CHECK_NE(x, y) (checks if x != y) TC_CHECK_LT(x, y) (checks if x < y) TC_CHECK_GT(x, y) (checks if x > y) TC_CHECK_GE(x, y) (checks if x <= y) TC_CHECK_LE(x, y) (checks if x >= y) Additional information can be passed through operator<< and is included in the exception's error message, e.g.: TC_CHECK_EQ(x, 42) << "x is not the answer"; Implementation detail: Information passed with calls to operator<< is maintained in a separate string. The reason is that a separator is printed between the base message and the additional info if it is present (e.g., 1 is not equal to 42: x is not the answer). Maintaining two strings helps determining the correct location for the separator.

The type of the exception thrown on failed checks can now be specified. E.g., TC_CHECK(i, v.size(), std::out_of_range)

nicolasvasilache · 2018-06-11T03:52:51Z

Landing this now without extra reviews because it has been accepted multiple times and ends up being delayed over and over since everything conflicts with it.

nicolasvasilache

Great @ttheodor , thanks!

skimo-openhub · 2018-06-11T07:37:39Z

tc/core/mapping_options-inl.h

@@ -161,7 +161,7 @@ MappingOptionsView& MappingOptionsView::scheduleFusionStrategy(

 MappingOptionsView& MappingOptionsView::scheduleFusionStrategy(
    const std::string& str) {
-  FusionStrategy fs;


Who complains about this and why?

see #500, I did not understand that either.

see #504 (it should show in the CI)

thetheodor requested review from nicolasvasilache, ftynse and skimo-openhub May 29, 2018 15:37

facebook-github-bot added the CLA Signed label May 29, 2018

skimo-openhub reviewed May 30, 2018

View reviewed changes

skimo-openhub suggested changes May 30, 2018

View reviewed changes

thetheodor force-pushed the tc_check branch 2 times, most recently from 0c1e9c2 to 0ef9454 Compare May 31, 2018 13:30

thetheodor force-pushed the tc_check branch from 0ef9454 to 49975ae Compare May 31, 2018 15:27

skimo-openhub self-requested a review May 31, 2018 16:19

skimo-openhub reviewed May 31, 2018

View reviewed changes

skimo-openhub previously approved these changes May 31, 2018

View reviewed changes

thetheodor dismissed skimo-openhub’s stale review via c1128d4 May 31, 2018 16:28

thetheodor force-pushed the tc_check branch from 49975ae to c1128d4 Compare May 31, 2018 16:28

nicolasvasilache previously approved these changes May 31, 2018

View reviewed changes

skimo-openhub dismissed nicolasvasilache’s stale review via e8720ac June 1, 2018 09:27

skimo-openhub force-pushed the tc_check branch from c1128d4 to e8720ac Compare June 1, 2018 09:27

skimo-openhub self-requested a review June 1, 2018 09:30

skimo-openhub previously approved these changes Jun 1, 2018

View reviewed changes

thetheodor dismissed skimo-openhub’s stale review via f1cb2b5 June 1, 2018 13:59

thetheodor force-pushed the tc_check branch from f1cb2b5 to 3f4bef4 Compare June 4, 2018 11:33

ftynse previously approved these changes Jun 4, 2018

View reviewed changes

thetheodor dismissed ftynse’s stale review via 634c512 June 5, 2018 15:12

thetheodor force-pushed the tc_check branch from 3f4bef4 to 634c512 Compare June 5, 2018 15:12

nicolasvasilache force-pushed the tc_check branch from 634c512 to 2226b13 Compare June 11, 2018 00:37

nicolasvasilache and others added 2 commits June 10, 2018 18:38

Fix undefined variable errors

02b7b05

For some reason they only triggered for me when trying to rebase tc_check. This is still relevant as an independent fix.

nicolasvasilache force-pushed the tc_check branch from 2226b13 to 9c31a0c Compare June 11, 2018 02:28

Theodoros Theodoridis added 2 commits June 10, 2018 20:30

Replace all occurences of CHECK_* with TC_CHECK_*

31a30ef

Implement TC_CHECKs with custom exception types

3682a4f

The type of the exception thrown on failed checks can now be specified. E.g., TC_CHECK(i, v.size(), std::out_of_range)

nicolasvasilache force-pushed the tc_check branch from 9c31a0c to 3682a4f Compare June 11, 2018 02:31

nicolasvasilache approved these changes Jun 11, 2018

View reviewed changes

nicolasvasilache merged commit de3eea1 into master Jun 11, 2018

nicolasvasilache deleted the tc_check branch June 11, 2018 03:53

skimo-openhub reviewed Jun 11, 2018

View reviewed changes

skimo-openhub mentioned this pull request Jun 12, 2018

autotuner should catch compilation failures #193

Closed

		@@ -0,0 +1 @@
		#259 switched from TC function names to using whole TCs (canonicalized) as part of compilation cache keys. However, the final step of the autotuner, that is retrieving the best found options, was not updated and it used just the function name. As a result, an empty optional<CudaMappingOptions> was always returned.

Tc check #460

Tc check #460

Conversation

thetheodor commented May 29, 2018

skimo-openhub commented May 29, 2018 via email

skimo-openhub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skimo-openhub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skimo-openhub commented May 30, 2018 via email

thetheodor commented May 30, 2018

skimo-openhub commented May 30, 2018 via email

skimo-openhub commented May 31, 2018 via email

thetheodor commented May 31, 2018

skimo-openhub left a comment

Choose a reason for hiding this comment

nicolasvasilache left a comment

Choose a reason for hiding this comment

thetheodor commented May 31, 2018

nicolasvasilache commented May 31, 2018

skimo-openhub left a comment

Choose a reason for hiding this comment

thetheodor commented Jun 1, 2018

nicolasvasilache commented Jun 4, 2018

thetheodor commented Jun 4, 2018

nicolasvasilache commented Jun 8, 2018

nicolasvasilache commented Jun 11, 2018

nicolasvasilache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment