[Rust] Fixup rust bindings #16712
[Rust] Fixup rust bindings #16712
Conversation
|
@Lunderberg @tqchen any chance you know somebody who could review? |
There was a problem hiding this comment.
I'm not too familiar with the TVM Rust bindings, but the changes look reasonable overall. I've made a couple of comments, but it would be good to get eyes from somebody more familiar with the Rust bindings as well. (@jroesch, maybe?)
| } | ||
| } | ||
|
|
||
| impl Attrs { |
There was a problem hiding this comment.
On the C++ side, most Attrs are constructed using their subclass, not as a BaseAttrsNode. Is this a difference that we want to have between the C++ and Rust bindings?
| @@ -94,9 +94,9 @@ external! { | |||
| fn _as_text(object: ObjectRef, show_meta_data: i32, annotate: runtime::Function) -> TString; | |||
| } | |||
|
|
|||
| pub fn as_text<T: IsObjectRef>(object: T) -> String { | |||
| pub fn as_text<T: IsObjectRef>(object: T, show_meta_data: i32) -> String { | |||
There was a problem hiding this comment.
- Does the new argument break existing callsites of
as_text? Since we can't add default parameters to preserve backwards compatibility, I'm wondering if this should be updated to use the builder pattern in the future. - Should the argument be
show_meta_data: boolinstead ofshow_meta_data: i32? The FFI cannot currently distinguish between the two, but exposing it asi32in the Rust bindings seems odd.
There was a problem hiding this comment.
Does the new argument break existing callsites of
as_text? Since we can't add default parameters to preserve backwards compatibility, I'm wondering if this should be updated to use the builder pattern in the future.
You mean object.with_metadata().as_text()? Unfortunately I am not sure wether backwards compatibility in this case is important enough, to warrant the increased API complexity.
| impl From<i32> for PrimExpr { | ||
| fn from(i: i32) -> PrimExpr { | ||
| IntImm::from(i).upcast() | ||
| } | ||
| } | ||
|
|
||
| impl Into<i64> for PrimExpr { |
There was a problem hiding this comment.
I don't think we should provide Into<i64> for PrimExpr, because Into should only be used for infallible conversions. Can we provide TryInto<i64> instead?
This is a rework of #16628 it does not introduce new bindings, but only adopts to some changes in the TVM API, by adding missing attribute values.
@jroesch @nhynes could you have a look.