-
Notifications
You must be signed in to change notification settings - Fork 20
Upgrade to v2.0.0-rc.10 #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions/nits but overall looks good
|
||
/// A faster (unsafe) way of creating an Array from an Erlang binary | ||
fn initialize_from_raw_ptr<T>(ptr: *const T, shape: &[Ix]) -> ArrayViewMut<T, IxDyn> { | ||
fn initialize_from_raw_ptr<T>(ptr: *const T, shape: &[Ix]) -> ArrayViewMut<'_, T, IxDyn> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lifetime looks unnecessary?
@@ -343,7 +444,7 @@ macro_rules! concatenate { | |||
// `typ` is the actual datatype, `ort_tensor_kind` is the OrtexTensor variant | |||
($tensors:expr, $axis:expr, $typ:ty, $ort_tensor_kind:ident) => {{ | |||
type ArrayType<'a> = ArrayBase<ViewRepr<&'a $typ>, Dim<IxDynImpl>>; | |||
fn filter(tensor: &OrtexTensor) -> Option<ArrayType> { | |||
fn filter<'a>(tensor: &'a OrtexTensor) -> Option<ArrayType<'a>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lifetimes can be removed, this is elided at compile time
} => t, | ||
_ => panic!("can't decode non tensor, got {}", dtype), | ||
}; | ||
|
||
let tensor = match ty { | ||
ort::TensorElementType::Bfloat16 => { | ||
OrtexTensor::bf16(e.try_extract_tensor::<half::bf16>()?.into_owned()) | ||
ort::tensor::TensorElementType::Bfloat16 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of repetition here, may be worth putting this in a function with a generic. Either way it works fine, just a thought.
// for output_descriptor in &session.outputs { | ||
// let output_name: &str = &output_descriptor.name; | ||
// let val = outputs.get(output_name).expect( | ||
// &format!( | ||
// "Expected {} to be in the outputs, but didn't find it", | ||
// output_name | ||
// )[..], | ||
// ); | ||
|
||
// let ortextensor: OrtexTensor = val.try_into()?; | ||
// let shape = ortextensor.shape(); | ||
// let (dtype, bits) = ortextensor.dtype(); | ||
|
||
// let collected_output = (ResourceArc::new(ortextensor), shape, dtype, bits); | ||
// collected_outputs.push(collected_output); | ||
// } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to stay?
Hi!
This is my attempt to upgrade the deps and be able to use the latest Ort and ONNX version.
Hopefully my approach does not affect performance, my own tests suggest its still as fast.
Would be nice if someone with actual rust competence can verify my approaches :)
But at least it works :)