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 2021/07/29 15:02:45 UTC

[GitHub] [tvm] Lunderberg opened a new pull request #8587: [Target] Allow spaces in target attributes

Lunderberg opened a new pull request #8587:
URL: https://github.com/apache/tvm/pull/8587


   Some target parameters, such as the device_name on vulkan, have spaces in them.  This prevented round-trips between string and Target objects, which can occur in some cases (e.g. [AOT codegen](https://github.com/apache/tvm/blob/main/src/relay/backend/aot_executor_codegen.cc#L782)).
   
   - Allowed for spaces in target attributes.
   - Bugfix for target string `vulkan -from_device=0`, was missing device name from device query.


-- 
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] leandron commented on a change in pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#discussion_r679744526



##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -76,6 +76,14 @@ def test_target_string_parse():
     assert tvm.target.arm_cpu().device_name == "arm_cpu"
 
 
+def test_target_string_with_spaces():
+    target = tvm.target.Target(
+        "vulkan -device_name=Name\ of\ GPU\ with\ spaces -device_type=discrete"

Review comment:
       In tvmc we faced this challenge of having substrings within target(s) (see [relevant tests](https://github.com/apache/tvm/blob/81480287a891f07f3939b98cc57fad39e217f317/tests/python/driver/tvmc/test_tvmc_common.py#L268-L281)), one example is `llvm` arguments. The decision there was to go with nested quotes, similar to Python. Have you explored this possibility in here?
   
   That would make it look like:
   ```
   "vulkan -device_name='Name of GPU with spaces' -device_type=discrete"
   ```
   
   I feel that is looks a bit more natural to type in nested quotes rather than preceding everything with a backslash. Also, I think it would be aspirational to offer a similar experience (where possible), when using API and command line.
   
   What do you think?




-- 
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] Lunderberg commented on a change in pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#discussion_r681779344



##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -76,6 +76,14 @@ def test_target_string_parse():
     assert tvm.target.arm_cpu().device_name == "arm_cpu"
 
 
+def test_target_string_with_spaces():
+    target = tvm.target.Target(
+        "vulkan -device_name=Name\ of\ GPU\ with\ spaces -device_type=discrete"

Review comment:
       And updated.




-- 
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] masahi merged pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8587:
URL: https://github.com/apache/tvm/pull/8587


   


-- 
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] zxybazh commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-889439432


   > Definitely agreed, long-term it's better to go through a standard format. I played around a bit with converting the tvm `Map` to a json string, but while it wasn't too hard to get something working for my test cases, getting it fully functional and clean felt like it would take longer than a quick edit. (And I'd probably have wanted to switch to a C++ JSON parser to avoid jumping between languages too often, which would have added its own issues.)
   
   Yes, it's more than a little work to make the Map 100% compatible to json. A c++ Json parser is ok to me but not necessary if there is nothing the current parser cannot do -- just to save time and as least the python parser is guaranteed to be correct right : )
   
   > Yeah, the compatibility was what I was most concerned about. I wanted to update the `.str()` method itself, rather than introducing a new function such as `.json()`, because that's what gets called elsewhere. The updated parser/serializer should maintain the exact same string output for attribute keys without embedded spaces, but "should" is always a bit of a dangerous word.
   
   I think the `export` function is acting as `.json()` right now. For `str` it would be a large project because it's used in a lot of places in key components. I think it's worth more discussion before we make it happen : ) But definitely direction to go in the future.
   
   


-- 
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] masahi edited a comment on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892284770


   @Lunderberg ~~On AMD I got a different error with this PR~~: (UPDATE) The error below is introduced in https://github.com/apache/tvm/pull/8602
   ```
     4: tvm::TargetInternal::ConstructorDispatcher(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*)
     3: tvm::Target::Target(tvm::runtime::String const&)
     2: tvm::TargetInternal::FromString(tvm::runtime::String const&)
     1: tvm::TargetInternal::FromRawString(tvm::runtime::String const&)
     0: tvm::TargetInternal::FromConfig(std::unordered_map<tvm::runtime::String, tvm::runtime::ObjectRef, std::hash<tvm::runtime::String>, std::equal_to<tvm::runtime::String>, std::allocator<std::pair<tvm::runtime::String const, tvm::runtime::ObjectRef> > >)
     File "/home/masa/projects/dev/tvm/src/target/target.cc", line 469
   TVMError: ValueError[10:18:05] /home/masa/projects/dev/tvm/src/target/target.cc:788: Expected runtime.String parameter for attribute 'device_name', but received TVMArgTypeCode(8) from device api
   . Target creation from string failed: vulkan -from_device=0
   ```


-- 
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] Lunderberg commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892672628


   Thank you.  I can reproduce the issue, and will have a PR shortly to resolve it.  I had tested the new behavior during development, but it looks like the final version was broken on intel CPUs as well.


