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

Update macOS onnx to v1.17 Universal build #10

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kcoul
Copy link

@kcoul kcoul commented Mar 1, 2024

Since the Universal build is over 100MB, this PR commits a .zip file of it instead, which CMake unzips when a macOS build target is detected.

This should fix #5

@kcoul kcoul marked this pull request as draft March 1, 2024 22:02
@kcoul
Copy link
Author

kcoul commented Mar 1, 2024

This should remain as a draft until Windows is also updated to v.1.17 of ONNX, otherwise there would be a header/lib mismatch.

@faressc
Copy link
Member

faressc commented Mar 27, 2024

Both onnxruntime libs in Scyclone are universal libs already. The problem in #5 seems to be a different one. Nonetheless at some point we should update our onnxruntime version...

@kcoul
Copy link
Author

kcoul commented Mar 27, 2024

Ah I see, yes you are right, I just verified that myself. In that case it should be possible to just pick one of them and link to it for either flavour of macOS build. I am going to try working with one of them in my fork and see how I get along with it compared to the one I built myself (I think I used ort-builder as well). I will report back. I'll leave this draft PR open awhile longer in case I discover anything new.

@faressc
Copy link
Member

faressc commented Mar 27, 2024

Sounds good. Are you facing the same issues with the arm version like #5?

@kcoul
Copy link
Author

kcoul commented Mar 27, 2024

@faressc no it seems fine so far! You might find my fork interesting as I have been trying to get Scyclone working on various platforms, with good success:

  • Linux (x86_64)
  • Linux (aarch64) like Raspberry Pi - works but OpenGLShader must not be run. Overheats the Pi until it starts to throttle then output gets choppy!
  • WASM (I was using a Projucer-based Makefile approach that is error-prone, I would need to see how to do this with CMake)
  • Build against JUCE6 to target older machines, and the WASM-enabled fork of JUCE 6.1.6 above

main...kcoul:Scyclone:main

@faressc
Copy link
Member

faressc commented Mar 28, 2024

This looks interesting. Linux support is something we are considering implementing in the near future. So thanks for your input on this! But we are trying to build a static lib with the ort builder and in your fork you are linking against a dynamic onnxruntime library. We use the static libs because we try to keep Scyclone free of runtime dependencies (other than the classic JUCE dependencies).
The WASM support is also exciting. So you got Scyclone working in a browser?

@kcoul
Copy link
Author

kcoul commented Mar 28, 2024

This looks interesting. Linux support is something we are considering implementing in the near future. So thanks for your input on this! But we are trying to build a static lib with the ort builder and in your fork you are linking against a dynamic onnxruntime library. We use the static libs because we try to keep Scyclone free of dependencies (other than the classic JUCE dependencies). The WASM support is also exciting. So you got Scyclone working in a browser?

With the Linux static lib of onnxruntime issue, I ran into some missing linker symbols in the Linux static library I produced from ort-builder, that was tricky to figure out. There are some commits related to that in my fork. An .mri file must be created listing the static libs to be merge together, then run ar -M < path_to_mri_file - ort-builder repo does not currently have this yet.

013f5c8
https://github.com/olilarkin/ort-builder/blob/main/build-linux.sh

A combined static lib for Linux that merged the same libs that ort-builder does for macOS and Windows still produced the linker errors, so I gave up and switched to dynamic at that point.

With WASM, it was very close but I ran into an issue with the approach I was using (Projucer-generated Makefile for Linux modified for Emscripten). I would like to try again using CMake+Emscripten when I can find some more time!

@faressc
Copy link
Member

faressc commented Mar 31, 2024

Actually, I must be running into the same linker errors as you. I am using the script

  python onnxruntime/tools/ci_build/build.py \
  --build_dir "onnxruntime/build/linux_${ARCH}" \
  --config=MinSizeRel \
  --parallel \
  --compile_no_warning_as_error \
  --skip_submodule_sync \
  --skip_tests \
  --minimal_build \
  --disable_ml_ops --disable_rtti \
  --include_ops_by_config "$ONNX_CONFIG" \
  --enable_reduced_operator_type_support \

Then I use

ar -M << EOM                                                                               
   CREATE "output.a"
   ADDLIB "./libonnxruntime_graph.a"
   ADDLIB ...
   SAVE
   END
EOM

The linker errors I get are of the type

undefined reference to `onnx::ModelProto::ModelProto(google::protobuf::Arena*, bool)'.
undefined reference to `onnx::ModelProto::~ModelProto()'.
# and more...

Do you get similar errors?

@faressc
Copy link
Member

faressc commented Mar 31, 2024

I just figured out the problem... When I was building onnxruntime, I had automatically linked against my system-wide installation of the protobuf library. When I then linked against the combined static onnxruntime library, that part of the library was missing...

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.

Syclone Validation Failed in Logic Pro (ARM64 m1)
2 participants