-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[microTVM][AutoTVM] Fix autotvm bug and tests #9003
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
Conversation
cebcd29 to
06d7331
Compare
tkonolige
left a comment
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.
Looks good. I'm glad autotvm now works with microtvm!
| measure_option=measure_option, | ||
| callbacks=[ | ||
| tvm.autotvm.callback.log_to_file("microtvm_autotune.log"), | ||
| tvm.autotvm.callback.log_to_file("microtvm_autotune.log.txt"), |
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.
Why the double extension here?
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.
to be consistent with cleanup step in doc stage:
tvm/tests/scripts/task_python_docs.sh
Line 31 in ff4bf3b
| rm -rf /tmp/$$.log.txt |
| std::shared_ptr<RPCEndpoint> RPCEndpoint::Create(std::unique_ptr<RPCChannel> channel, | ||
| std::string name, std::string remote_key) { | ||
| std::string name, std::string remote_key, | ||
| TypedPackedFunc<void()> fshutdown) { |
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.
@tqchen this is needed when a Python-side session constructor may keep a reference to the underlying session. Does it make sense to you?
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.
TQ requests change to fcleanup
areusch
left a comment
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.
thanks @mehrdadh!
| self.transport.__exit__(exc_type, exc_value, exc_traceback) | ||
|
|
||
| def _shutdown(self): | ||
| self.__exit__(None, None, None) |
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.
perhaps for a future cleanup: let's ensure we avoid double-calling __exit__ by checking some local var to determine whether we did already.
| std::string name_; | ||
| // The remote key | ||
| std::string remote_key_; | ||
| // The shutdown Packed Function |
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.
nit: say something more like: "invoked when the RPC session is terminated"
* debuggging * cleanup and fix tutorial, zephyr and crt test * fix crt test * address comments
* fix test and cleanup * fix tutorial doc * fix verbose for tutorial * fix tune check * address comments * address comments
* debuggging * cleanup and fix tutorial, zephyr and crt test * fix crt test * address comments
* fix test and cleanup * fix tutorial doc * fix verbose for tutorial * fix tune check * address comments * address comments
microTVM autotvm is still broken and our zephyr/crt tests doesn't catch the error. This PR fixed the autotvm and change the tests to capture autotvm error.
cc @areusch @tkonolige