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] add onnx proto & meson setting #2891

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

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Jan 21, 2025

  • add onnx proto & compile it using meson protoc
  • add "enable-onnx-interpreter" meson option.

I'll upload a separate pr for the onnx interpreter script.

Self evaluation:

Build test: [x]Passed [ ]Failed [ ]Skipped
Run test: [x]Passed [ ]Failed [ ]Skipped

Signed-off-by: Seungbaek Hong sb92.hong@samsung.com

@baek2sm
Copy link
Contributor Author

baek2sm commented Jan 21, 2025

i'll check the doxygen issue (that compiled-onnx-schema script is automatically generated by the protocol buffer compiler with onnx schema file)

Copy link
Contributor

@EunjuYang EunjuYang left a comment

Choose a reason for hiding this comment

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

I have a question about the onnx proto.
Isn't the proto already defined for onnx format? What about adding proto file (not pb.cc & pb.h) and using it by compilation?

@DonghakPark
Copy link
Member

Member

For external file : i think we don't need consider doxygen
and for clang-format relate CI : you can use "// clang-format off ~~ // clang-format on " or just ignore that too

@myungjoo
Copy link
Member

myungjoo commented Jan 22, 2025

If onnx.pb.cc/h files are generated by protoc, add a onnx.proto file and let meson script generate .cc/.h files from it.

Or is there a reason you cannot generate files in the build scripts?

Reference: nnstreamer/nnstreamer@ff5c1fd

@baek2sm
Copy link
Contributor Author

baek2sm commented Jan 22, 2025

@EunjuYang @myungjoo Actually, I just didn't want to make a dependency on protoc..!
Then I'll add a onnx.proto file and compile it using meson script & protoc. and thanks for reference!

@baek2sm
Copy link
Contributor Author

baek2sm commented Jan 22, 2025

@DonghakPark Thank you for your detailed guide!!

@myungjoo
Copy link
Member

@EunjuYang @myungjoo Actually, I just didn't want to have a dependency on protoc..! But if that's not important, I'll add a onnx.proto file and compile it using meson script. thanks!

Having auto-generated files commited in a git repo is not recommended. It is like having .so file in the git repo where you can compile that .so file easily from available open source codes.

You already have a dependency to protobuf-devel. Adding protoc dependency shouldn't be a problem.
For ubuntu, add the following to /debian/control :

 protobuf-compiler (>=3.12), libprotobuf-dev,

For Tizen:

BuildRequires: protobuf-devel >= 3.4.0

For Android, unfortunately, you may need to touch a few more scripts. (I guess this is why you added the auto-generated files.) But you don't have any dependency issue for this; you just need to enforce developers to install a proper protobuf-compiler here.

compiler_sources += [
'onnx.pb.cc',
]
endif
Copy link
Member

@myungjoo myungjoo Jan 22, 2025

Choose a reason for hiding this comment

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

You need a script to generate onnx.pb.cc around here.

Hints:

# Protobuf compiler
pb_comp = find_program('protoc', required: get_option('protobuf-support'))

https://github.com/nnstreamer/nnstreamer/blob/ff5c1fd34230e4d72eee0711118fedef64f3b8a1/meson.build#L156-L159

if protobuf_support_is_available
  pb_gen = generator(pb_comp,
      output: ['@BASENAME@.pb.h', '@BASENAME@.pb.cc'],
      arguments : [
        '--proto_path=@CURRENT_SOURCE_DIR@/include',
        '--cpp_out=@BUILD_DIR@',
        '@INPUT@'
      ]
  )
  pb_gen_src = pb_gen.process('./include/nnstreamer.proto')
endif

https://github.com/nnstreamer/nnstreamer/blob/ff5c1fd34230e4d72eee0711118fedef64f3b8a1/ext/nnstreamer/meson.build#L8-L18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@myungjoo I haven't finished editing yet, but I accidentally pushed while working out of habit.
I'll refer to the code you provided, make the necessary changes, and push again. Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protobuf_dep = dependency('protobuf', required: true)
nntrainer_base_deps += protobuf_dep

protoc = find_program('protoc', required: true)  
pb_gen = generator(
      protoc,
      output: ['@BASENAME@.pb.cc', '@BASENAME@.pb.h'],
      arguments: ['--cpp_out=@BUILD_DIR@', '-I=@CURRENT_SOURCE_DIR@', '@INPUT@'],
    )
pb_gen_src = pb_gen.process('onnx.proto')
// + shared_library setting using pb_gen_src

@myungjoo I referred to the given code and tried to use the generator as shown above, and I think this method is more appropriate for this purpose. However, since the same meson script uses custom_target instead of generator for FlatBuffer and TFLite, I uploaded the code using custom_target to unify the code within the same script. How about updating all parts to use generator instead of custom_target at a later time(when I have some spare time)?

Copy link
Member

Choose a reason for hiding this comment

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

Yup. As long as you now have .proto file to generate .pb.cc/h automatically, it's all good.

@baek2sm baek2sm marked this pull request as draft January 22, 2025 12:12
@baek2sm baek2sm changed the title [ONNX] add onnx schema & meson setting [ONNX] add onnx proto & meson setting Jan 22, 2025
@baek2sm baek2sm removed the WIP label Jan 22, 2025
@baek2sm baek2sm marked this pull request as ready for review January 22, 2025 14:54
Copy link
Contributor

@EunjuYang EunjuYang left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for reflecting the opinion 👍

@jijoongmoon
Copy link
Collaborator

We do have tflite scheme file (fb) in compiler directory as well. I recommend to make scheme dir and put those in the same dir for better managing.

@baek2sm
Copy link
Contributor Author

baek2sm commented Jan 23, 2025

We do have tflite scheme file (fb) in compiler directory as well. I recommend to make scheme dir and put those in the same dir for better managing.

@jijoongmoon Thank you for your feedback. then I'll make a new commit for making a new directory and put those schema files(onnx, tflite, nntrainer schema) in the same directory!

- add onnx proto & compile it using meson protoc
- add "enable-onnx-interpreter" meson option.

I'll upload a separate pr for the onnx interpreter script.

**Self evaluation:**

Build test: [x]Passed [ ]Failed [ ]Skipped
Run test: [x]Passed [ ]Failed [ ]Skipped

Signed-off-by: Seungbaek Hong <sb92.hong@samsung.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants