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] Restore num_threads parameter in tuning API #13561

Merged
merged 11 commits into from
Dec 9, 2022

Conversation

masahi
Copy link
Member

@masahi masahi commented Dec 6, 2022

num_threads parameter in the Relay tuning API was (accidentally?) removed in #12895. This PR restores this parameter and also uses it consistently for XGB model training and builder / runner as well.

@zxybazh

@tvm-bot
Copy link
Collaborator

tvm-bot commented Dec 6, 2022

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 by @-ing them in a comment.

Generated by tvm-bot

@junrushao
Copy link
Member

I am not sure it should be a top-level parameter as it’s less frequently used by introductory-level users, but don’t have much strong opinions.

In the meantime, I’m not sure if using the same num_threads for xgb and for evo search is a good idea because I assume xgb prefers physical cores? I don’t have numbers handy so I’m not 100% sure.

Removed this because @spectrometerHBH @Hzfengsy @jinhongyii @MasterJH5574 suggested to, so I’d love to hear more from you guys.

@spectrometerHBH
Copy link
Contributor

spectrometerHBH commented Dec 6, 2022

I would suggest using a better name because num_threads can be confusing, especially when the TIR programs to be tuned are using parallelization.

@masahi
Copy link
Member Author

masahi commented Dec 6, 2022

The main use case of this param is for tuning on a high-core system shared by many users. Currently if one user starts tuning, it occupies all CPU resources, which disrupts other users. So the goal is to limit the number of cores used by MS throughout the tuning process (evo search, post order apply, XGB training, builder / runner).

Also, tune_tir API has num_threads param as well.

num_threads: Union[Literal["physical", "logical"], int] = "physical",

I would suggest using a better name because num_threads can be confusing

Agreed, but that's what TuneContext calls... I can replace num_threads in the high-level API with max_workers or something, and initialize TuneContext by num_threads=max_workers. If people think this is better I can do that, otherwise I'd keep the existing convention.

Copy link
Member

@zxybazh zxybazh left a comment

Choose a reason for hiding this comment

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

I'm kinda in favor of this change because it allows more customization. We may want to find a good naming and default value for num_threads though.

@junrushao
Copy link
Member

I don't have a clear idea (very bad at naming). Perhaps @tqchen you could suggest?

@masahi
Copy link
Member Author

masahi commented Dec 7, 2022

I'm going to go with num_tuning_cores per discussion with @zxybazh, if there is no objection. For now I'll keep num_threads in TuneContext.

@junrushao
Copy link
Member

The reason that I don’t like “cores” is that they are threads, which are not necessarily related to physical cpu cores. Perhaps num_threads makes more sense at the moment.

@zxybazh
Copy link
Member

zxybazh commented Dec 7, 2022

Just want to add that Runner & Builder are using new processes, that's why we may want to consider using other terminology. On the other hand, if we don't have very good idea on new naming, I don't mind just sticking to the current naming nun_threads before that.

@masahi
Copy link
Member Author

masahi commented Dec 7, 2022

I think "cores" is better because we are also using this parameter to set the number of workers used by builder / runner. Also, when a user wants to limit the amount of CPU resources used by MS, they would think in terms of the number of "cores", not "threads". So as an API, "core" sounds more intuitive to me.

@junrushao
Copy link
Member

The argument that builder/runner workers are using processes rather than threads makes sense to me. Thanks for the explanation! Then I'm happy with the "core" terminology

@masahi
Copy link
Member Author

masahi commented Dec 8, 2022

Replaced num_threads with num_tuning_cores in the API.

Copy link
Member

@zxybazh zxybazh 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
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

@masahi masahi merged commit 3b001ef into apache:main Dec 9, 2022
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
…13561)

* [MetaSchedule] Restore num_threads argument in tune_relay

* pass num_threads to XGBModel

* fix default

* pass num_threads as max_workers to Builder and Runner

* add test

* clean up

* fix kwarg

* num_threads -> num_tuning_cores

* typo

* num_threads -> num_tuning_cores in contrib/torch

* typo in document
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
…13561)

* [MetaSchedule] Restore num_threads argument in tune_relay

* pass num_threads to XGBModel

* fix default

* pass num_threads as max_workers to Builder and Runner

* add test

* clean up

* fix kwarg

* num_threads -> num_tuning_cores

* typo

* num_threads -> num_tuning_cores in contrib/torch

* typo in document
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.

None yet

6 participants