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.

Sign up for GitHub

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

Jump to bottom

[TVMC] Runner.py Updates #7779

Merged
merged 13 commits into from Apr 7, 2021
Merged

[TVMC] Runner.py Updates #7779

merged 13 commits into from Apr 7, 2021

Conversation

CircleSpin
Copy link
Contributor

This PR does three things:

  1. Makes hostname optional in run_module
  2. Makes device mandatory in run_module
  3. Changes the runner time measurements to be in milliseconds rather than seconds (with additional test updates)

@CircleSpin
Copy link
Contributor Author

@jwfromm @mdw-octoml @leandron @comaniac

Please let me know your thoughts :)

comaniac
comaniac requested changes Mar 31, 2021
View reviewed changes
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.

First, if we agree with this change, your need to adjust the argument order. A required position argument cannot be placed after an optional argument. Second, I didn't find the logic of handling the default value when hostname is None, nor I'm not sure if this makes sense to make it optinoal.

python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
@jwfromm
Copy link
Contributor

jwfromm commented Mar 31, 2021

@comaniac, if you look at the full function, its already handling hostname=None and requiring that device is set. The change to the arguments here just more closely matches the actual behavior.

@comaniac
Copy link
Contributor

comaniac commented Apr 1, 2021

@comaniac, if you look at the full function, its already handling hostname=None and requiring that device is set. The change to the arguments here just more closely matches the actual behavior.

You're right. Yeah then it makes sense to adjust the arguments to align the actual behavior. Then please change the order accordingly and update the type in docstring.

comaniac
comaniac approved these changes Apr 1, 2021
View reviewed changes
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.

LGTM
cc @leandron

@jwfromm jwfromm merged commit 417c598 into apache:main Apr 7, 2021
@jwfromm
Copy link
Contributor

jwfromm commented Apr 7, 2021

Thanks @CircleSpin and @comaniac. This is now merged.

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@CircleSpin @jwfromm
[TVMC] Runner.py Updates (apache#7779)
f240e8c 
* change runner to ms instead of s, consider reformatting

* adjust formatting and test in test_runner.py to be more realistic

* change device in run_module runner.py to be mandatory

* make hostname optional in run_module, in runner.py

* update order and doc string

* remove print statement

* black files

* device error lint

* argument order was incorrect

* arguments funkiness attempt fix 2

* Fix merge with main.

Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
Co-authored-by: Josh Fromm <jwfromm@uw.edu>
Co-authored-by: Josh Fromm <jwfromm@octoml.ai>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@CircleSpin @jwfromm
[TVMC] Runner.py Updates (apache#7779)
bba762f 
* change runner to ms instead of s, consider reformatting

* adjust formatting and test in test_runner.py to be more realistic

* change device in run_module runner.py to be mandatory

* make hostname optional in run_module, in runner.py

* update order and doc string

* remove print statement

* black files

* device error lint

* argument order was incorrect

* arguments funkiness attempt fix 2

* Fix merge with main.

Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
Co-authored-by: Josh Fromm <jwfromm@uw.edu>
Co-authored-by: Josh Fromm <jwfromm@octoml.ai>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@CircleSpin @jwfromm
[TVMC] Runner.py Updates (apache#7779)
5d4cc8e 
* change runner to ms instead of s, consider reformatting

* adjust formatting and test in test_runner.py to be more realistic

* change device in run_module runner.py to be mandatory

* make hostname optional in run_module, in runner.py

* update order and doc string

* remove print statement

* black files

* device error lint

* argument order was incorrect

* arguments funkiness attempt fix 2

* Fix merge with main.

Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
Co-authored-by: Josh Fromm <jwfromm@uw.edu>
Co-authored-by: Josh Fromm <jwfromm@octoml.ai>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
@CircleSpin @jwfromm @trevor-m
[TVMC] Runner.py Updates (apache#7779)
a73e2a3 
* change runner to ms instead of s, consider reformatting

* adjust formatting and test in test_runner.py to be more realistic

* change device in run_module runner.py to be mandatory

* make hostname optional in run_module, in runner.py

* update order and doc string

* remove print statement

* black files

* device error lint

* argument order was incorrect

* arguments funkiness attempt fix 2

* Fix merge with main.

Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
Co-authored-by: Josh Fromm <jwfromm@uw.edu>
Co-authored-by: Josh Fromm <jwfromm@octoml.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comaniac comaniac comaniac approved these changes

@jwfromm jwfromm jwfromm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@CircleSpin @jwfromm @comaniac