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

Modify RunONNXModel.py for pip install #2857

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

chentong319
Copy link
Collaborator

@chentong319 chentong319 commented Jun 27, 2024

The goal is to create a python package so that we can run an inference like onnxruntime in python script. Since the RunONNXModel.py provides most of the necessary functionality, I added extra support in that script. I tested with a local pip install. We can upload the package later.

Major changes:

  1. The code for package, onnxmlir, is added in utils.
  2. The main code is modified from utils/RunONNXModel.py. Therefore, we can run inference in onnxruntime way, or our RunONNXModel.py way.
  3. Related document is added in docs.
  4. Details of the package can be found at utils/onnxmlir/README.md, and example can be found in utils/onnxmlir/tests

Example:

import numpy as np
import onnxmlirrun

a = np.zeros((3,4,5), dtype=np.float32)
b = a + 4
r = onnxmlirrun.RunONNXModel.onnxmlirrun(compiled_so="/Users/chentong/Projects/onnx-mlir/build/test_add.so", inputs=[a,b])
print(r)
r = onnxmlirrun.RunONNXModel.onnxmlirrun(onnx_model="/Users/chentong/Projects/onnx-mlir/build/test_add.onnx", inputs=[a,b])
print(r)

We may have different name for the functions and the package. Comments are welcome.

@AlexandreEichenberger
Copy link
Collaborator

@chentong319 Can you add a reference to the ORT interface that this PR is imitating.

In your first example

r = onnxmlirrun.RunONNXModel.onnxmlirrun(compiled_so="/Users/chentong/Projects/onnx-mlir/build/test_add.so", inputs=[a,b])

was the onnx_model parameter omitted on purpose? Just for me to understand better the interface.

@chentong319
Copy link
Collaborator Author

@chentong319 Can you add a reference to the ORT interface that this PR is imitating.

In your first example

r = onnxmlirrun.RunONNXModel.onnxmlirrun(compiled_so="/Users/chentong/Projects/onnx-mlir/build/test_add.so", inputs=[a,b])

was the onnx_model parameter omitted on purpose? Just for me to understand better the interface.

Yes, onnx_model is not needed if the compiled .so is provided. By the way, the two onnxmilrrun.RunONNXModel.onnxmlirrun commands are two independent ways to do inference.

@AlexandreEichenberger
Copy link
Collaborator

Got it, so we can provide a onnx file or a pre-compiled binary, smart, thanks.

if onnx_model :
args.model = onnx_model
if compiled_so :
args.load_so = compiled_so
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if both onnx_model and compiled_so are not given?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, and maybe if none are given, an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the check whether both onnx_mdel and compiled_so are given or not give (XOR).

@tungld
Copy link
Collaborator

tungld commented Jul 1, 2024

onnxmlirrun.RunONNXModel.onnxmlirrun

My two cents here. For the package name (the first onnxmlirrun), just onnxmlir is enough to avoid typing two Rs.
I would remove RunONNXModel, but it seems not straightforward unless we reorganize RunONNXModel.py.

Anyway, we have several python utilities, perhaps it's time to reorganize them into a single python package.

@chentong319
Copy link
Collaborator Author

I changed the init.py for the package to simplify onnxmlirrun.RunONNXModel.onnxmlirrun to onnxmlir.onnxmlir.
I am thinking to add the code for package into onnx-mlir in future.

@AlexandreEichenberger
Copy link
Collaborator

AlexandreEichenberger commented Jul 18, 2024

Can you copy/link a description of the ORT runtime interface and update ours with the new names, so that we can see how they relate?

Is it something on this page? https://onnxruntime.ai/docs/get-started/with-python.html

@chentong319
Copy link
Collaborator Author

The Ort code is like

import onnxruntime as ort
import numpy as np
x, y = test_data[0][0], test_data[0][1]
ort_sess = ort.InferenceSession('fashion_mnist_model.onnx')
outputs = ort_sess.run(None, {'input': x.numpy()})

Since the current RunModelPython does not separate the session creation and run, our code will be like:

import onnxmlir
import numpy as np
x, y = test_data[0][0], test_data[0][1]
outputs = onnxmlir.run(onnx_model='fashion_mnist_model.onnx', inputs = [a, b])

It is doable to totally mimic ORT in separating the session creation and run, and using dictionary for input.

@AlexandreEichenberger
Copy link
Collaborator

Ideally

import onnxmlir as ort
import numpy as np
x, y = test_data[0][0], test_data[0][1]
ort_sess = ort.InferenceSession('fashion_mnist_model.onnx')
outputs = ort_sess.run(None, {'input': x.numpy()})

with additional parameters ok

import onnxruntime as ort
import numpy as np
x, y = test_data[0][0], test_data[0][1]
ort_sess = ort.InferenceSession('fashion_mnist_model.onnx', compile_flags="-O3")
outputs = ort_sess.run(None, {'input': x.numpy()})

reusing flags that already exists, if any.



def inferenceSession(model_name, **kwargs):
global args
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this code into the __init__ of InferenceSession class and remove inferenceSession method? The reason is it is a bit confusing about InferenceSession class and inferenceSession method. Other parts looks good to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks for the suggestion.

tungld and others added 2 commits August 6, 2024 16:40
@@ -0,0 +1 @@
1.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version number here is different than onnx-mlir/VERSION_NUMBER. Is that on purpose?
Also the number here is different than the version listed in the file below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I used a version number different from the onnx-mlir. The version for onnxmlir is for upstream onnxmlir to pip repo. It could be viewed as a pre-required package.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest a 0.1.0 and move to a 1.0.0 once we have the installation of the compiler with it at least one z?

@@ -0,0 +1,959 @@
#!/usr/bin/env python3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an identical copy? If so, at a minimum there should be a comment stating that they should stay in sync. Or maybe we could have a single version and we could "build" the pip structure by copying files in the right place. That would be my preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a target in CMake to check the files.

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check error message if file does not exist.

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: let's try to have parallel default and/or as set in onnxruntime.

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: sync also with #2899 with respect to catching errors/crasshes

Signed-off-by: chentong319 <[email protected]>
@chentong319
Copy link
Collaborator Author

Check error message if file does not exist.

onnx-mlir will report the error if file does not exist. Leave this to the TODO list to catch all the errors gracefully.

@@ -0,0 +1 @@
1.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest a 0.1.0 and move to a 1.0.0 once we have the installation of the compiler with it at least one z?

build-backend = "hatchling.build"
[project]
name = "onnxmlir"
version = "1.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having it in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants