Implement function retrieval APIs; Add documentation for functions #1112

raymondxyang · 2018-06-12T23:37:48Z

Implement function retrieval python APIs on existing ABIs
Add documentation generation scripts for functions
Minor fixes on dependencies and configurations in CMake files

… build issues

linkerzhang · 2018-06-18T04:01:27Z

onnx/defs/function.h

@@ -57,27 +57,4 @@ class FunctionBuilderRegistry {
  static Common::Status function_builder_##counter##_status = \
      FunctionBuilderRegistry::OnnxInstance().Register(function_builder);

-// Example to register a function.


please keep it here as an example.

linkerzhang · 2018-06-18T04:02:01Z

onnx/defs/experiments/functions.cc

+#include "onnx/defs/function.h"
+using namespace ONNX_NAMESPACE;
+
+static Common::Status BuildFc(std::unique_ptr<FunctionProto>* func_proto) {


Let's switch MeanVarianceNormalization to be a function. FC was removed/deprecated.

linkerzhang · 2018-06-22T15:43:56Z

onnx/defs/schema.cc

@@ -191,6 +190,14 @@ void OpSchema::Verify(const NodeProto& node) const {
      fail_check("Unrecognized attribute: ", name);
    }

+    if (attr_proto.has_ref_attr_name()) {


this if should be removed?

The if logic here determine whether this is a solid attribute or a placeholder attribute. I extract the if- logic here for checks on placeholder attr_proto going under this condition. IMO it is a valid condition checker here.

linkerzhang · 2018-06-22T15:50:22Z

onnx/defs/function.cc

+        OpSchemaRegistry::DomainToVersionRange::Instance().Map().at(
+            func_builder.GetDomain());
+    op_set.insert({func_builder.GetDomain(),
+                   function_proto->since_version() < version_range.second


if function's since version is greater than the version_range.second, it should be an error, I think.

bddppq · 2018-07-11T19:00:37Z

CMakeLists.txt

@@ -30,6 +30,8 @@ list(APPEND CMAKE_MODULE_PATH ${ONNX_ROOT}/cmake/Modules)
 if(NOT MSVC)
  set(CMAKE_C_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0")
  set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0")
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pthread")


why is this needed?

bddppq · 2018-07-11T19:07:30Z

onnx/defs/schema.cc

@@ -191,6 +190,14 @@ void OpSchema::Verify(const NodeProto& node) const {
      fail_check("Unrecognized attribute: ", name, " for operator ", node.op_type());
    }

+    if (attr_proto.has_ref_attr_name()) {
+      if (!attr_proto.has_type() || attr_proto.type() != expected_type) {


looks to me this check is also applicable when the attr doesn't have ref_attr_name (however you will need to guard the has_type check by ir_version, IIRC type was added in ir_version 2

Actually if it is a solid attr_proto the validation will be covered under checker.cc L199-208.. This validation logic is aimed to be only conducted on the referencing attr_proto.

sorry if this check is already done in checker, why do we check it again here?

There is no type check for referencing attr_proto, I tested removing this and there is no enforcement for such attr during the registration (ie change attr0->set_type(AttributeProto_AttributeType_INTS) to INT and it can still pass the check)

bddppq · 2018-07-11T19:09:44Z

CMakeLists.txt

@@ -361,6 +363,7 @@ if (MSVC)
        /wd4800 # disable warning type' : forcing value to bool 'true' or 'false' (performance warning)
        /wd4503 # identifier' : decorated name length exceeded, name was truncated
        /wd4146 # unary minus operator applied to unsigned type, result still unsigned
+        /wd4244 # conversion from 'protobuf::int64' to 'int'


where does such cast happen? do you also need to ignore this warning in other targets?

This happens when I convert fields in functionproto in cpp2py.. I only notice the warning in onnx target..

at least you would also need to ignore it in other platforms as well. maybe what's better is to find the place in pybind11 binding and fix it?

linkerzhang · 2018-07-11T22:39:43Z

Thank you!

bddppq · 2018-07-12T01:00:06Z

onnx/cpp2py_export.cc

@@ -96,6 +97,57 @@ PYBIND11_MODULE(onnx_cpp2py_export, onnx_cpp2py_export) {
      .value("COMMON", OpSchema::SupportType::COMMON)
      .value("EXPERIMENTAL", OpSchema::SupportType::EXPERIMENTAL);

+  py::class_<FunctionProto> function_proto(defs, "FunctionProto");


This is not good, because python land already has a FunctionProto class from the auto-generated protobuf python files (onnx.FunctionProto), and its underlying memory layout is totally different from the c++ protobuf structs, and so python land users will see two classes with the same name while they are completely different. what we have been doing when passing protobuf structs across c++ and python is serialize on one side and then deserialize on the other side (look at check_model)

Currently the solution is blocked by #1194 (and seems the import issue is in discussion in protobuf repo as well). I could rename the generated pybind class (to function_schema probably) and explore a way to resolve #1194 (probably need to manually run text replacement scripts), switch to use function_proto later. This class is only used for the display of function information now so there is no use case for users to really create one using function classes in python module.

Found a way to manually correct the import in build. Pushed.

Comment resolved

bddppq · 2018-07-14T16:39:14Z

onnx/test/c++/function_get_test.cc

+  std::multimap<std::string, std::unique_ptr<FunctionProto>> temp_map;
+  FunctionBuilderRegistry& function_registry =
+      FunctionBuilderRegistry::OnnxInstance();
+  Common::Status status = function_registry.GetFunctions("", &temp_map);


change to use the ONNX_DOMAIN instead of hardcode ""?

fixed. Thanks for pointing out 👍

bddppq · 2018-07-18T05:52:58Z

onnx/__init__.py

@@ -6,6 +6,7 @@
 from .onnx_pb import *  # noqa
 from .onnx_operators_pb import * # noqa
 from .version import version as __version__  # noqa
+from .onnx_operators_pb import *  # noqa


bddppq · 2018-07-18T05:57:23Z

onnx/defs/schema.cc

@@ -191,6 +190,14 @@ void OpSchema::Verify(const NodeProto& node) const {
      fail_check("Unrecognized attribute: ", name, " for operator ", node.op_type());
    }

+    if (attr_proto.has_ref_attr_name()) {
+      if (!attr_proto.has_type() || attr_proto.type() != expected_type) {


sorry if this check is already done in checker, why do we check it again here?

bddppq · 2018-07-18T05:59:32Z

onnx/defs/gen_doc.py


 if __name__ == '__main__':
    base_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
    docs_dir = os.path.join(base_dir, 'docs')

    class Args(object):
        output = os.path.join(docs_dir, 'Operators' + ext)
+        function_output = os.path.join(docs_dir, 'Functions' + ext)


Actually how about putting Functions and Operators together?

IMO the formatting and visualization could be a future task. Currently I am trying to have type/shape inference of function working and may change the visualization accordingly. And we also talked with Netron people and try to have image generated for the functions.

good suggestion, let's merge the two documents into one in next PR. Thank you!

all comments except this resolved

bddppq · 2018-07-18T06:00:22Z

onnx/defs/gen_doc.py

 from onnx.backend.test.case import collect_snippets
 from typing import Text, Sequence, Dict, List, Type, Set, Tuple


 SNIPPETS = collect_snippets()
 ONNX_ML = bool(os.getenv('ONNX_ML') == '1')
+ONNX_DOMAIN = ""
+ONNX_ML_DOMAIN = 'ai.onnx.ml'


onnx.defs has ONNX_DOMAIN defined, maybe also move ONNX_ML_DOMAIN to there and here will just refer them through onnx.defs?

bddppq · 2018-07-18T06:00:49Z

onnx/defs/gen_doc.py

@@ -9,13 +9,16 @@
 from collections import defaultdict

 from onnx import defs
-from onnx.defs import OpSchema
+from onnx.defs import OpSchema, FunctionProto


FunctionProto should be imported from onnx

bddppq · 2018-07-18T06:02:01Z

onnx/defs/function.h

@@ -7,6 +7,7 @@
 #include <string>
 #include <vector>

+#include "onnx/common/constants.h"


nit: move this include to function.cc? (since it's used there)

bddppq · 2018-07-18T06:03:25Z

onnx/cpp2py_export.cc

+        FunctionBuilderRegistry& function_registry =
+            FunctionBuilderRegistry::OnnxInstance();
+
+        Common::Status status =


check status?

bddppq · 2018-07-18T06:04:18Z

onnx/cpp2py_export.cc

+                std::unordered_map<std::string, std::vector<py::bytes>>::
+                    value_type(iter->first, tmp_vec));
+          } else {
+            temp_map.at(iter->first).emplace_back(py::bytes(bytes));


std::move bytes?

bddppq · 2018-07-18T06:06:04Z

onnx/cpp2py_export.cc

+                "Failed to serilize registered function for '" + iter->first +
+                "'!");
+          }
+          if (!temp_map.count(iter->first)) {


you can use [] of unordered_map to implicitly create an entry if the key doesn't already exist.

bddppq

Thanks for patiently addressing all comments!

raymondxyang · 2018-07-19T07:04:25Z

Thanks for the detailed review :)

…nnx#1112) * Add for test function build * Add pybind functions for getFunction API * Implement FC function testcase * Lint code using clang-format * Disable warning in covert protobuf::int64 to int * Update doc generation scripts and functions doc * Trimming some comments * Fix doc generation scripts for typecheck * Improve cmake scripts for unittest project dependencies and fix linux build issues * Lint python code * Update API to support functions with multiple versions * Update line ending * Add changelogs for functions * Refactor and lint code * Fix bad linebreaks in markdown files generated * Resolve PR comments; Refine Function type check implementation * Add checker for function version; Bump version in onnx_domain * Update doc_string and generated md files * Add helper function; Update documents for MVN function * Fix a bug in MVN function definition * Experimental: add python test case for MVN * Update line ends * Add attribute mvn testcase * Update generated files on MVN function * Rename buildnode function * Remove node part in generated files; Reformat doc_string for MVN * Fix changelog generation script * Update MVN testcase to caculate expected output on the fly * Rephrase description for MVN function * Formatting docstring for MVN function * Refactor helper function code; Add epsilon before division * Move the helper functions into common folder * Fix typo in function docstring * Refactor model helper function; Add version check; Fix typo * Switch to use FunctionProto class generated in python * Remove unnecessary ignore flag * Minor fix in c++ tests; Generated function docs for ML domain * Refactor code to avoid hardcode domain name * Resolve PR comments

Raymond Yang added 15 commits May 22, 2018 15:58

Add for test function build

f14cabb

Add pybind functions for getFunction API

cdbc593

Merge branch 'master' of github.com:onnx/onnx into function-doc-gen

d251325

Implement FC function testcase

cc7bdf4

Lint code using clang-format

2a4fd29

Disable warning in covert protobuf::int64 to int

65ab836

Update doc generation scripts and functions doc

83d5328

Trimming some comments

668c3bd

Fix doc generation scripts for typecheck

fcfd814

Improve cmake scripts for unittest project dependencies and fix linux…

826da02

… build issues

Lint python code

e11baa5

Update API to support functions with multiple versions

03970b5

Update line ending

c3639d1

Add changelogs for functions

3bca586

Refactor and lint code

fd7b65f

raymondxyang requested review from linkerzhang and bddppq June 12, 2018 23:37

raymondxyang force-pushed the function-doc-gen branch from 0384a6e to 587498e Compare June 12, 2018 23:42

Fix bad linebreaks in markdown files generated

cb82317

raymondxyang force-pushed the function-doc-gen branch from 587498e to cb82317 Compare June 12, 2018 23:56

linkerzhang reviewed Jun 18, 2018

View reviewed changes

Resolve PR comments; Refine Function type check implementation

7bd3ded

raymondxyang force-pushed the function-doc-gen branch from 1274128 to 7bd3ded Compare June 22, 2018 01:06

linkerzhang reviewed Jun 22, 2018

View reviewed changes

Raymond Yang added 3 commits June 22, 2018 11:21

Merge branch 'master' of github.com:onnx/onnx into function-doc-gen

411a5c7

Add checker for function version; Bump version in onnx_domain

1bb892d

Update doc_string and generated md files

bbeffa0

raymondxyang force-pushed the function-doc-gen branch from 3ac7c46 to bbeffa0 Compare June 22, 2018 21:13

bddppq reviewed Jul 11, 2018

View reviewed changes

Refactor model helper function; Add version check; Fix typo

5f3794e

raymondxyang force-pushed the function-doc-gen branch from a52a1c6 to 5f3794e Compare July 11, 2018 21:56

linkerzhang approved these changes Jul 11, 2018

View reviewed changes

bddppq suggested changes Jul 12, 2018

View reviewed changes

Raymond Yang added 2 commits July 13, 2018 17:19

Switch to use FunctionProto class generated in python

9579398

Merge branch 'master' into function-doc-gen

c4a25f7

raymondxyang force-pushed the function-doc-gen branch 4 times, most recently from 9761f75 to 0ee0adf Compare July 14, 2018 04:44

wschin approved these changes Jul 16, 2018

View reviewed changes

bddppq reviewed Jul 16, 2018

View reviewed changes

Raymond Yang added 3 commits July 16, 2018 15:34

Remove unnecessary ignore flag

9c6a01e

Merge branch 'master' of github.com:onnx/onnx into function-doc-gen

f07f977

Minor fix in c++ tests; Generated function docs for ML domain

f081fd6

raymondxyang force-pushed the function-doc-gen branch from 0ee0adf to f081fd6 Compare July 16, 2018 23:09

Refactor code to avoid hardcode domain name

abad4cc

bddppq reviewed Jul 18, 2018

View reviewed changes

Raymond Yang added 2 commits July 18, 2018 11:44

Resolve PR comments

d22b043

Merge branch 'master' of github.com:onnx/onnx into function-doc-gen

293c4c8

raymondxyang force-pushed the function-doc-gen branch from 5a7897a to 293c4c8 Compare July 18, 2018 18:45

Merge branch 'master' into function-doc-gen

af7009e

bddppq approved these changes Jul 19, 2018

View reviewed changes

linkerzhang merged commit f09d8d6 into onnx:master Jul 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement function retrieval APIs; Add documentation for functions #1112

Implement function retrieval APIs; Add documentation for functions #1112

raymondxyang commented Jun 12, 2018

linkerzhang Jun 18, 2018

linkerzhang Jun 18, 2018

linkerzhang Jun 22, 2018

raymondxyang Jun 22, 2018

linkerzhang Jun 22, 2018

bddppq Jul 11, 2018

bddppq Jul 11, 2018

raymondxyang Jul 11, 2018 •

edited

bddppq Jul 18, 2018

raymondxyang Jul 18, 2018 •

edited

bddppq Jul 11, 2018

raymondxyang Jul 11, 2018

bddppq Jul 12, 2018

linkerzhang commented Jul 11, 2018

bddppq Jul 12, 2018

raymondxyang Jul 13, 2018 •

edited

raymondxyang Jul 14, 2018

raymondxyang Jul 17, 2018

bddppq Jul 14, 2018

raymondxyang Jul 17, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

raymondxyang Jul 18, 2018

linkerzhang Jul 19, 2018

raymondxyang Jul 19, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq Jul 18, 2018

bddppq left a comment

raymondxyang commented Jul 19, 2018

Implement function retrieval APIs; Add documentation for functions #1112

Implement function retrieval APIs; Add documentation for functions #1112

Conversation

raymondxyang commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondxyang Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondxyang Jul 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linkerzhang commented Jul 11, 2018

Choose a reason for hiding this comment

raymondxyang Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bddppq left a comment

Choose a reason for hiding this comment

raymondxyang commented Jul 19, 2018

raymondxyang Jul 11, 2018 •

edited

raymondxyang Jul 18, 2018 •

edited

raymondxyang Jul 13, 2018 •

edited