Open
Conversation
mortont
reviewed
Aug 13, 2025
Collaborator
mortont
left a comment
mortont
left a comment
There was a problem hiding this comment.
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> { |
Collaborator
There was a problem hiding this comment.
This lifetime looks unnecessary?
Author
There was a problem hiding this comment.
This was suggested by the compiler so I implemented the suggestion.
Would you rather we explicitly define the lifetime for Ix ?
native/ortex/src/tensor.rs
Outdated
| ($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>> { |
Collaborator
There was a problem hiding this comment.
These lifetimes can be removed, this is elided at compile time
| let tensor = match ty { | ||
| ort::TensorElementType::Bfloat16 => { | ||
| OrtexTensor::bf16(e.try_extract_tensor::<half::bf16>()?.into_owned()) | ||
| ort::tensor::TensorElementType::Bfloat16 => { |
Collaborator
There was a problem hiding this comment.
Lots of repetition here, may be worth putting this in a function with a generic. Either way it works fine, just a thought.
Author
There was a problem hiding this comment.
Sounds like a good idea, but I am not that provisioned in Rust to to that I think :)
native/ortex/src/model.rs
Outdated
Comment on lines
129
to
145
| // 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); | ||
| // } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 :)