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

[MetaSchedule] Fix XGBoost Import Issue #12936

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

zxybazh
Copy link
Member

@zxybazh zxybazh commented Sep 28, 2022

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 mentioened above under a function that returns this class when needed. In this way we avoided unwanted import of xgboost in meta_schedule.

CC @shingjan @junrushao

@zxybazh zxybazh marked this pull request as ready for review September 28, 2022 23:37
Copy link

@shingjan shingjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this! Moving xgboost out as a top-level import makes sense here.

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zxybazh, thanks for the PR. Can you clarify what this change is actually doing, in the description of the PR? I see some functions moved around, but I can't understand what is actually being introduced.

Apart from that, can you also add a unit test to make sure your fix don't get regress in future, as it seems this is fixing something that stopped working, and we didn't notice because we don't have a test to assert that behaviour. Thanks.

@zxybazh
Copy link
Member Author

zxybazh commented Sep 29, 2022

Hi @leandron, thanks for looking into this PR! Let me explain more context of what's going on in this PR. 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 mentioened above under a function that returns this class when needed. In this way we avoided unwanted import of xgboost in meta_schedule.

In this case, it's a bit tricky to create a unit test for that, I've tested locally though. Would really appreciate it if you know how to create a test case for unwanted import in meta_schedule.

@zxybazh
Copy link
Member Author

zxybazh commented Sep 29, 2022

Hi, I made some changes to the PR so that the test for legacy callback function is only run for certain xgboost version, and fixed the CI issues. Would you please take another look, thanks!

@junrushao
Copy link
Member

I think the main thing @leandron cares about is the documentation for each PR, i.e. would you mind if you add your explanation to the git commit?

@junrushao junrushao merged commit 4e4089e into apache:main Sep 30, 2022
@junrushao
Copy link
Member

i added the detailed explanation to the commit message

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants