Coremltools: 'Object' in VisionFeaturePrint.proto is invalid in JavaScript

[lutzroeder]

🐞Describe the bug

VisionFeaturePrint.proto in #731 defines a new message called Object. Since Object is a keyword in JavaScript, code generated in protobuf.js will fail with the following error:

TypeError: Object.create is not a function
    at ~/Projects/netron/src/coreml-proto.js:617:66
    at ~/Projects/netron/src/coreml-proto.js:621:27
    at ~/Projects/netron/src/coreml-proto.js:624:23

This is blocking adoption of the new Core ML .proto in JavaScript.

To Reproduce

1. Install Node.js and npm
2. git clone https://github.com/lutzroeder/netron.git
3. cd netron
4. npm install
5. ./tools/coreml sync schema - this will sync and update ./src/coreml-proto.js to latest
6. npx electron . - launches the app
7. Open Core ML model

[znation]

I think the problem is actually a bug in the protobuf.js code generator -- it's allowing name collision at different scopes. So the following generated code is fundamentally broken, because the name Object refers to the local Object here (defined by proto), not the global Object, but Object.create needs to refer to the global:

                        Object.ObjectVersion = (function() {
                            var valuesById = {}, values = Object.create(valuesById);
                            values[valuesById[0] = "OBJECT_VERSION_INVALID"] = 0;
                            values[valuesById[1] = "OBJECT_VERSION_1"] = 1;
                            return values;
                        })();

This generated code could instead use window.Object.create (to explicitly refer to the global instead of lexical scope) in a web browser; or a better fix that would work outside of a browser environment might be to grab a reference to the global Object in an outer lexical scope using a non-colliding name, and use that name here instead:

var globalObjectWithUniqueName = Object;
...
                        Object.ObjectVersion = (function() {
                            var valuesById = {}, values = globalObjectWithUniqueName.create(valuesById);
                            values[valuesById[0] = "OBJECT_VERSION_INVALID"] = 0;
                            values[valuesById[1] = "OBJECT_VERSION_1"] = 1;
                            return values;
                        })();

The command used by netron to generate this is:

pbjs -t static-module -w closure --no-encode --no-delimited --no-comments --no-convert --no-verify --no-create --keep-case -r coreml -o ./src/coreml-proto.js ./third_party/src/coremltools/mlmodel/format/Model.proto

Which appears to be using this 3rd party library for using Protobuf in JavaScript, not the official Protobuf library from Google (both seem to support JavaScript, but protobuf.js seems more popular for this use case). So my conclusion is that this is a bug in this library, and should be fixed there.

@lutzroeder, as a workaround, I think you could try one of the following:

1. Use a different mode of protobuf.js to consume the .proto - one of the reflection based approaches may not have the same bug as this bug may be specific to the static code approach.

2. Use Google's protobuf instead of protobuf.js. It claims to have JS support built-in. Since it is a different implementation it may not have the same bug.

[gauravka]

nice :)

On Jun 24, 2020, at 3:40 PM, Zach Nation [e-mail] wrote:

I think the problem is actually a bug in the protobuf.js code generator -- it's allowing name collision at different scopes. So the following generated code is fundamentally broken, because the name Object refers to the local Object here (defined by proto), not the global Object, but Object.create needs to refer to the global:

                    Object.ObjectVersion = (function() {
                        var valuesById = {}, values = Object.create(valuesById);
                        values[valuesById[0] = "OBJECT_VERSION_INVALID"] = 0;
                        values[valuesById[1] = "OBJECT_VERSION_1"] = 1;
                        return values;
                    })();

This generated code could instead use window.Object.create (to explicitly refer to the global instead of lexical scope) in a web browser; or a better fix that would work outside of a browser environment might be to grab a reference to the global Object in an outer lexical scope using a non-colliding name, and use that name here instead:

var globalObjectWithUniqueName = Object;
...
Object.ObjectVersion = (function() {
var valuesById = {}, values = globalObjectWithUniqueName.create(valuesById);
values[valuesById[0] = "OBJECT_VERSION_INVALID"] = 0;
values[valuesById[1] = "OBJECT_VERSION_1"] = 1;
return values;
})();
The command used by netron to generate this is:

pbjs -t static-module -w closure --no-encode --no-delimited --no-comments --no-convert --no-verify --no-create --keep-case -r coreml -o ./src/coreml-proto.js ./third_party/src/coremltools/mlmodel/format/Model.proto

Which appears to be using this 3rd party library for using Protobuf in JavaScript https://github.com/protobufjs/protobuf.js, not the official Protobuf library from Google (both seem to support JavaScript, but protobuf.js seems more popular for this use case). So my conclusion is that this is a bug in this library, and should be fixed there.

@lutzroeder https://github.com/lutzroeder, as a workaround, I think you could try one of the following:

Use a different mode of protobuf.js to consume the .proto - one of the reflection based https://github.com/protobufjs/protobuf.js#reflection-vs-static-code approaches may not have the same bug as this bug may be specific to the static code approach.

Use Google's protobuf https://github.com/protocolbuffers/protobuf instead of protobuf.js. It claims to have JS support built-in. Since it is a different implementation it may not have the same bug.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub https://github.com/apple/coremltools/issues/734#issuecomment-649110320, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGPTFOWPZ67HL76W5AUTPTRYJ6GHANCNFSM4OGHM3ZQ.

[lutzroeder]

@znation Having been in those discussions a few times my recommendation would be to consider renaming the API. It should be a non-breaking change and would be easier to do now when things aren't set in stone. Otherwise you'd have to work with the folks who maintain protobuf.js in their spare time to get this fixed. In the bigger scheme of things I'd rather have them pay attention to other items like merging JavaScript .pbtxt support. Switching to the other implementations isn't an option as it has other limitations and would be a very expensive task in the context of having to support 10+ protobuf formats for no reason other than working around this naming issue. Object is such an overloaded term in programming, using it tends to lead to problems and confusion that can be avoided.

[anilkatti]

@lutzroeder that's a fair point. We will address this in Core ML. I will let you know once we have a fix for you to try.