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

Onnx example #13

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

Onnx example #13

wants to merge 6 commits into from

Conversation

JeroenVerstraelen
Copy link

Onnx allows users to create their models in e.g. tensorflow or pytorch and then save them to a common file format. They can then be executed in an openEO UDF without needing to provide the original libraries as dependencies.

Open-EO/openeo-geopyspark-driver#543
https://onnx.ai/

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

a couple of notes

python/OnnxUdf/example.py Outdated Show resolved Hide resolved
python/OnnxUdf/example.py Outdated Show resolved Hide resolved
python/OnnxUdf/onnx_udf.py Outdated Show resolved Hide resolved
python/OnnxUdf/example.py Show resolved Hide resolved
python/OnnxUdf/example.py Outdated Show resolved Hide resolved
@m-mohr m-mohr removed their assignment Nov 13, 2023
@m-mohr
Copy link
Member

m-mohr commented Nov 13, 2023

I don't have funding for this work, so removing myself from assignees.

import xarray as xr
from openeo.udf.debug import inspect

sys.path.insert(0, "onnx_deps")
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this path has to be prepended at the start of sys.path? Because doing this increases the risk on "frankenstein" imports (packages that are partly imported from different versions). If it works by appending at the end, I think it's better to promote that pattern.


# This will be required to import onnxruntime in the UDF.
dependencies_url = "https://artifactory.vgt.vito.be:443/auxdata-public/openeo/onnx_dependencies.zip"
# You can upload your own model and paste the url here.
Copy link
Member

Choose a reason for hiding this comment

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

This a bit confusing, I don't think you mean that users can upload to artifactory themself.

Suggested change
# You can upload your own model and paste the url here.
# Optionally, put the URL to your own model here

# This will be required to import onnxruntime in the UDF.
dependencies_url = "https://artifactory.vgt.vito.be:443/auxdata-public/openeo/onnx_dependencies.zip"
# You can upload your own model and paste the url here.
model_url = "https://artifactory.vgt.vito.be:443/auxdata-public/openeo/test_onnx_model.zip"
Copy link
Member

Choose a reason for hiding this comment

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

This is apparently a zip file with just one .onnx file. Isn't it possible to just provide this .onnx file?

@jdries
Copy link
Contributor

jdries commented Feb 2, 2024

@JeroenVerstraelen it seems like these comments are easy enough to fix, can you have a look so we can merge this pr?

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

4 participants