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

Add autoscheduler support to tvmc #7070

Merged
merged 8 commits into from
Dec 17, 2020
Merged

Conversation

giuseros
Copy link
Contributor

@giuseros giuseros commented Dec 9, 2020

  • Add an autoschedule module to tvmc
  • Extract common tuning option between autotuner and autoscheduler
  • Add testing

@giuseros
Copy link
Contributor Author

giuseros commented Dec 9, 2020

cc @leandron @merrymercy

python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autoscheduler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
@giuseros
Copy link
Contributor Author

Hi @comaniac , @leandron ,
Thanks for your comments, now the code is way more sound. I thought of summing up here some issues left:

  • It's now hard to make autotuner vs autoscheduler options mutually exclusive. So if I specify --tuner with --enable-autoscheduling, the tuner I selected won't be considered. This is because we are using default values, so a tuner will be always defined.
  • Extracting tasks. @comaniac , you flagged the function, but this is actually how we do for the autotuner. I guess this is because we want to unit-test the function
  • Testing: I removed the test_autoscheduler.py test. When I wrote it, I was inspired by the test_autotuner.py. Can we agree on a set of tests we want to execute? @comaniac , I agree we should not test the autotuner, but only the user interface. But I fear to test the user interface, we will need to test a bit the functionality as well. What do you guys think?

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

It's much clean now and overall LGTM. Thanks!
The last batch of comments are mostly about refactoring.

python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Show resolved Hide resolved
python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
@comaniac
Copy link
Contributor

Hi @comaniac , @leandron ,
Thanks for your comments, now the code is way more sound. I thought of summing up here some issues left:

  • It's now hard to make autotuner vs autoscheduler options mutually exclusive. So if I specify --tuner with --enable-autoscheduling, the tuner I selected won't be considered. This is because we are using default values, so a tuner will be always defined.

I'm ok with this for now.

  • Extracting tasks. @comaniac , you flagged the function, but this is actually how we do for the autotuner. I guess this is because we want to unit-test the function

No worries. It is no longer an issue based on the latest implementation. It is reasonable to have separate functions for two frameworks launched by the same CLI command.

  • Testing: I removed the test_autoscheduler.py test. When I wrote it, I was inspired by the test_autotuner.py. Can we agree on a set of tests we want to execute? @comaniac , I agree we should not test the autotuner, but only the user interface. But I fear to test the user interface, we will need to test a bit the functionality as well. What do you guys think?

You're right. It's always hard to keep the proper unit test scope of TVMC. We should have a thread to discuss all of them and make a agreement. cc @leandron

@comaniac comaniac added the status: need update need update based on feedbacks label Dec 15, 2020
Giuseppe Rossini added 7 commits December 15, 2020 17:30
- Add an autoschedule module to tvmc
- Extract common tuning option between autotuner and autoscheduler
- Add testing
Change-Id: I207872757473210681d9db04bfdcd2c5e6deaa05
@giuseros
Copy link
Contributor Author

Hi @comaniac , @leandron ,
I updated the PR as follows:

  • I added two more options (--log-estimated-latency and --include-simple-tasks)
  • I added the tests back (as they were before).
    Please, let me know if those tests are ok (or if I need to add more or less), and if there is any other autoscheduler relevant option I need to consider.

Thanks,

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thanks. Just a naming nit.

python/tvm/driver/tvmc/autotuner.py Outdated Show resolved Hide resolved
Change-Id: I11f73c9b32e83c013cfb2224ccce06f60a128af7
@comaniac comaniac merged commit fb8de5a into apache:main Dec 17, 2020
@comaniac
Copy link
Contributor

Thanks @giuseros @leandron @insop.

@comaniac comaniac added status: accepted and removed status: need update need update based on feedbacks labels Dec 17, 2020
masahi pushed a commit to masahi/tvm that referenced this pull request Dec 18, 2020
* Add autoscheduler support to tvmc

- Add an autoschedule module to tvmc
- Extract common tuning option between autotuner and autoscheduler
- Add testing

* Linting and small bug-fixing

* Addressing comments and refactoring

* Fix linting

* rebasing

* Addressing comments - 2

* Addressing comments -3

Change-Id: I207872757473210681d9db04bfdcd2c5e6deaa05

* Addressing comments - 4

Change-Id: I11f73c9b32e83c013cfb2224ccce06f60a128af7
@merrymercy
Copy link
Member

For x86 cpu, it is recommended to use enable_cpu_cache_flush=True in LocalRunner, as shown in the tutorial.

runner=auto_scheduler.LocalRunner(repeat=10, enable_cpu_cache_flush=True),

This can make the measurement faster and more accurate. We should add this to tvmc.

@giuseros
Copy link
Contributor Author

Hi @merrymercy ,
Thanks for the comment! Sure, I will open a new PR to add this in

TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Jan 20, 2021
* Add autoscheduler support to tvmc

- Add an autoschedule module to tvmc
- Extract common tuning option between autotuner and autoscheduler
- Add testing

* Linting and small bug-fixing

* Addressing comments and refactoring

* Fix linting

* rebasing

* Addressing comments - 2

* Addressing comments -3

Change-Id: I207872757473210681d9db04bfdcd2c5e6deaa05

* Addressing comments - 4

Change-Id: I11f73c9b32e83c013cfb2224ccce06f60a128af7
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jan 21, 2021
* Add autoscheduler support to tvmc

- Add an autoschedule module to tvmc
- Extract common tuning option between autotuner and autoscheduler
- Add testing

* Linting and small bug-fixing

* Addressing comments and refactoring

* Fix linting

* rebasing

* Addressing comments - 2

* Addressing comments -3

Change-Id: I207872757473210681d9db04bfdcd2c5e6deaa05

* Addressing comments - 4

Change-Id: I11f73c9b32e83c013cfb2224ccce06f60a128af7
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
* Add autoscheduler support to tvmc

- Add an autoschedule module to tvmc
- Extract common tuning option between autotuner and autoscheduler
- Add testing

* Linting and small bug-fixing

* Addressing comments and refactoring

* Fix linting

* rebasing

* Addressing comments - 2

* Addressing comments -3

Change-Id: I207872757473210681d9db04bfdcd2c5e6deaa05

* Addressing comments - 4

Change-Id: I11f73c9b32e83c013cfb2224ccce06f60a128af7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants