[Rust] Unify types between bindings and pure Rust impl #2616

nhynes · 2019-02-18T01:40:06Z

This PR unifies the types (currently) common to runtime and frontend:

share c_runtime_api.rs (removed need for tvm-sys)
TVM(Arg|Ret)?Value
TVMType, TVMContext

Still TODO (but low priority since they're not shared):

PackedFunc
TVMContext
TVMByteArray

cc @ehsanmok @kazimuth for review

ehsanmok · 2019-02-18T02:31:44Z

@nhynes Thanks a lot for PR! I'll start the review as soon as I can.

When the number of changes is a lot, could you rebase all of your commits in one commit to see all the changes at once?

Another thing in here you said for runtime

bindgen doesn't work when the target is exotic (e.g., wasm or sgx)

now you're using it? (that was the reason I kept tvm-sys though)

nhynes · 2019-02-18T04:07:07Z

number of changes is a lot, could you rebase all of your commits in one commit

The github PR diff shows all of the changes at once. Squashing commits before merging defeats the purpose of creating logically atomic changesets.

bindgen

Right. The simple solution is to check in c_runtime_api.rs. I'll see if bindgen works yet with non-host targets.

ehsanmok

Great set of changes contributing to more readability for idiomatic Rust :)

Please add a README to runtime and show some snippets from tests and the overall introductions to runtime. For better documentations of runtime, consider adding a sentence at the module level as well.

rust/frontend/examples/resnet/src/main.rs

rust/frontend/src/function.rs

rust/common/src/packed_func.rs

ehsanmok · 2019-02-18T19:48:38Z

rust/common/src/packed_func.rs

+/// assert_eq!(String::try_from(t).unwrap(), s);
+/// ```
+pub struct TVMRetValue {
+    pub value: TVMValue,


You've previously used prim_val: i64 while I used TVMValue and you were confident that I should change it to i64 but now you're back to TVMValue for POD representations! I still think usize should fit.

this is temporary. TVMValue is going to become a proper enum with conversions between ffi::TVMValue. The box_value is a hack to fix the unsafety around ffi::TVMByteArray conversions.

Why adding temporary here? you mean it's in your todo for this PR?

Temporary in terms of the next two rust PRs. I want @kazimuth to be able to merge his PR, but I agree that committing bad code is a cardinal sin. I'd have to ask him whether he's willing to wait for another round of commits which truly bury the TVM ffi types.

Let's keep things right as you've previously wanted from me (in TVMRetValue POD val frontend), lay the groundwork and later if he's interested he can add the GraphModule on top of it after. We're not in rush.

We're not in rush.

🙏 thank you for your understanding

What's the status of this? it's not resolved yet and you're using TVMValue so no POD discussion!

This will be addressed in a future PR. This one was supposed to unify the types, which it did. Delaying merging this one just blocks further contribution from others.

This has exactly happened when you requested me to include it in frontend PR. This sort of double standard is not good.

Well, I personally have no issue with sitting on this PR for another few months and get things really polished. If you want to add some commits, you're more than welcome to. I'll defer to @tqchen about timelines.

rust/frontend/src/lib.rs

tests/scripts/task_rust.sh

kazimuth · 2019-02-18T21:29:19Z

I don't know this codebase that well, but I'm happy to look over this.

One question I'm still not sure about is: what's the relationship between NDArrayContainer, ArrayHandle, DLTensor, NDArray, and ndarray::Array? I remember having issues where returning NDArrayContainers from a Function caused panics, is there a test for that somewhere?

Also, do you intend to unify Function and PackedFunc? I'm still not certain if they do the same thing or not.

Please don't worry about accommodating the changes in my PR, I'm happy to rewrite it over whatever the API ends up looking like.

ehsanmok · 2019-02-18T22:23:28Z

@kazimuth appreciate your upcoming contributions in advance!

Great questions :)

what's the relationship between ...

