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

Adding a Go definition file #1328

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@owulveryck

owulveryck commented Aug 25, 2018

I am trying to implement a completely new backend in Go.

Purpose of the PR

To add an official Go Definition/API to the project.

Why do I do this PR:

There is no, at the present time, Go API for ONNX. Everyone needs to compile it on their own.
I think that the community would benefit an "Official API".

This file I could be hosted it in any third party repo but:

  • this shall not be manually modified;
  • It has to be generated by the official protobuf definition;
  • It has to be the official (and only) API for any backend implementation in the Go language.

About the code

The Go file has not been modified manually and is the result of a processing by protocol buffer compilers only. I have simply added a Makefile to be able to reproduce this step.
It could be good to add the mechanism into the ONNX integration mechanism but I have no clue on how to do that.

owulveryck added some commits Aug 24, 2018

@bddppq

This comment has been minimized.

Member

bddppq commented Aug 25, 2018

I personally like Go and I like the idea of enabling the Go community to be able to access ONNX. But:

  1. I don't think we should put the auto-generated onnx.pb.go file into source control. Similar for Python and C++, we add a step in the build system to auto-generate the language specific api files on the fly

    onnx/CMakeLists.txt

    Lines 234 to 239 in 6146a85

    add_custom_command(
    OUTPUT "${OUTPUT_PB_SRC}" "${OUTPUT_PB_HEADER}"
    COMMAND ${ONNX_PROTOC_EXECUTABLE} ARGS ${PROTOC_ARGS}
    DEPENDS ${GENERATED_PROTO} ${DEPEND}
    COMMENT "Running C++ protocol buffer compiler on ${GENERATED_PROTO}"
    VERBATIM)
  2. I have to admit at this point we don't have enough resources to maintain one more language interface. I would suggest let's add a separate repo like https://github.com/onnx/onnx-r to host the go interface and grant you write access to the repo (if you are willing to be responsible for the maintenance work for it). @jspisak, @prasanthpul @lupesko

@bddppq bddppq self-requested a review Aug 25, 2018

@owulveryck

This comment has been minimized.

owulveryck commented Aug 25, 2018

The separate repo sounds like the best idea indeed. And to be honest, it will be a lot better to have a lighter repo. It will be more efficient for "go-getting" and update it.

@jspisak

This comment has been minimized.

Collaborator

jspisak commented Aug 25, 2018

Completely agree. A separate repo is definitely the way to go. @owulveryck - thanks for contributing to the project! cc @houseroad

@owulveryck

This comment has been minimized.

owulveryck commented Aug 27, 2018

How do we proceed then?
Is there any special Licence required for the repo? Any requirement that needs to be fulfilled? I have read the contribution guide but nothing seems to be related to this.

@houseroad

This comment has been minimized.

Member

houseroad commented Aug 27, 2018

@owulveryck you may want to take a look at https://github.com/onnx/onnx-r

Since ONNX uses MIT License, you can stick to it you like. Setting up the CI and running tests will be recommended.

If you can create one repo under your user account for pre-review, it will be great.

@bddppq

This comment has been minimized.

Member

bddppq commented Aug 28, 2018

Closing this one as discussed.

@bddppq bddppq closed this Aug 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment