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/05/26 21:48:15 UTC
[GitHub] [tvm] tkonolige opened a new pull request, #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
tkonolige opened a new pull request, #11479:
URL: https://github.com/apache/tvm/pull/11479
I'm hitting this issue when tuning `dense_pack.x86`.
@tqchen @merrymercy
--
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] merrymercy commented on pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
merrymercy commented on PR #11479:
URL: https://github.com/apache/tvm/pull/11479#issuecomment-1142693775
You are right. Sorry, I thought this was auto_scheduler instead of autotvm.
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887304060
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -243,18 +243,27 @@ def fit_log(self, records, plan_size, min_seed_records=500):
else:
raise RuntimeError("Invalid feature type: " + self.fea_type)
result = pool.map_with_error_catching(feature_extract_func, data)
+ result = list(result) # store results so we can iterate through them twice
- # filter out feature with different shapes
- fea_len = len(self._get_feature([0])[0])
+ # get maximum feature length
+ fea_len = -1
+ for res in result:
+ if res.status != StatusKind.COMPLETE:
+ continue
+ x, _ = res.value
+ fea_len = max(fea_len, x.shape[0])
xs, ys = [], []
for res in result:
if res.status != StatusKind.COMPLETE:
continue
x, y = res.value
- if len(x) == fea_len:
Review Comment:
Well, the training of the model only occurs on data that the model has already predicted performance for. The prediction code would then error out before we hit this training code which would gracefully fail by dropping data. I'm not sure how major of a change you would consider this then...
Usually the xgboost model keeps all previous features in its training set (sometime it drops the oldest ones). With this PR, the feature size is determined by the longest feature in the training set. So if there's a new round with a longer feature, then the feature length with grow.
--
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] AndrewZhaoLuo commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887226091
##########
python/tvm/testing/autotvm.py:
##########
@@ -62,9 +62,12 @@ def matmul(N, L, M, dtype):
# schedule according to config
yo, yi = cfg["tile_y"].apply(s, C, y)
- xo, xi = cfg["tile_x"].apply(s, C, x)
+ if cfg["tile_x"].size[-1] > 1:
+ xo, xi = cfg["tile_x"].apply(s, C, x)
Review Comment:
Can you give some context on this in the code? Is this to hit the previously failing case?
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -243,18 +243,27 @@ def fit_log(self, records, plan_size, min_seed_records=500):
else:
raise RuntimeError("Invalid feature type: " + self.fea_type)
result = pool.map_with_error_catching(feature_extract_func, data)
+ result = list(result) # store results so we can iterate through them twice
- # filter out feature with different shapes
- fea_len = len(self._get_feature([0])[0])
+ # get maximum feature length
+ fea_len = -1
+ for res in result:
+ if res.status != StatusKind.COMPLETE:
+ continue
+ x, _ = res.value
+ fea_len = max(fea_len, x.shape[0])
xs, ys = [], []
for res in result:
if res.status != StatusKind.COMPLETE:
continue
x, y = res.value
- if len(x) == fea_len:
Review Comment:
Hmm, I think previously glossed over the significance of this. Before, it would only use features from those with the same shape as the first entry to fit the model so basically loops with the same number of axis. So this is actually quite a major change is it not?
The model is at the task level, so how did it handle configs with different size features between training rounds?
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -329,15 +338,16 @@ def _get_feature(self, indexes):
for i, fea in zip(need_extract, feas):
fea_cache[i] = fea.value if fea.status == StatusKind.COMPLETE else None
- feature_len = None
+ feature_len = -1
for idx in indexes:
if fea_cache[idx] is not None:
- feature_len = fea_cache[idx].shape[-1]
- break
+ feature_len = max(fea_cache[idx].shape[-1], feature_len)
ret = np.empty((len(indexes), feature_len), dtype=np.float32)
for i, ii in enumerate(indexes):
t = fea_cache[ii]
+ if t.shape[0] < feature_len:
+ t = np.pad(t, (0, feature_len - t.shape[0]))
Review Comment:
I'm wondering if you've given thought on whether 0 is the most appropriate padding value. I know at this stage we don't have good insight into the actual corresponding features, but I am worried that 0 for some features may have some meaning we don't want.
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -329,15 +338,16 @@ def _get_feature(self, indexes):
for i, fea in zip(need_extract, feas):
fea_cache[i] = fea.value if fea.status == StatusKind.COMPLETE else None
- feature_len = None
+ feature_len = -1
for idx in indexes:
if fea_cache[idx] is not None:
- feature_len = fea_cache[idx].shape[-1]
- break
+ feature_len = max(fea_cache[idx].shape[-1], feature_len)
ret = np.empty((len(indexes), feature_len), dtype=np.float32)
for i, ii in enumerate(indexes):
t = fea_cache[ii]
+ if t.shape[0] < feature_len:
+ t = np.pad(t, (0, feature_len - t.shape[0]))
Review Comment:
I'm wondering if you've given thought on whether 0 is the most appropriate padding value. I know at this stage we don't have good insight into the actual corresponding features, but I am worried that 0 for some features may have some meaning we don't want.
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887226647
##########
tests/python/unittest/test_autotvm_xgboost_model.py:
##########
@@ -43,6 +43,8 @@ def test_fit():
upper_model.fit(xs, ys, plan_size=32)
+ upper_model.predict(np.ones(12))
Review Comment:
shorter will be padded. Let me add that to this portion of the test too
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887362429
##########
python/tvm/testing/autotvm.py:
##########
@@ -62,9 +62,12 @@ def matmul(N, L, M, dtype):
# schedule according to config
yo, yi = cfg["tile_y"].apply(s, C, y)
- xo, xi = cfg["tile_x"].apply(s, C, x)
+ if cfg["tile_x"].size[-1] > 1:
+ xo, xi = cfg["tile_x"].apply(s, C, x)
Review Comment:
done
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r886198392
##########
tests/python/unittest/test_autotvm_xgboost_model.py:
##########
@@ -43,6 +43,8 @@ def test_fit():
upper_model.fit(xs, ys, plan_size=32)
+ upper_model.predict(np.ones(12))
Review Comment:
Surprisingly, xgboost seems to be fine doing predictions on vectors longer than what it was trained on.
--
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] AndrewZhaoLuo merged pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo merged PR #11479:
URL: https://github.com/apache/tvm/pull/11479
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887321142
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -329,15 +338,16 @@ def _get_feature(self, indexes):
for i, fea in zip(need_extract, feas):
fea_cache[i] = fea.value if fea.status == StatusKind.COMPLETE else None
- feature_len = None
+ feature_len = -1
for idx in indexes:
if fea_cache[idx] is not None:
- feature_len = fea_cache[idx].shape[-1]
- break
+ feature_len = max(fea_cache[idx].shape[-1], feature_len)
ret = np.empty((len(indexes), feature_len), dtype=np.float32)
for i, ii in enumerate(indexes):
t = fea_cache[ii]
+ if t.shape[0] < feature_len:
+ t = np.pad(t, (0, feature_len - t.shape[0]))
Review Comment:
I think autoscheduler pads the extra features with zero, but I don't know if that would also be a good approach 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] tkonolige commented on pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #11479:
URL: https://github.com/apache/tvm/pull/11479#issuecomment-1142452381
@junrushao1994 @AndrewZhaoLuo Could one of you review?
--
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] AndrewZhaoLuo commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887225383
##########
tests/python/unittest/test_autotvm_xgboost_model.py:
##########
@@ -43,6 +43,8 @@ def test_fit():
upper_model.fit(xs, ys, plan_size=32)
+ upper_model.predict(np.ones(12))
Review Comment:
So longer is fine, but not shorter?
--
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] tkonolige commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887296132
##########
python/tvm/testing/autotvm.py:
##########
@@ -62,9 +62,12 @@ def matmul(N, L, M, dtype):
# schedule according to config
yo, yi = cfg["tile_y"].apply(s, C, y)
- xo, xi = cfg["tile_x"].apply(s, C, x)
+ if cfg["tile_x"].size[-1] > 1:
+ xo, xi = cfg["tile_x"].apply(s, C, x)
Review Comment:
Yeah, this is to hit the failing test case. It will conditionally split one of the loops so that some configurations have n itervars and some have n+1.
--
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] AndrewZhaoLuo commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887349583
##########
python/tvm/testing/autotvm.py:
##########
@@ -62,9 +62,12 @@ def matmul(N, L, M, dtype):
# schedule according to config
yo, yi = cfg["tile_y"].apply(s, C, y)
- xo, xi = cfg["tile_x"].apply(s, C, x)
+ if cfg["tile_x"].size[-1] > 1:
+ xo, xi = cfg["tile_x"].apply(s, C, x)
Review Comment:
Can you add a comment
--
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] tkonolige commented on pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #11479:
URL: https://github.com/apache/tvm/pull/11479#issuecomment-1142558863
@merrymercy as far as I can tell, feature extraction does not guarantee that feature vectors are the same length. Looking at `src/autotvm/touch_extractor.cc` https://github.com/apache/tvm/blob/main/src/autotvm/touch_extractor.cc#L335 the length of the the feature vector depends on the number of itervars in the program. If the schedule used applies a different number of splits based on some autotvm config, then the number of itervars will not be consistent.
It seems that we have to add padding somewhere. We can either add it in the xgb tuner or in the feature extractor. If we add it to the feature extractor, then we need to guess the maximum number of itervars allowed which could either lead to lots of wasted space in the feature vector or failures when there are more itervars than expected. To me it seems easier to apply the fix in the tuner.
--
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] tkonolige commented on pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #11479:
URL: https://github.com/apache/tvm/pull/11479#issuecomment-1142724655
@AndrewZhaoLuo good idea. I've added a test.
--
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] AndrewZhaoLuo commented on a diff in pull request #11479: [FIX] Pad feature vectors to the same size in xgboost cost model
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #11479:
URL: https://github.com/apache/tvm/pull/11479#discussion_r887349425
##########
python/tvm/autotvm/tuner/xgboost_cost_model.py:
##########
@@ -329,15 +338,16 @@ def _get_feature(self, indexes):
for i, fea in zip(need_extract, feas):
fea_cache[i] = fea.value if fea.status == StatusKind.COMPLETE else None
- feature_len = None
+ feature_len = -1
for idx in indexes:
if fea_cache[idx] is not None:
- feature_len = fea_cache[idx].shape[-1]
- break
+ feature_len = max(fea_cache[idx].shape[-1], feature_len)
ret = np.empty((len(indexes), feature_len), dtype=np.float32)
for i, ii in enumerate(indexes):
t = fea_cache[ii]
+ if t.shape[0] < feature_len:
+ t = np.pad(t, (0, feature_len - t.shape[0]))
Review Comment:
Yeah ok, i think it's outside scope of this pr but good to think about
--
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