Everything starts from dlpack's DLTensor as a common tensor/ndarray format for DL frameworks (adopted by Apache MXNet and Pytorch).
TVM uses the type alias TVMArray for DLTensor.
TVMArrayHandle is an opaque pointer to TVMArray.
In frontends (Rust/Python/Java) they define NDArray wrapper (struct/class) around TVMArrayHandle with more info such as is_view.
Rust has the luxury of defining its own ndarray/tensor in runtime.
There's Rust's crate ndarray which we use here to interact with our own Tensor/NDArray. Similar to numpy.ndarray vs. mxnet.NDArray/pytorch.Tensor.
NDArrayContainer is runtime managed NDArray and it's not supported in Rust frontend Function for now. See #1418 for more info.

Also, do you intend to unify Function and PackedFunc? I'm still not certain if they do the same thing or not.

Rust frontend Function (like NDArray) contains a TVMFunctionHandle of C++ PackedFunc. Rust frontend optimum representation of C++ PackedFunc is fn(&[TVMArgValue]) -> Result<TVMRetValue> (which you can use in register_global_func! macro to register Rust's packed function as a callback to C++ backend) because first fn is a (safe) primitive function pointer type constructor and is already Fn + Pointer + Send + Sync + ... and second it captures the Result. Runtime representation is Box<Fn(&[TVMArgValue]) -> TVMRetValue + Send + Sync> as direct translation of std::function<void (TVMArgs args, TVMRetValue* rv)>;. It's in this PR TODO to change it so that function can capture Result.

jroesch · 2019-02-20T11:22:20Z

I'll review tomorrow, been very busy lately.

nhynes · 2019-02-20T17:25:00Z

I'll review tomorrow, been very busy lately.

@jroesch no rush. I'll take another pass later this week to iron out those TODOs in the PR body.

Ichange runtime packed function representation to the optimum one.

Just pushed a fix to that (3d10c6b). Note that the fat pointer is required if you want to make a PackedFunc out of a closure. Otherwise, it's probably sufficient to from_raw a C function pointer so to prevent allocation. It shouldn't matter w.r.t. the UI since Box<T> is Deref<T>

ehsanmok · 2019-02-20T17:47:29Z

Note that the fat pointer is required if you want to make a PackedFunc out of a closure.

Right! Frontend PackedFunc is not a closure and don't need to be. But for runtime, since you're directly translating std::function<void (TVMArgs args, TVMRetValue* rv)>; you can argue that boxing is necessary and so be it.

ehsanmok · 2019-03-07T00:50:12Z

@nhynes Any update on this PR?

tqchen · 2019-03-12T01:39:29Z

ping @nhynes might be a good idea to push some of this

nhynes · 2019-03-12T03:18:20Z

Sorry about that. This was definitely a bit much to bite off for one PR :) I'll get this in by the end of the week.

nhynes · 2019-03-23T10:37:14Z

I'll have an update for this PR soon. Right now I'm just working through a bug where a Rust RetVal in a callback is being dropped because TVM CFuncSetReturn doesn't take ownership.

nhynes · 2019-03-23T19:22:38Z

ok, once the tests pass, this PR will be ready to go. It accomplishes the goal of unifying the types, but it doesn't do things like

making a Rusty TVMPodValue
a proc macro for register_global
all the other TODOs :)

nhynes · 2019-03-23T23:56:03Z

we currently have a bit of a catch-22: the build is failing because we need a new version of rustfmt, but we can't upgrade rustfmt because rust-sgx-sdk depends on a particular nightly version. I'm about to do a rewrite of TVM SGX using the Fortanix EDP, so I'm in favor of breaking the current SGX, merging this, and then dropping in new SGX. Thoughts?

nhynes · 2019-03-29T18:18:20Z

@tqchen @jroesch could you please review this PR or suggest reviewers? (pending addressing comments) This PR is ready to go.

ehsanmok · 2019-03-29T18:24:47Z

rust/common/src/packed_func.rs

