You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "jinhongyii (via GitHub)" <gi...@apache.org> on 2023/09/05 21:22:50 UTC
[GitHub] [tvm] jinhongyii opened a new pull request, #15674: [Unity][Disco] Minor change for MLC+Disco integration
jinhongyii opened a new pull request, #15674:
URL: https://github.com/apache/tvm/pull/15674
1. Add wrapper for copytoworker0
2. change DRef and Session to nullable for easy initlialization
3. add a "func_exists" function in executable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1318649749
##########
src/runtime/relax_vm/executable.cc:
##########
@@ -75,6 +75,10 @@ PackedFunc Executable::GetFunction(const String& name, const ObjectPtr<Object>&
vm->LoadExecutable(GetObjectPtr<Executable>(this));
*rv = Module(vm);
});
+ } else if (name == "func_exists") {
Review Comment:
thinking a bit more, how about rename to has_function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316514183
##########
include/tvm/runtime/disco/session.h:
##########
@@ -195,6 +195,18 @@ class SessionObj : public Object {
* \param host_array The array to be copied to worker-0
* \param remote_array The NDArray on worker-0
*/
+
+ DRef CopyToWorker0Wrapper(const NDArray& host_array) {
Review Comment:
`Empty` is exposed on python: https://github.com/apache/tvm/blob/unity/python/tvm/runtime/disco/session.py#L117-L142.
Shall we migrate some of the python pieces to C++?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #15674: [Unity] Add `has_function` to RelaxVM
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1319340562
##########
src/runtime/relax_vm/executable.cc:
##########
@@ -75,6 +75,10 @@ PackedFunc Executable::GetFunction(const String& name, const ObjectPtr<Object>&
vm->LoadExecutable(GetObjectPtr<Executable>(this));
*rv = Module(vm);
});
+ } else if (name == "func_exists") {
Review Comment:
sounds good. I'm updating the name on my end to accelerate PR merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316532945
##########
include/tvm/runtime/disco/session.h:
##########
@@ -236,7 +248,7 @@ class Session : public ObjectRef {
public:
/*! \brief Create a session backed by a thread pool of workers */
static Session ThreadedSession(int num_workers);
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
Review Comment:
To be clear, it's not-nullable since it's properly created. If you wanted to cover the case where Session could be a field of another TVM Object, the common paradigm we are using across TVM codebase is to mark it as "initially null":
```C++
Session sess{nullptr};
```
Note that this pattern is widely used in MetaSchedule already.
There are multiple folds of reasons for using not-nullable, as TVM object system will enforce more checking, and also developers of other modules could feel very safe to use an object without worrying it's null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao merged pull request #15674: [Unity] Add `has_function` to RelaxVM
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao merged PR #15674:
URL: https://github.com/apache/tvm/pull/15674
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316539990
##########
include/tvm/runtime/disco/session.h:
##########
@@ -195,6 +195,18 @@ class SessionObj : public Object {
* \param host_array The array to be copied to worker-0
* \param remote_array The NDArray on worker-0
*/
+
+ DRef CopyToWorker0Wrapper(const NDArray& host_array) {
Review Comment:
If we want to expose in c++, that's only 2 lines of code. So I don't think it's necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316518795
##########
include/tvm/runtime/disco/session.h:
##########
@@ -236,7 +248,7 @@ class Session : public ObjectRef {
public:
/*! \brief Create a session backed by a thread pool of workers */
static Session ThreadedSession(int num_workers);
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
Review Comment:
In MLC we want to make Session a field. To initialize it, we are using ThreadSession(worker_num) instead of its constructor to create a session, so it has to be nullable to compile. Another reason is that in definition of DRef, we are already having
``` /*! \brief Back-pointer to the host controler session */
ObjectRef session{nullptr};
```
So there's no reason session can't be nullable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on PR #15674:
URL: https://github.com/apache/tvm/pull/15674#issuecomment-1709407700
please review @junrushao @tqchen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316513189
##########
include/tvm/runtime/disco/session.h:
##########
@@ -236,7 +248,7 @@ class Session : public ObjectRef {
public:
/*! \brief Create a session backed by a thread pool of workers */
static Session ThreadedSession(int num_workers);
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
Review Comment:
Why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316514867
##########
include/tvm/runtime/disco/session.h:
##########
@@ -155,7 +155,7 @@ class DRefObj : public Object {
*/
class DRef : public ObjectRef {
public:
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(DRef, ObjectRef, DRefObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(DRef, ObjectRef, DRefObj);
Review Comment:
If there isn't a particular reason, please keep objects as "not-nullable" as possible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316518795
##########
include/tvm/runtime/disco/session.h:
##########
@@ -236,7 +248,7 @@ class Session : public ObjectRef {
public:
/*! \brief Create a session backed by a thread pool of workers */
static Session ThreadedSession(int num_workers);
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
Review Comment:
In MLC we want to make Session a field. To initialize it, we are using ThreadSession(worker_num) instead of its constructor to create a session, so it has to be nullable to compile. Another reason is that in definition of DRef, we are already having
```
/*! \brief Back-pointer to the host controler session */
ObjectRef session{nullptr};
```
So there's no reason session can't be nullable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316512081
##########
include/tvm/runtime/disco/session.h:
##########
@@ -195,6 +195,18 @@ class SessionObj : public Object {
* \param host_array The array to be copied to worker-0
* \param remote_array The NDArray on worker-0
*/
+
+ DRef CopyToWorker0Wrapper(const NDArray& host_array) {
Review Comment:
To minimize the interface, let us consider only expose remote.Empty.
Then move the wrapper to the upper level applications
##########
src/runtime/relax_vm/executable.cc:
##########
@@ -75,6 +75,10 @@ PackedFunc Executable::GetFunction(const String& name, const ObjectPtr<Object>&
vm->LoadExecutable(GetObjectPtr<Executable>(this));
*rv = Module(vm);
});
+ } else if (name == "func_exists") {
Review Comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on a diff in pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on code in PR #15674:
URL: https://github.com/apache/tvm/pull/15674#discussion_r1316538803
##########
include/tvm/runtime/disco/session.h:
##########
@@ -236,7 +248,7 @@ class Session : public ObjectRef {
public:
/*! \brief Create a session backed by a thread pool of workers */
static Session ThreadedSession(int num_workers);
- TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
+ TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS(Session, ObjectRef, SessionObj);
Review Comment:
sounds good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jinhongyii commented on pull request #15674: [Unity][Disco] Minor change for MLC+Disco integration
Posted by "jinhongyii (via GitHub)" <gi...@apache.org>.
jinhongyii commented on PR #15674:
URL: https://github.com/apache/tvm/pull/15674#issuecomment-1707327306
cc: @junrushao @tqchen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org