-- 
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] masahi edited a comment on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892284770


   @Lunderberg ~~On AMD I got a different error with this PR~~: (UPDATE) The error below is introduced in https://github.com/apache/tvm/pull/8602. With 8602 reverted and applying this PR, the original error I saw was gone.
   ```
     4: tvm::TargetInternal::ConstructorDispatcher(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*)
     3: tvm::Target::Target(tvm::runtime::String const&)
     2: tvm::TargetInternal::FromString(tvm::runtime::String const&)
     1: tvm::TargetInternal::FromRawString(tvm::runtime::String const&)
     0: tvm::TargetInternal::FromConfig(std::unordered_map<tvm::runtime::String, tvm::runtime::ObjectRef, std::hash<tvm::runtime::String>, std::equal_to<tvm::runtime::String>, std::allocator<std::pair<tvm::runtime::String const, tvm::runtime::ObjectRef> > >)
     File "/home/masa/projects/dev/tvm/src/target/target.cc", line 469
   TVMError: ValueError[10:18:05] /home/masa/projects/dev/tvm/src/target/target.cc:788: Expected runtime.String parameter for attribute 'device_name', but received TVMArgTypeCode(8) from device api
   . Target creation from string failed: vulkan -from_device=0
   ```


-- 
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] Lunderberg commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892672628


   Thank you.  I can reproduce the issue, and will have a PR shortly to resolve it.  I had tested the new behavior during development, but it looks like the final version was broken on intel CPUs as well.


-- 
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] masahi commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892284770


   @Lunderberg On AMD I got a different error with this PR: 
   ```
     4: tvm::TargetInternal::ConstructorDispatcher(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*)
     3: tvm::Target::Target(tvm::runtime::String const&)
     2: tvm::TargetInternal::FromString(tvm::runtime::String const&)
     1: tvm::TargetInternal::FromRawString(tvm::runtime::String const&)
     0: tvm::TargetInternal::FromConfig(std::unordered_map<tvm::runtime::String, tvm::runtime::ObjectRef, std::hash<tvm::runtime::String>, std::equal_to<tvm::runtime::String>, std::allocator<std::pair<tvm::runtime::String const, tvm::runtime::ObjectRef> > >)
     File "/home/masa/projects/dev/tvm/src/target/target.cc", line 469
   TVMError: ValueError[10:18:05] /home/masa/projects/dev/tvm/src/target/target.cc:788: Expected runtime.String parameter for attribute 'device_name', but received TVMArgTypeCode(8) from device api
   . Target creation from string failed: vulkan -from_device=0
   ```


-- 
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] Lunderberg commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-889226324


   @zxybazh Long-term, I'd like to switch to using JSON exclusively for `target -> str` conversions, as we had discussed, with the existing argument parsing only used for `str -> target` conversions.  For now, since the target string is saved in autotvm's logs and I'm not sure how extensively it is used, extending the argument parsing to escape strings, in order to avoid potential breakage.


-- 
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] Lunderberg commented on a change in pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#discussion_r680022546



##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -76,6 +76,14 @@ def test_target_string_parse():
     assert tvm.target.arm_cpu().device_name == "arm_cpu"
 
 
+def test_target_string_with_spaces():
+    target = tvm.target.Target(
+        "vulkan -device_name=Name\ of\ GPU\ with\ spaces -device_type=discrete"

Review comment:
       I'd be down for either, and agree that the nested quotes are an easier way to read it.  Consistency with how tvmc handles spaces is also something that I want to maintain, so I'll update this PR to use the same convention.




-- 
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] masahi commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-892316422


   Thanks @Lunderberg, please take a look at the issue of https://github.com/apache/tvm/pull/8602 above.


-- 
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] zxybazh commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-889355628


   Thanks for the contribution!
   
   Json-style target string parsing is supported as the canonical way for target creation and json export is supported using the `export` function (though right now the export result is not pure json, it's actually a tvm Map object). In the long term, I am in favor of using json exclusively for `target -> str` conversion.
   
   Meanwhile, IIRC we are using the current `str` function (`target->str`) for compatibility with AutoTVM and Ansor (AutoScheduling). IMHO, If we want to extend the argument parsing to escape strings, I would suggest that we keep the `str` function the same, and work on parsing and `export` function first (with compatibility for `str` function output). Other than that, I'm not aware of any other potential breakage. I think Junru might add to that.
   
   cc @junrushao1994 .


-- 
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] Lunderberg commented on pull request #8587: [Target] Allow spaces in target attributes

Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8587:
URL: https://github.com/apache/tvm/pull/8587#issuecomment-889396661


   > Json-style target string parsing is supported as the canonical way for target creation and json export is supported using the `export` function (though right now the export result is not pure json, it's actually a tvm Map object). In the long term, I am in favor of using json exclusively for `target -> str` conversion.
   
   Definitely agreed, long-term it's better to go through a standard format.  I played around a bit with converting the tvm `Map` to a json string, but while it wasn't too hard to get something working for my test cases, getting it fully functional and clean felt like it would take longer than a quick edit.  (And I'd probably have wanted to switch to a C++ JSON parser to avoid jumping between languages too often, which would have added its own issues.)
    
   > Meanwhile, IIRC we are using the current `str` function (`target->str`) for compatibility with AutoTVM and Ansor (AutoScheduling). IMHO, If we want to extend the argument parsing to escape strings, I would suggest that we keep the `str` function the same, and work on parsing and `export` function first (with compatibility for `str` function output). Other than that, I'm not aware of any other potential breakage. I think Junru might add to that.
   
   Yeah, the compatibility was what I was most concerned about.  I wanted to update the `.str()` method itself, rather than introducing a new function such as `.json()`, because that's what gets called elsewhere.  The updated parser/serializer should maintain the exact same string output for attribute keys without embedded spaces, but "should" is always a bit of a dangerous word.
   


-- 
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