+/// assert_eq!(String::try_from(t).unwrap(), s);
+/// ```
+pub struct TVMRetValue {
+    pub value: TVMValue,


What's the status of this? it's not resolved yet and you're using TVMValue so no POD discussion!

ehsanmok · 2019-03-29T18:26:26Z

rust/frontend/examples/resnet/src/main.rs

@@ -85,14 +88,19 @@ fn main() {
        .get_function("set_input", false)
        .unwrap();

-    call_packed!(set_input_fn, "data", &input).unwrap();
+    let data_str = std::ffi::CString::new("data").unwrap();


This should be hidden from the user.

ehsanmok · 2019-03-29T19:55:21Z

rust/frontend/src/function.rs

-                &mut ret_type_code as *mut _
-            ));
-        }
+    pub fn invoke(&mut self) -> Result<TVMRetValue, Error> {


Since you removed FnOnce impl, is there any other place needing unboxed_closure feature? if none, please remove it from frontend.

tqchen · 2019-03-31T17:11:14Z

@ehsanmok @nhynes would be great if you can agree on a plan for moving forward.

From what I see, there is a TVMRetValue debate about what the signature should be which could merit a discussion. If we view this PR as a refactor and the code is strictly better than the old version, we can merge this in. If the TVMRetValue is a new feature and given that the public data structure affects ABI, it is good to polish and make it better.

In this case, I think the main disagreement comes from how to treat the setting. If it takes a while to settle down on the TVMRetValue struct and the code is a strict improvement, I think we can consider merging it. If we can settle down the design of TVMRetValue in a few days, then perhaps it is better to pin that down first.

Either way, let us have a healthy discussion and make suggestions on how to move forward

nhynes · 2019-03-31T18:21:07Z

Thanks for weighing in @tqchen.

In my mind, the discussion about TVMRetValue pertains only to its implementation details. @ehsanmok and I are in agreement that thinly wrapping the TVMValue union (taken directly from the FFI) is not a Rusty solution and we'd both like to see it turned into a Rust enum (still a tagged union, but more idiomatic). However, the API provided by TVMRetValue--namely From and Into--is stable. It's more of a question of "do we bundle more changes into this PR or make more PRs." This one accomplishes its original goal of having the frontend and runtime use the same types.

I can emphasize with Ehsan, though. In his original PR, I was fussy about the same issue and asked him to change it to the previous repr (now the proposed repr). I do apologize for this, but unfortunately my precognition isn't what it used to be, and I didn't know what repr would make the types easier to unify. All I can do is promise that I (or someone else) will eventually make a PR (perhaps even this one) that addresses the issue. After all, who doesn't want a better Rust TVM? :)

@ehsanmok if you feel strongly about enum-ifying TVMRetValue, I will gladly incorporate the changes into this PR, but it'll take a while since I also want to turn register_global into a proc macro attribute ;)

ehsanmok · 2019-04-02T02:13:11Z

@tqchen This is fine for now! I don't have any other comments to make for this PR. @nhynes has explained what each of us is thinking about the final Rusty repr (at least for this PR), but since it's rather fundamental, I'd really love to decide on a solid one as soon as we can after this is merged. Nick, please feel free to propose and update your plans in this regard.

We, as a community, will continue helping each other professionally in any way we can :)

tqchen · 2019-04-02T05:18:01Z

OK @ehsanmok, please https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

ehsanmok · 2019-04-02T05:52:28Z

@tqchen Done.

tqchen · 2019-04-03T00:24:35Z

Thanks, @ehsanmok @nhynes , this is now merged

nhynes requested review from jroesch and tqchen February 18, 2019 01:40

ehsanmok reviewed Feb 18, 2019

View reviewed changes

nhynes mentioned this pull request Feb 19, 2019

Add TVM to testing dockerfile oasisprotocol/oasis-core#1480

Closed

nhynes force-pushed the rust-next branch from 6b07ad2 to 5185056 Compare February 25, 2019 08:53

nhynes force-pushed the rust-next branch from 82859e8 to c431743 Compare March 23, 2019 19:18

This was referenced Mar 24, 2019

[SGX] Use Fortanix EDP #2885

Closed

[Rust] Use version invariant rustfmt #2886

Merged

nhynes and others added 9 commits March 29, 2019 16:24

Begin unify Rust lib

1f38ac7

Update runtime

36bc545

Merge types for frontend

c57bfdc

Fix tests

42645d4

PackedFunc returns result

2520796

Remove unused imports

05f1ae3

Use failure in tvm_common

2f57770

Use failure in tvm_runtime

909b3ba

Use optimal PackedFunc

b6ed736

nhynes added 7 commits March 29, 2019 16:24

Add GetLastError to runtime

5da3ca3

