Skip to content
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

v2.0 #78

Merged
merged 57 commits into from
Nov 21, 2023
Merged

v2.0 #78

merged 57 commits into from
Nov 21, 2023

Conversation

decahedron1
Copy link
Member

@decahedron1 decahedron1 commented Aug 16, 2023

Fixes #97, #80, #70, #69, #66

TODO

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Attention: 714 lines in your changes are missing coverage. Please review.

Comparison is base (6a1c19d) 51.88% compared to head (34f8161) 37.99%.
Report is 3 commits behind head on main.

❗ Current head 34f8161 differs from pull request most recent head a4e731c. Consider uploading reports for the commit a4e731c to get more accurate results

Files Patch % Lines
src/value.rs 48.71% 100 Missing ⚠️
src/session/mod.rs 54.64% 83 Missing ⚠️
src/execution_providers/tensorrt.rs 0.00% 70 Missing ⚠️
src/lib.rs 47.11% 55 Missing ⚠️
src/execution_providers/rocm.rs 0.00% 38 Missing ⚠️
src/execution_providers/cuda.rs 0.00% 35 Missing ⚠️
src/execution_providers/qnn.rs 0.00% 35 Missing ⚠️
src/io_binding.rs 0.00% 31 Missing ⚠️
src/tensor/types.rs 27.50% 29 Missing ⚠️
src/execution_providers/cann.rs 0.00% 28 Missing ⚠️
... and 16 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #78       +/-   ##
===========================================
- Coverage   51.88%   37.99%   -13.89%     
===========================================
  Files          13       28       +15     
  Lines        1089     1479      +390     
===========================================
- Hits          565      562        -3     
- Misses        524      917      +393     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cpgddln
Copy link

cpgddln commented Aug 26, 2023

Thanks for the work on this new version.

The Session::run method returns an ort::Value<'static>. Despite the 'static lifetime, these require the Session to exist to refer to valid memory. The documentation notes:

Note that ORT-owned values will prevent the session from being dropped until all values are dropped; to prevent this behavior, extract the data with [Value::extract_tensor].

Similarly, the OrtOwnedTensor<'static> require the session to exist, and their drop method also seems to be doing some reference counting.

However, the following segfaults in the gpt example:

let outputs = session.run(inputs![&array])?;
let generated_tokens: OrtOwnedTensor<f32, _> = outputs["output1"].extract_tensor()?;
drop(outputs);
drop(session);
println!("{:?}", generated_tokens);

The array is properly displayed when removing the drop.

Either:

  • It was assumed that the C-side ORT keeps the memory alive until all Value and OrtOwnedTensor have been dropped, even if the rust Session gets dropped. This does not seem to be the case, and the output Value should be tied to the lifetime of the Session.
  • Only the Value does reference counting for the Session, in which case extract_tensor(&'a self) on Value<'static> should return a OrtOwnedTensor<'a>, not a OrtOwnedTensor<'static>

@bharrisau
Copy link
Contributor

Bit of an issue with the SessionOutputs. IoBinding::bind_input takes a Value, but SessionOutputs can only give an &Value. So not possible to use an output as a subsequent input.

Current workaround

let outs = bind.run().unwrap();
assert!(outs.len() == 1);
let out_copy = unsafe { std::ptr::read(&outs["output0"]) };
std::mem::forget(outs); // Because we copied an Arc
bind.bind_input("images", out_copy).unwrap();

@decahedron1
Copy link
Member Author

@bharrisau Thank you, just implemented DerefMut for SessionOutputs which allows you to move the Values out like so:

let output1: Value<'_> = outputs.remove("output1").unwrap();

@bharrisau
Copy link
Contributor

As I mentioned in #88 - there is a significant performance penalty from the CowArray doing an extra copy in Value::from_array. First CowArray.as_standard_layout() which either copies the array if layout is wrong, or else returns a CowRepr::View of the input array. Then CowArray.as_mut_ptr() which will result in a copy if the input is CowRepr::View.

Probably need another way of creating the Value, either taking an &mut ArcArray, or else creating from a slice+shape. With the &mut ArcArray, it looks like you could call .as_mut_ptr() which will skip the copy if the Arc is unique. Then call .clone() and save the clone until the call to run(), then drop the clone and it would be unique again. Should ensure only 1 mutable reference at a time.

bharrisau and others added 5 commits October 3, 2023 13:00
* WIP - Use ArcArray to remove copy

* Updated no-copy wip

---------

Co-authored-by: Ben Harris <[email protected]>
* ci: replace actions-rs with dtolnay/rust-toolchain (#101)

* Fixed work with parameters

* Fixed docs
Onnxruntime can be built with `--use_extensions` to include
onnxruntime-extensions inside. In this case, we need to
call `EnableOrtCustomOps` for sessions that need ops in
extensions. Also, the build.rs is updated to include
libraries for custom ops.
* Created ort-sys create

* Fixed CI
- Refactor execution providers to be separate builder pattern structs
- Improve `IoBinding` ergonomics
- Reduce allocations in hot paths
- Fix a few memory leaks
- Make the `ndarray` dependency optional

TODO:
- Make mutable `Value` methods
- Useful `IoBinding` example/test (Stable DIffusion?)
- Allow passing in pre-allocated `Value`s to `Session::run`, see C docs
@decahedron1 decahedron1 marked this pull request as draft November 11, 2023 01:45
@JewishLewish
Copy link
Contributor

JewishLewish commented Nov 17, 2023

Referencing to e694ebc Commit.

symlink works smoothly on Linux 👍
Windows users require for Developer Mode to be turned on.

@decahedron1 decahedron1 marked this pull request as ready for review November 21, 2023 03:12
@decahedron1 decahedron1 merged commit fd84626 into main Nov 21, 2023
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment