You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/06/16 07:55:22 UTC
[GitHub] [incubator-tvm] t-vi opened a new pull request #5822: fix relay.build to not change the module argument in place
t-vi opened a new pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822
This fixes the observation at
https://discuss.tvm.ai/t/tvm-relay-build-modifying-its-argument/6958
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644903807
@tqchen Thank you for the hint. I applied it, but I must admit I'm not entirely certain what's going on.
Do I need to call Update on the ptr returned by CopyOnWrite or on original module? Previously I called it on the original module, but 1) I'm not sure, 2) then the compiler complains about the return of CopyOnWrite being unused.
Also, it seems that CopyOnWrite + Update is certain to do the copy, is it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644889021
Ooops let me take a closer look
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644898971
What I would do instead is to simply move `relay_module.CopyOnWrite()` inside `params.size() != 0`, which triggers copy if there is additional copy of relay module outside the function.
Note that for mods pass from python, CopyOnWrite always triggers a copy, unless we do `mod._move()` (moves the python side of the ref)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-645015404
Thanks @t-vi for the contribution ! Thanks @junrushao1994 for reviewing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644961499
@t-vi Here is what is going on: Internally, `CopyOnWrite` checks if the object (in our case, `relay_module`) is unique. If so, it does nothing; otherwise, it triggers a copy to make sure that the object is unique. Therefore, after calling `CopyOnWrite`, it makes sure that any change only happens locally, in our case, making relay.build don't change the input.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644599550
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644915095
We don't have to use the ptr of CopyOnWrite in this case, the module also get updated to points to the new copy .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-645015404
Thanks @t-vifor the contribution ! Thanks @junrushao1994 for reviewing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on a change in pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on a change in pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#discussion_r440716367
##########
File path: src/relay/backend/build_module.cc
##########
@@ -244,6 +244,9 @@ class RelayBuildModule : public runtime::ModuleNode {
GlobalVar main_glb_var = relay_module->GetGlobalVar("main");
Function main_func = Downcast<Function>(relay_module->Lookup(main_glb_var));
auto new_main = BindParamsByName(main_func, params);
+ // copy module to avoid changing our input
+ relay_module = IRModule(relay_module->functions, relay_module->type_definitions,
Review comment:
OK, so I changed it to call `CopyOnWrite` and it appears to work.
To understand this better, I got the re-instatiation from `FunctionPassNode::operator()` here:
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L123
is there a reason that isn't done via `CopyOnWrite`?
Superficially, it seems that the update is then
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L135
which would seem very similar to the `Update` done in the `Optimize`
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/backend/build_module.cc#L247
which itself just calls add
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/ir/module.cc#L271-L273
Is something that could also use `CopyOnWrite` or is it not applicable there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on a change in pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on a change in pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#discussion_r440716367
##########
File path: src/relay/backend/build_module.cc
##########
@@ -244,6 +244,9 @@ class RelayBuildModule : public runtime::ModuleNode {
GlobalVar main_glb_var = relay_module->GetGlobalVar("main");
Function main_func = Downcast<Function>(relay_module->Lookup(main_glb_var));
auto new_main = BindParamsByName(main_func, params);
+ // copy module to avoid changing our input
+ relay_module = IRModule(relay_module->functions, relay_module->type_definitions,
Review comment:
Thank you for the feedback!
OK, so I changed it to call `CopyOnWrite` and it appears to work.
To understand this better, I got the re-instatiation from `FunctionPassNode::operator()` here:
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L123
is there a reason that isn't done via `CopyOnWrite`?
Superficially, it seems that the update is then
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L135
which would seem very similar to the `Update` done in the `Optimize`
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/backend/build_module.cc#L247
which itself just calls add
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/ir/module.cc#L271-L273
Is something that could also use `CopyOnWrite` or is it not applicable there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644771969
Now it's not working anymore. :( I think I need a hint.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#discussion_r440679465
##########
File path: src/relay/backend/build_module.cc
##########
@@ -244,6 +244,9 @@ class RelayBuildModule : public runtime::ModuleNode {
GlobalVar main_glb_var = relay_module->GetGlobalVar("main");
Function main_func = Downcast<Function>(relay_module->Lookup(main_glb_var));
auto new_main = BindParamsByName(main_func, params);
+ // copy module to avoid changing our input
+ relay_module = IRModule(relay_module->functions, relay_module->type_definitions,
Review comment:
If I understand the issue correctly, I think we need to call CopyOnWrite instead.
IRModule is actually defined with `CopyOnWrite` method [here](https://github.com/apache/incubator-tvm/blob/master/include/tvm/ir/module.h#L332), the macro expands to [this](https://github.com/apache/incubator-tvm/blob/master/include/tvm/runtime/object.h#L755-L763). What we need to do is just call its `CopyOnWrite` method to generate a unique copy of IRModule.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#issuecomment-644999578
Thanks for the explanation, I'll have to read the source a bit more, but now I can do so with a high-level sense of what's going on. :slightly_smiling_face:
Happily the CI likes it better now, too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] t-vi commented on a change in pull request #5822: fix relay.build to not change the module argument in place
Posted by GitBox <gi...@apache.org>.
t-vi commented on a change in pull request #5822:
URL: https://github.com/apache/incubator-tvm/pull/5822#discussion_r440792289
##########
File path: src/relay/backend/build_module.cc
##########
@@ -244,6 +244,9 @@ class RelayBuildModule : public runtime::ModuleNode {
GlobalVar main_glb_var = relay_module->GetGlobalVar("main");
Function main_func = Downcast<Function>(relay_module->Lookup(main_glb_var));
auto new_main = BindParamsByName(main_func, params);
+ // copy module to avoid changing our input
+ relay_module = IRModule(relay_module->functions, relay_module->type_definitions,
Review comment:
I must admit I'm not understanding this very well yet, in particular in for the use of the new `relay_module_ptr` I get from `CopyOnWrite` and the original module.
Which one would I use for what after CopyOnWrite?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org