Fix lints in runtime tests

022a144

box_syntax

97660df

Fix warnings

c882576

Use failure in frontend

89750eb

Fix SGX build

3234342

Update resnet example

506a920

nhynes force-pushed the rust-next branch from 2014a62 to 506a920 Compare March 29, 2019 16:25

ehsanmok suggested changes Mar 29, 2019

View reviewed changes

Address review comments

9a02eae

ehsanmok approved these changes Apr 2, 2019

View reviewed changes

tqchen merged commit 4968279 into apache:master Apr 3, 2019

ajtulloch pushed a commit to ajtulloch/tvm that referenced this pull request Apr 5, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

c7e4074

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 7, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

1ce512a

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 7, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

b79bb73

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 8, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

8dadf60

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 10, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

746ac1b

wweic pushed a commit to neo-ai/tvm that referenced this pull request Apr 11, 2019

[Rust] Unify types between bindings and pure Rust impl (apache#2616)

d73a9f3

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust] Unify types between bindings and pure Rust impl #2616

[Rust] Unify types between bindings and pure Rust impl #2616

nhynes commented Feb 18, 2019 •

edited

Loading

ehsanmok commented Feb 18, 2019 •

edited

Loading

nhynes commented Feb 18, 2019

ehsanmok left a comment

ehsanmok Feb 18, 2019

nhynes Feb 18, 2019

ehsanmok Feb 18, 2019

nhynes Feb 18, 2019

ehsanmok Feb 18, 2019

nhynes Feb 18, 2019

ehsanmok Mar 29, 2019 •

edited

Loading

nhynes Mar 29, 2019 •

edited

Loading

ehsanmok Mar 29, 2019

nhynes Mar 29, 2019

kazimuth commented Feb 18, 2019

ehsanmok commented Feb 18, 2019 •

edited

Loading

jroesch commented Feb 20, 2019

nhynes commented Feb 20, 2019 •

edited

Loading

ehsanmok commented Feb 20, 2019 •

edited

Loading

ehsanmok commented Mar 7, 2019

tqchen commented Mar 12, 2019

nhynes commented Mar 12, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 29, 2019

ehsanmok Mar 29, 2019 •

edited

Loading

ehsanmok Mar 29, 2019

ehsanmok Mar 29, 2019

tqchen commented Mar 31, 2019

nhynes commented Mar 31, 2019

ehsanmok commented Apr 2, 2019 •

edited

Loading

tqchen commented Apr 2, 2019

ehsanmok commented Apr 2, 2019

tqchen commented Apr 3, 2019

[Rust] Unify types between bindings and pure Rust impl #2616

[Rust] Unify types between bindings and pure Rust impl #2616

Conversation

nhynes commented Feb 18, 2019 • edited Loading

ehsanmok commented Feb 18, 2019 • edited Loading

nhynes commented Feb 18, 2019

ehsanmok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehsanmok Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

nhynes Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazimuth commented Feb 18, 2019

ehsanmok commented Feb 18, 2019 • edited Loading

jroesch commented Feb 20, 2019

nhynes commented Feb 20, 2019 • edited Loading

ehsanmok commented Feb 20, 2019 • edited Loading

ehsanmok commented Mar 7, 2019

tqchen commented Mar 12, 2019

nhynes commented Mar 12, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 23, 2019

nhynes commented Mar 29, 2019

ehsanmok Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Mar 31, 2019

nhynes commented Mar 31, 2019

ehsanmok commented Apr 2, 2019 • edited Loading

tqchen commented Apr 2, 2019

ehsanmok commented Apr 2, 2019

tqchen commented Apr 3, 2019

nhynes commented Feb 18, 2019 •

edited

Loading

ehsanmok commented Feb 18, 2019 •

edited

Loading

ehsanmok Mar 29, 2019 •

edited

Loading

nhynes Mar 29, 2019 •

edited

Loading

ehsanmok commented Feb 18, 2019 •

edited

Loading

nhynes commented Feb 20, 2019 •

edited

Loading

ehsanmok commented Feb 20, 2019 •

edited

Loading

ehsanmok Mar 29, 2019 •

edited

Loading

ehsanmok commented Apr 2, 2019 •

edited

Loading