-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
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 |
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. |
I would suggest using a better name because num_threads can be confusing, especially when the TIR programs to be tuned are using parallelization. |
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,
Agreed, but that's what |
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.
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.
tests/python/contrib/test_hexagon/metaschedule_e2e/test_resnet50_int8.py
Outdated
Show resolved
Hide resolved
b3599ae
to
71452b5
Compare
I don't have a clear idea (very bad at naming). Perhaps @tqchen you could suggest? |
I'm going to go with |
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. |
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. |
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. |
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 |
Replaced |
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
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
…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
…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
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