[Rust] Improve NDArray, GraphRt, and Relay bindings by mwillsey · Pull Request #6563 · apache/tvm

mwillsey · 2020-09-26T01:23:57Z

This PR packages up some varied work that @jroesch, @gussmith23, and I have been working on to improve the Rust bindings.

WIP

- Converts Conv2d attrs to use tvm::String, so that we can add Rust binding - Uses Type for checked_type in Rust bindings - Fix type key in Rust bindings - Make data field contain NDArray in Rust bindings

Add GlobalPool2DAttrs Rust binding Add ExpandDimsAttrs Rust bindings Add MaxPool2DAttrs rust bindings

imalsogreg · 2020-09-30T00:12:10Z

-            v.set_len(sz);
-        }
-        Ok(v)
+        let n = self.size() / size_of::<T>();


Could a user ever request T ~ (), or some other zero-sized item, giving a divide-by-zero error?

We probably need to constrain T anyways, @mwillsey and I were trying to move the arrays on to the new bindings but it seems like there are so many unsafe gotchas from previous bindings we are trying to work through. In general I think we should probably change NDArray to be like NDArray where T: DataType or something like that. We could probably just slap a : Sized on it for now.

Yeah i think panicking on div-by-zero would be the correct behavior. Follow up on jared, I think a goal should be to move to NDArray, because right now these conversions are all effectively transmutes.

imalsogreg · 2020-09-30T00:17:14Z

-        let shape = &mut [1, 2, 3];
+        let shape = &[1, 2, 3];
        let ctx = Context::cpu(0);
+        println!("before empty");


Stale helpers, not needed anymore?

I'm fine with println! in tests, since the output is hidden by default, but that's just me.

Yep! They don't bug me, just flagged them in case they were leftovers.

imalsogreg · 2020-09-30T00:37:25Z

