-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Meta Schedule][XGBoost] Update the custom callback function of xgboost in meta schedule #12141
[Meta Schedule][XGBoost] Update the custom callback function of xgboost in meta schedule #12141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Generally it looks good, some more unit tests are required, and a few nit picks. I would also expect some integration test locally with the tune_relay
functions to make sure the tuning works fine with migrated cost model.
2fda92d
to
2f42667
Compare
2f42667
to
2c80287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes and feedbacks. The test looks ok to me and I will be fine with it as long as we can pass the CI. Let's finish the PR and get it in once the local integration tests are done!
be956d7
to
9444134
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit, otherwise LGTM. Would you please also run some local integration tests and let me know the results?
Local integration test for resnet18/llvm:
Profiler table:
|
@tvm-bot rerun |
bert base llvm:
profiler table
|
bert base cuda:
profiler table:
|
resnet18 cuda:
profiler table:
|
mobilenetv2 on cuda
profiler table
|
bert base on llvm 20k trials:
profiler table
|
087ed69
to
918ea89
Compare
918ea89
to
33eb891
Compare
@zxybazh The pandas warning should be suppressed now with the last commit. |
46507b3
to
33eb891
Compare
33eb891
to
08663ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shingjan for the hard work and @junrushao for reviewing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tvm-bot rerun |
Previous upgrade introduced a import of xgboost in meta_schedule, removed in current version by using a function to return the call back class. We've recently introduced a XGBoost Model upgrade to support new xgboost version of callback class in #12141. However, in this PR it uses a function called `optional_xgboost_callback` that works to avoid compatibility issue (xgboost 1.5.2 v.s. 1.6.0). In this specific function, it tries to import the newly introduced xgboost callback class and create a new class using it as base class. This actually imported xgboost when meta_schedule is imported, which is not ideal because xgboost is not a dependency of tvm and meta_schedule, it should only be required when xgboost cost model is employed. This PR fixes the problem by moving the class and the function mentioned above under a function that returns this class when needed. In this way we avoided unwanted import of xgboost in meta_schedule.
…st in meta schedule (apache#12141) * update the custom callback function of xgboost * fix lint * fix ci * fix lint * add unit test * remote unused code * fix lint * add decorator * address comment * fix lint * address comments * fix mypy * fix lint * remove unused comments * address comments * Fix xgboost unit test import. Co-authored-by: Xiyou Zhou <xiyou@octoml.ai>
Previous upgrade introduced a import of xgboost in meta_schedule, removed in current version by using a function to return the call back class. We've recently introduced a XGBoost Model upgrade to support new xgboost version of callback class in apache#12141. However, in this PR it uses a function called `optional_xgboost_callback` that works to avoid compatibility issue (xgboost 1.5.2 v.s. 1.6.0). In this specific function, it tries to import the newly introduced xgboost callback class and create a new class using it as base class. This actually imported xgboost when meta_schedule is imported, which is not ideal because xgboost is not a dependency of tvm and meta_schedule, it should only be required when xgboost cost model is employed. This PR fixes the problem by moving the class and the function mentioned above under a function that returns this class when needed. In this way we avoided unwanted import of xgboost in meta_schedule.
This PR intends to update the custom callback function of xgboost in meta schedule.
This change is tested against xgboost==(1.2.0, 1.5.2 & 1.6.0) to ensure backwards compatibility on
tests/python/unittest/test_meta_schedule_cost_model.py
.This is related to the second action item in #12009.
cc: @zxybazh @junrushao1994