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/07/22 20:40:52 UTC

[GitHub] [incubator-tvm] binarybana opened a new pull request #6116: Some rust cleanups

binarybana opened a new pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116


   * Turn off default features for bindgen
   * Upgrade some deps for smaller total dep tree
   * Switch (/complete switch) to thiserror
   * Remove unnecessary transmutes
   
   
   Thanks for contributing to TVM!   Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.


----------------------------------------------------------------
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] binarybana commented on a change in pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
binarybana commented on a change in pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#discussion_r459134821



##########
File path: rust/tvm-sys/Cargo.toml
##########
@@ -32,4 +32,4 @@ ndarray = "0.12"
 enumn = "^0.1"
 
 [build-dependencies]
-bindgen = "0.51"
+bindgen = { version="0.51", default-features=false }

Review comment:
       The default-features of `bindgen` pulls in `clap` which has a pretty big (albeit `build-dep`) dependency load. So it'd be good to trim if 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#discussion_r459078752



##########
File path: rust/tvm-sys/Cargo.toml
##########
@@ -32,4 +32,4 @@ ndarray = "0.12"
 enumn = "^0.1"
 
 [build-dependencies]
-bindgen = "0.51"
+bindgen = { version="0.51", default-features=false }

Review comment:
       I think @kazum wanted this on for building the WASM backend, can you confirm?




----------------------------------------------------------------
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] jroesch commented on pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#issuecomment-662693206


   LGTM, @mwillsey can you review 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#discussion_r459284775



##########
File path: rust/tvm-sys/Cargo.toml
##########
@@ -32,4 +32,4 @@ ndarray = "0.12"
 enumn = "^0.1"
 
 [build-dependencies]
-bindgen = "0.51"
+bindgen = { version="0.51", default-features=false }

Review comment:
       Thanks for following up @kazum all the big changes to Rust should be done. I will be sending a bunch of doc updates in coming weeks after I finish landing new Relay error reporting. Max and I have been using the bindings at OctoML for some experiments. 




----------------------------------------------------------------
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] binarybana commented on pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
binarybana commented on pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#issuecomment-662743251


   Okay, builds and tests pass now.


----------------------------------------------------------------
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] kazum commented on a change in pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
kazum commented on a change in pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#discussion_r459278406



##########
File path: rust/tvm-sys/Cargo.toml
##########
@@ -32,4 +32,4 @@ ndarray = "0.12"
 enumn = "^0.1"
 
 [build-dependencies]
-bindgen = "0.51"
+bindgen = { version="0.51", default-features=false }

Review comment:
       It was necessary before, but now I confirm wasm32 can be compiled successfully without it.  I'm not sure why.
   
   @binarybana Could you revive a wasm32 test to make sure that this PR doesn't break wasm32 build?
   
   ```diff
   diff --git a/tests/scripts/task_rust.sh b/tests/scripts/task_rust.sh
   index 6d159f6..7cdfb1a 100755
   --- a/tests/scripts/task_rust.sh
   +++ b/tests/scripts/task_rust.sh
   @@ -71,11 +71,11 @@ cd tests/test_tvm_dso
    cargo run
    cd -
   
   -# # run wasm32 test
   -# cd tests/test_wasm32
   -# cargo build
   +# run wasm32 test
   +cd tests/test_wasm32
   +cargo build
    # wasmtime $RUST_DIR/target/wasm32-wasi/debug/test-wasm32.wasm
   -# cd -
   +cd -
   
    # run nn graph test
    cd tests/test_nn
   ```




----------------------------------------------------------------
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] binarybana commented on pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
binarybana commented on pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#issuecomment-662712240


   I noticed a small build error I introduced in the last rebase. Will fix and push when I get a second, but wanted to flag it first.


----------------------------------------------------------------
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] jroesch merged pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116


   


----------------------------------------------------------------
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] jroesch commented on pull request #6116: [Rust] Some rust cleanups

Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6116:
URL: https://github.com/apache/incubator-tvm/pull/6116#issuecomment-663231660


   Thanks Jason!


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