+    /// assert_eq!(ndarray.shape(), shape);
    /// assert_eq!(ndarray.to_vec::<i32>().unwrap(), data);
    /// ```
    pub fn to_vec<T>(&self) -> Result<Vec<T>, NDArrayError> {


I'm confused about how this function works. If I call it as my_array.to_vec<u8>, I'll get back a vector with more elements than if I call it as my_array.to_vec::<u32>, which seems like one or the other would not correspond to a flattened version of self.

Yeah, see above. The problem is that NDArray isn't NDArray. So we trust the type of T given to us by to_vec::<T>. That makes it effectively a transmute of the buffer.

imalsogreg · 2020-09-30T01:22:13Z

 paste = "0.1"
 mashup = "0.1"
 once_cell = "^1.3.1"
+pyo3 = { version = "0.11.1", optional = true }


Nit: ^0.11.1, to pick up new non-breaking changes? Or do we want 0.11.1 specifically?

I believe that "^" is implicit, no? https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio

mwillsey · 2020-09-30T19:16:53Z

@imalsogreg @jroesch Just to break this out into a comment. Many of the remaining issues on this PR that @imalsogreg commented on stem from the fact that NDArray (and NDArrayContainer) hide the type of the underlying data. This means that typed conversions must be given the type, and we basically have no choice but to trust it.

Should we block this PR on fixing this? I'm leaning toward no.

jroesch · 2020-10-01T00:35:00Z

I think we should just add a Rust bindings stabilization tracking issue for all these remaining items and set a stabilization goal on the current API, etc at which we will impose more strict coding standards and clean up.

jroesch · 2020-10-01T00:37:27Z

We can put them here: #6604.

imalsogreg · 2020-10-01T00:38:04Z

@mwillsey 👍 for merge from me. I didn't mean to block anything, mostly I'm trying to get oriented.

jroesch · 2020-10-05T19:46:36Z

We need to update the docker images to match MxNet see #6628

* WIP WIP * Add support for loading Python packed functions * Flesh out Relay AST in Rust * More tweeks for getting functions out * Deploy Rust docs as part of build * Add some more types * Introduce NDArray 2.0 * Work on NDArray 2.0 before restoring tests * Formatting and code fixes to get it to compile * Add more Rust bindings - Converts Conv2d attrs to use tvm::String, so that we can add Rust binding - Uses Type for checked_type in Rust bindings - Fix type key in Rust bindings - Make data field contain NDArray in Rust bindings * Clean up object ptr passing. * WIP * Add debugging for NDArray and fix all test cases * Add breaking test * Dispatch some todos * Format * Fix ndarray size and len * Add BiasAddAttrs rust bindings * Add DenseAttrs rust bindings * Change to TVM string * Add more Rust bindings Add GlobalPool2DAttrs Rust binding Add ExpandDimsAttrs Rust bindings Add MaxPool2DAttrs rust bindings * Fix some test attributes * Improve the NDArray api * Fix some more ndarray stuff * Get the resnet demo kinda working * Add SoftmaxAttrs Rust bindings * Implement Hash and Eq for Relay Exprs * Add underscore to unused function * Fix broken ass resnet script * Improve some ndarray conversions * Make sure the build script runs correctly * Clean up ResNet example tremedously Expose C++ graph runtime via cleaner Rust API rewrite example. * Add ASF header * Format * Format * Format resnet rust python script * Add type files and refactor span * Format * Format * Change types from std::string to tvm::String in packed function * Add ASF header * Fix test w/ ndarray's API change * Fix array test * Fix anyhow import * Put back some anyhow stuff * Clean up * Try and fix tests/scripts/task_rust.sh * Disable ResNet for now * Turn off building of Rust docs until we update CI * Actually disable Co-authored-by: Jared Roesch <jroesch@octoml.ai> Co-authored-by: Gus Smith <guscomps@gmail.com>

jroesch and others added 30 commits September 25, 2020 18:13

WIP

8c36f88

WIP

Add support for loading Python packed functions

b0b2b59

Flesh out Relay AST in Rust

d282ef5

More tweeks for getting functions out

a80a9e4

Deploy Rust docs as part of build

5f9b521

Add some more types

3343f61

Introduce NDArray 2.0

772375f

Work on NDArray 2.0 before restoring tests

2b982c1

Formatting and code fixes to get it to compile

dbe32f1

Add more Rust bindings

f7c36ca

- Converts Conv2d attrs to use tvm::String, so that we can add Rust binding - Uses Type for checked_type in Rust bindings - Fix type key in Rust bindings - Make data field contain NDArray in Rust bindings

Clean up object ptr passing.

5363ff4

WIP

930e29e

Add debugging for NDArray and fix all test cases

d35c03c

Add breaking test

5eb46c2

Dispatch some todos

518b230

Format

ba92c43

Fix ndarray size and len

f99154d

Add BiasAddAttrs rust bindings

4830621

Add DenseAttrs rust bindings

53d8377

Change to TVM string

5062ecb

Add more Rust bindings

66fcc91

Add GlobalPool2DAttrs Rust binding Add ExpandDimsAttrs Rust bindings Add MaxPool2DAttrs rust bindings

Fix some test attributes

4519acd

Improve the NDArray api

0b922f2

Fix some more ndarray stuff

15f88fd

Get the resnet demo kinda working

a435cb8

Add SoftmaxAttrs Rust bindings

d119570

Implement Hash and Eq for Relay Exprs

f48282a

Add underscore to unused function

4979041

Fix broken ass resnet script

0ef7e3e

Improve some ndarray conversions

f01dcfc

imalsogreg reviewed Sep 30, 2020

View reviewed changes

Comment thread rust/tvm/examples/resnet/build.rs

gussmith23 and others added 4 commits September 30, 2020 07:42

Fix test w/ ndarray's API change

644b746

Fix array test

5be2063

Fix anyhow import

83eb87f

Put back some anyhow stuff

72bce31

Clean up

9e580aa

jroesch force-pushed the rust-relay-graph_rt branch from f223c50 to 9e580aa Compare October 2, 2020 20:30

Try and fix tests/scripts/task_rust.sh

778f3ba

jroesch added 3 commits October 5, 2020 12:47

Disable ResNet for now

2266ddd

Turn off building of Rust docs until we update CI

d93d134

Actually disable

422e970

jroesch approved these changes Oct 6, 2020

View reviewed changes

jroesch merged commit 277bfc8 into apache:master Oct 6, 2020

tqchen mentioned this pull request Oct 8, 2020

[Rust] Memory leak in NDArray #6559

Closed

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Conversation

mwillsey commented Sep 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwillsey commented Sep 30, 2020

Uh oh!

jroesch commented Oct 1, 2020

Uh oh!

jroesch commented Oct 1, 2020

Uh oh!

imalsogreg commented Oct 1, 2020

Uh oh!

jroesch commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants