You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/10/14 20:26:24 UTC
[GitHub] [tvm] zxybazh opened a new pull request, #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
zxybazh opened a new pull request, #13084:
URL: https://github.com/apache/tvm/pull/13084
The current datatype issue #12997 could cause the postproc application to fail in massive number, causing tons of exception logging onto the screen via `LOG(WARN)`. This change removes the postproc application exception logging in the utility of search strategy, the reasons are:
1. The errors are triggered in multi-thread environment, the logging is hard to read anyway
2. We have a large population of candidates to mutate and apply postproc, in scenarios like the data type issue, it generates tons of logging, which is not helpful.
3. To debug we always need to run the postproc alone, which doesn’t matter in this case.
4. Without the warning log, we can still see failed postproc number count in the logging, and check for failure if necessary. If all the application fails, we would be able to see the workload fails in the performance tuning table. Example:
```c++
2022-10-13 17:03:42.383 INFO /home/apache/tvm/src/meta_schedule/search_strategy/evolutionary_search.cc:595 Evolve iter # 3 done. Summary:
Postproc # 0 [meta_schedule.DisallowDynamicLoop(0x561b625b8238)]: 0 failure(s)
Postproc # 1 [meta_schedule.RewriteParallelVectorizeUnroll(0x561b62606e78)]: 589 failure(s)
Postproc # 2 [meta_schedule.RewriteReductionBlock(0x561b6266a358)]: 0 failure(s)
Postproc # 3 [meta_schedule.RewriteLayout(0x561b625323b8)]: 0 failure(s)
```
There’s another alternate way I thought about to do this, which is to add a field called `postproc exception` counter that can output after the output of failures like
```c++
2022-10-13 17:03:42.383 INFO /home/apache/tvm/src/meta_schedule/search_strategy/evolutionary_search.cc:595 Evolve iter # 3 done. Summary:
Postproc # 0 [meta_schedule.DisallowDynamicLoop(0x561b625b8238)]: 0 failure(s)
Postproc #1 [meta_schedule.RewriteParallelVectorizeUnroll(0x561b62606e78)]: 0 failure(s) 589 exceptions(s)
Postproc # 2 [meta_schedule.RewriteReductionBlock(0x561b6266a358)]: 0 failure(s)
Postproc # 3 [meta_schedule.RewriteLayout(0x561b625323b8)]: 0 failure(s)
```
I think this is unnecessary given we can always check failure reasons here and we would always expect postproc to work except verification postprocs.
There's only one other logging interface like this in Postproc implementation [here](https://github.com/apache/tvm/blob/605876e638a7303acb63ef8a9dd9ebfefc30ae24/src/meta_schedule/postproc/rewrite_tensorize.cc#L46) in `RewriteTensorize` post processor. I think this output can potentially cause the same problem, we can address it if we have consensus on the current logging interface change.
CC @junrushao @vinx13
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r996145902
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
This issue is not caused by a single candidate TIR running multiple times but thousands of them generated by sampler or mutator. They could all be different from each other so only log some number of exceptions is not representative. On the other hand, this is applied in multi-thread, if we keep track of the bounded logging numbers, it may impact the tuning speed.
Even if we don't log out the exceptions here, we keep track of the failing count, and it's reflected in the log. If the failing count doesn't make sense, it's easy to spot from the log. And we can easily reproduce with running the postprocessor separately once there's any issue. We should be able to see failed tuning of this workload or impacted performance as a result.
One thing we can do to further improve visibility is like I said create a exception counter field in the output, or simply set a check that warns user when there's unexpected postproc failure in the counter. Let me know which one you think is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r996145902
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
Thanks for pointing that out Andrew!
This issue is not caused by a single candidate TIR running multiple times but by thousands of TIR candidate workloads generated by search space samples or mutators. They could all be different from each other. Therefore, only log some number of exceptions may not be representative. On the other hand, this is applied in multi-thread, if we keep track of the bounded logging numbers, it may slightly impact the tuning speed.
Even though we don't log out the exceptions here, we still keep track of the failing count, and it's still reflected in the log. If the failing count doesn't make sense, it's easy to spot that from the log. And we can also easily reproduce the issue with running the postprocessor separately once there's any issue. Moreover, we should be able to see failed tuning of this workload or impacted performance in the performance table as a result.
One thing we can do to further improve visibility is create an exception counter field in the output as I mentioend above, or simply set a check that warns user when there's unexpected post processor failures in the counter.
Would like to hear everyone’s thought on how to proceed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh commented on PR #13084:
URL: https://github.com/apache/tvm/pull/13084#issuecomment-1284430500
Following up on the discussion, I agree with Masa’s idea of letting it crash if the exception of postproc is unexpected, and let the postproc itself handle the expected exceptions. In this case, I removed the try-catch here instead, and expect any crash of the postproc application to be a bug to look into.
Please take another look on the PR @7andrew7 @masahi , thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r996145902
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
Thanks for pointing that out!
This issue is not caused by a single candidate TIR running multiple times but by thousands of TIR candidate workloads generated by search space samples or mutators. They could all be different from each other. Therefore, only log some number of exceptions may not be representative. On the other hand, this is applied in multi-thread, if we keep track of the bounded logging numbers, it may slightly impact the tuning speed.
Even though we don't log out the exceptions here, we still keep track of the failing count, and it's still reflected in the log. If the failing count doesn't make sense, it's easy to spot that from the log. And we can also easily reproduce the issue with running the postprocessor separately once there's any issue. Moreover, we should be able to see failed tuning of this workload or impacted performance in the performance table as a result.
One thing we can do to further improve visibility is create an exception counter field in the output as I mentioend above, or simply set a check that warns user when there's unexpected post processor failures in the counter.
Would like to hear everyone’s thought on how to proceed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r996145902
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
This issue is not caused by a single candidate TIR running multiple times but thousands of them generated by sampler or mutator. They could all be different from each other so only log some number of exceptions is not representative. On the other hand, this is applied in multi-thread, if we keep trace of the logging numbers, it may impact the tuning speed.
Even if we don't log out the exceptions here, we keep track of the failing count, and it's reflected in the log. If the failing count doesn't make sense, it's easy to spot from the log. And we can easily reproduce with running the postprocessor separately once there's any issue. We should be able to see failed tuning of this workload or impacted performance as a result.
One thing we can do to further improve visibility is like I said create a exception counter field in the output, or simply set a check that warns user when there's unexpected postproc failure in the counter. Let me know which one you think is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] 7andrew7 commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
7andrew7 commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r996135473
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
One problem here is that we have no visibility into failures.
Would it make sense to catch and log a bounded number before giving up and re-raising?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a diff in pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13084:
URL: https://github.com/apache/tvm/pull/13084#discussion_r998807081
##########
src/meta_schedule/utils.h:
##########
@@ -318,14 +318,16 @@ struct ThreadedTraceApply {
for (int i = 0; i < n_; ++i) {
Item& item = items_[i];
+ bool applied = false;
try {
- if (!item.postproc->Apply(sch)) {
- ++item.fail_counter;
- return NullOpt;
+ if (item.postproc->Apply(sch)) {
+ applied = true;
}
} catch (const std::exception& e) {
- // Used in multi-thread, only output to screen but failure summary sent to logging
- LOG(WARNING) << "ThreadedTraceApply::Apply failed with error " << e.what();
+ // left blank intentionally
Review Comment:
"Benign" failures should be caught at the postproc level, e.g. this is how the error is handled for auto tensorization https://github.com/apache/tvm/blob/02c2eae510d6d6c15189427c97819f7ce05f002d/src/meta_schedule/postproc/rewrite_tensorize.cc#L43-L46.
Other errors that propagate to `ThreadedTraceApply` level should be treated as a bug in MS. So I think it is ok to remove this log or this try /catch entirely, and let it crash to reveal the error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh merged pull request #13084: [MetaSchedule] Remove Exception Catch for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
zxybazh merged PR #13084:
URL: https://github.com/apache/tvm/pull/13084
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13084: [MetaSchedule] Remove Logging for Multithread Postproc Application
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13084:
URL: https://github.com/apache/tvm/pull/13084#issuecomment-1279428188
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org