onnx / onnx-tensorflow

Tensorflow Backend for ONNX
Other
1.27k stars 296 forks source link

Must ONNX be installed from source for ONNX-TF to work? If so, say so. #173

Open philferriere opened 6 years ago

philferriere commented 6 years ago

pypi's ONNX is version 1.1.2 (see here). conda-forge's ONNX version is 1.1.1 (see here)

ONN-TF's frontend.py uses API calls that, I believe, have been introduced after those releases (see issue #158). Here's an example:

Latest onnx\helper.py:

def make_graph(
    nodes,  # type: Sequence[NodeProto]
    name,  # type: Text
    inputs,  # type: Sequence[ValueInfoProto]
    outputs,  # type: Sequence[ValueInfoProto]
    initializer=None,  # type: Optional[Sequence[TensorProto]]
    doc_string=None,  # type: Optional[Text]
    value_info=[],  # type: Sequence[ValueInfoProto]
):  # type: (...) -> GraphProto

Releases' onnx\helper.py:

def make_graph(nodes, name, inputs, outputs, initializer=None, doc_string=None):

The README.md for ONNX-TF needs to be updated to point out that ONNX must be installed and built from source. I can only encourage you to warn developers of something like this, because it's a big deal (and definitely a deal breaker for me). Or, did I miss something @tjingrant @fumihwh?

tjingrant commented 6 years ago

First of all, please accept my sincere apology for such an oversight; you are not missing anything and it is an undeniable mistake on our side. We need to start matching our release with that of ONNX.

Our current priority has been correctness, which is why we test with onnx master in our Travis CI. This is already a non-trivial amount of commitment since ONNX continues to publish ops and tests at a fast pace. To resolve this unfortunate oversight, I will try to make sure that we test against the latest release as well.

On the other hand, please bear with me for the cliché: please understand that this project has been, from day one, a community project. Therefore, when you see such a deal breaker, please remember that:

  1. There is no deal, you are taking what we give for free and our companies do not make money out of these contributions, which means we cannot be fully committed to developing onnx-tensorflow.
  2. This is a "deal breaker" you have every power to fix or prevent. The moral imperative of an open source community is such that when a problem like this occurs, it is everyone's problem.

@philferriere I absolutely appreciate your high standards and patience to direct us along the way and we strive to keep up. But please be aware of the constraints we face and that the resource we can command is significantly less than those available to the ONNX main community. And certainly, please stop throwing out comments like this in an issue description. Nonetheless, I hope to address your concerns very soon.

philferriere commented 6 years ago

Good to know. I sincerely wish you the best of luck with your project @tjingrant!

tjingrant commented 6 years ago

Hello @philferriere, several actions have been taken to ensure that a serious mistake like this does not occur again.

  1. (PR https://github.com/onnx/onnx-tensorflow/pull/174) The matching ONNX release version has been added in this file and in our Travis continuous integration test, we now test ONNX-TF with the ONNX release version specified in this file in addition to ONNX master. In the future, we will know the breakage of backward compatibilities as soon as they are introduced.
  2. (PR https://github.com/onnx/onnx-tensorflow/pull/180) We have updated our installation documentation to explicitly disclose to the user our ONNX version requirement.
  3. (PR https://github.com/onnx/onnx-tensorflow/pull/179) We have fixed several known breakage of backward compatibility in our frontends.

We have also uploaded a new release containing these patches. Thank you again for pointing out our mistakes.